Обсуждение: [HACKERS] Refreshing subscription relation state inside a transaction block

Hi,

The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
workers stop when subscription relation entry is removed. It doesn't
work fine inside transaction block. I think we should disallow to use
the following subscription DDLs inside a transaction block. Attached
patch.

* ALTER SUBSCRIPTION SET PUBLICATION WITH (refresh = true)
* ALTER SUBSCRIPTION REFRESH PUBLICATION

Also, even if we rollback ALTER SUBSCRIPTION REFRESH PUBLICATION, the
error message "NOTICE:  removed subscription for table public.t1"
appears. It's not a bug but might confuse the user.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 13/06/17 09:06, Masahiko Sawada wrote:
> Hi,
> 
> The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
> workers stop when subscription relation entry is removed. It doesn't
> work fine inside transaction block. I think we should disallow to use
> the following subscription DDLs inside a transaction block. Attached
> patch.
> 

Can you be more specific than "It doesn't work fine inside transaction
block", what do you expect to happen and what actually happens?

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 13/06/17 09:06, Masahiko Sawada wrote:
>> Hi,
>>
>> The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
>> workers stop when subscription relation entry is removed. It doesn't
>> work fine inside transaction block. I think we should disallow to use
>> the following subscription DDLs inside a transaction block. Attached
>> patch.
>>
>
> Can you be more specific than "It doesn't work fine inside transaction
> block", what do you expect to happen and what actually happens?
>

If we do ALTER SUBSCRIPTION SET PUBLICATION during executing table
sync then it forcibly stops concurrently running table sync worker for
a table that had been removed from pg_subscription_rel. Then if we
rollback the transaction then these table sync worker start again from
the first. it's an rarely situation and not a damaging behavior but
what I expected is the table sync worker is stopped when the
refreshing transaction is committed.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Wed, Jun 14, 2017 at 1:02 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> On 13/06/17 09:06, Masahiko Sawada wrote:
>>> Hi,
>>>
>>> The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
>>> workers stop when subscription relation entry is removed. It doesn't
>>> work fine inside transaction block. I think we should disallow to use
>>> the following subscription DDLs inside a transaction block. Attached
>>> patch.
>>>
>>
>> Can you be more specific than "It doesn't work fine inside transaction
>> block", what do you expect to happen and what actually happens?
>>
>
> If we do ALTER SUBSCRIPTION SET PUBLICATION during executing table
> sync then it forcibly stops concurrently running table sync worker for
> a table that had been removed from pg_subscription_rel.

Also, until commit the transaction the worker cannot launch new table
sync worker due to conflicting tuple lock on pg_subscription_rel.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 13/06/17 18:33, Masahiko Sawada wrote:
> On Wed, Jun 14, 2017 at 1:02 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> On 13/06/17 09:06, Masahiko Sawada wrote:
>>>> Hi,
>>>>
>>>> The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
>>>> workers stop when subscription relation entry is removed. It doesn't
>>>> work fine inside transaction block. I think we should disallow to use
>>>> the following subscription DDLs inside a transaction block. Attached
>>>> patch.
>>>>
>>>
>>> Can you be more specific than "It doesn't work fine inside transaction
>>> block", what do you expect to happen and what actually happens?
>>>
>>
>> If we do ALTER SUBSCRIPTION SET PUBLICATION during executing table
>> sync then it forcibly stops concurrently running table sync worker for
>> a table that had been removed from pg_subscription_rel.
> 

Hmm, forcibly stopping currently running table sync is not what was
intended, I'll have to look into it. We should not be forcibly stopping
anything except the main apply worker during drop subscription (and we
do that only because we can't drop the remote replication slot otherwise).

> Also, until commit the transaction the worker cannot launch new table
> sync worker due to conflicting tuple lock on pg_subscription_rel.
> 

That part is correct, we don't know if the transaction will be rolled
back or not so we can't start workers as the state of the data in the
table in unknown.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 6/15/17 02:41, Petr Jelinek wrote:
> Hmm, forcibly stopping currently running table sync is not what was
> intended, I'll have to look into it. We should not be forcibly stopping
> anything except the main apply worker during drop subscription (and we
> do that only because we can't drop the remote replication slot otherwise).

The change being complained about was specifically to address the
problem described in the commit message:
   Stop table sync workers when subscription relation entry is removed
   When a table sync worker is in waiting state and the subscription table   entry is removed because of a concurrent
subscriptionrefresh, the   worker could be left orphaned.  To avoid that, explicitly stop the   worker when the
pg_subscription_relentry is removed.
 


Maybe that wasn't the best solution.  Alternatively, the tablesync
worker has to check itself whether the subscription relation entry has
disappeared, or we need a post-commit check to remove orphaned workers.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 15/06/17 15:52, Peter Eisentraut wrote:
> On 6/15/17 02:41, Petr Jelinek wrote:
>> Hmm, forcibly stopping currently running table sync is not what was
>> intended, I'll have to look into it. We should not be forcibly stopping
>> anything except the main apply worker during drop subscription (and we
>> do that only because we can't drop the remote replication slot otherwise).
> 
> The change being complained about was specifically to address the
> problem described in the commit message:
> 
>     Stop table sync workers when subscription relation entry is removed
> 
>     When a table sync worker is in waiting state and the subscription table
>     entry is removed because of a concurrent subscription refresh, the
>     worker could be left orphaned.  To avoid that, explicitly stop the
>     worker when the pg_subscription_rel entry is removed.
> 
> 
> Maybe that wasn't the best solution.  Alternatively, the tablesync
> worker has to check itself whether the subscription relation entry has
> disappeared, or we need a post-commit check to remove orphaned workers.
> 

Ah I missed that commit/discussion, otherwise I would have probably
complained. I don't like killing workers in the DDL command, as I said
the only reason we do it in DropSubscription is to free the slot for
dropping.

I think that either of the options you suggested now would be better.
We'll need that for stopping the tablesync which is in progress during
DropSubscription as well as those will currently still keep running. I
guess we could simply just register syscache callback, the only problem
with that is we'd need to AcceptInvalidationMessages regularly while we
do the COPY which is not exactly free so maybe killing at the end of
transaction would be better (both for refresh and drop)?

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On Thu, Jun 15, 2017 at 11:49 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 15/06/17 15:52, Peter Eisentraut wrote:
>> On 6/15/17 02:41, Petr Jelinek wrote:
>>> Hmm, forcibly stopping currently running table sync is not what was
>>> intended, I'll have to look into it. We should not be forcibly stopping
>>> anything except the main apply worker during drop subscription (and we
>>> do that only because we can't drop the remote replication slot otherwise).
>>
>> The change being complained about was specifically to address the
>> problem described in the commit message:
>>
>>     Stop table sync workers when subscription relation entry is removed
>>
>>     When a table sync worker is in waiting state and the subscription table
>>     entry is removed because of a concurrent subscription refresh, the
>>     worker could be left orphaned.  To avoid that, explicitly stop the
>>     worker when the pg_subscription_rel entry is removed.
>>
>>
>> Maybe that wasn't the best solution.  Alternatively, the tablesync
>> worker has to check itself whether the subscription relation entry has
>> disappeared, or we need a post-commit check to remove orphaned workers.
>>
>
> Ah I missed that commit/discussion, otherwise I would have probably
> complained. I don't like killing workers in the DDL command, as I said
> the only reason we do it in DropSubscription is to free the slot for
> dropping.
>
> I think that either of the options you suggested now would be better.
> We'll need that for stopping the tablesync which is in progress during
> DropSubscription as well as those will currently still keep running. I
> guess we could simply just register syscache callback, the only problem
> with that is we'd need to AcceptInvalidationMessages regularly while we
> do the COPY which is not exactly free so maybe killing at the end of
> transaction would be better (both for refresh and drop)?
>

Attached patch makes table sync worker check its relation subscription
state at the end of COPY and exits if it has disappeared, instead of
killing table sync worker in DDL. There is still a problem that a
table sync worker for a large table can hold a slot for a long time
even after its state is deleted. But it would be for PG11 item.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> I think that either of the options you suggested now would be better.
>> We'll need that for stopping the tablesync which is in progress during
>> DropSubscription as well as those will currently still keep running. I
>> guess we could simply just register syscache callback, the only problem
>> with that is we'd need to AcceptInvalidationMessages regularly while we
>> do the COPY which is not exactly free so maybe killing at the end of
>> transaction would be better (both for refresh and drop)?
>
> Attached patch makes table sync worker check its relation subscription
> state at the end of COPY and exits if it has disappeared, instead of
> killing table sync worker in DDL. There is still a problem that a
> table sync worker for a large table can hold a slot for a long time
> even after its state is deleted. But it would be for PG11 item.

Do we still need to do something about this?  Should it be an open item?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jul 26, 2017 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>> I think that either of the options you suggested now would be better.
>>> We'll need that for stopping the tablesync which is in progress during
>>> DropSubscription as well as those will currently still keep running. I
>>> guess we could simply just register syscache callback, the only problem
>>> with that is we'd need to AcceptInvalidationMessages regularly while we
>>> do the COPY which is not exactly free so maybe killing at the end of
>>> transaction would be better (both for refresh and drop)?
>>
>> Attached patch makes table sync worker check its relation subscription
>> state at the end of COPY and exits if it has disappeared, instead of
>> killing table sync worker in DDL. There is still a problem that a
>> table sync worker for a large table can hold a slot for a long time
>> even after its state is deleted. But it would be for PG11 item.
>
> Do we still need to do something about this?  Should it be an open item?
>

Thank you for looking at this.

Yeah, I think it should be added to the open item list. The patch is
updated by Petr and discussed on another thread[1] that also addresses
other issues of subscription codes. 0004 patch of that thread is an
updated patch of the patch attached on this thread.

[1] https://www.postgresql.org/message-id/4c6e7ab3-091e-6336-df38-28937b153e64%402ndquadrant.com

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Jul 27, 2017 at 9:31 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Wed, Jul 26, 2017 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>> I think that either of the options you suggested now would be better.
>>>> We'll need that for stopping the tablesync which is in progress during
>>>> DropSubscription as well as those will currently still keep running. I
>>>> guess we could simply just register syscache callback, the only problem
>>>> with that is we'd need to AcceptInvalidationMessages regularly while we
>>>> do the COPY which is not exactly free so maybe killing at the end of
>>>> transaction would be better (both for refresh and drop)?
>>>
>>> Attached patch makes table sync worker check its relation subscription
>>> state at the end of COPY and exits if it has disappeared, instead of
>>> killing table sync worker in DDL. There is still a problem that a
>>> table sync worker for a large table can hold a slot for a long time
>>> even after its state is deleted. But it would be for PG11 item.
>>
>> Do we still need to do something about this?  Should it be an open item?
>>
>
> Thank you for looking at this.
>
> Yeah, I think it should be added to the open item list. The patch is
> updated by Petr and discussed on another thread[1] that also addresses
> other issues of subscription codes. 0004 patch of that thread is an
> updated patch of the patch attached on this thread.
>

Does anyone have any opinions? Barring any objections, I'll add this
to the open item list.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Jul 28, 2017 at 1:13 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Thu, Jul 27, 2017 at 9:31 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> On Wed, Jul 26, 2017 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>>>> I think that either of the options you suggested now would be better.
>>>>> We'll need that for stopping the tablesync which is in progress during
>>>>> DropSubscription as well as those will currently still keep running. I
>>>>> guess we could simply just register syscache callback, the only problem
>>>>> with that is we'd need to AcceptInvalidationMessages regularly while we
>>>>> do the COPY which is not exactly free so maybe killing at the end of
>>>>> transaction would be better (both for refresh and drop)?
>>>>
>>>> Attached patch makes table sync worker check its relation subscription
>>>> state at the end of COPY and exits if it has disappeared, instead of
>>>> killing table sync worker in DDL. There is still a problem that a
>>>> table sync worker for a large table can hold a slot for a long time
>>>> even after its state is deleted. But it would be for PG11 item.
>>>
>>> Do we still need to do something about this?  Should it be an open item?
>>>
>>
>> Thank you for looking at this.
>>
>> Yeah, I think it should be added to the open item list. The patch is
>> updated by Petr and discussed on another thread[1] that also addresses
>> other issues of subscription codes. 0004 patch of that thread is an
>> updated patch of the patch attached on this thread.
>>
>
> Does anyone have any opinions? Barring any objections, I'll add this
> to the open item list.
>

Added it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center