Re: [HACKERS] Refreshing subscription relation state inside a transaction block
On Fri, Jul 28, 2017 at 1:13 PM, Masahiko Sawadawrote: > On Thu, Jul 27, 2017 at 9:31 AM, Masahiko Sawada > wrote: >> On Wed, Jul 26, 2017 at 10:29 PM, Robert Haas wrote: >>> On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada >>> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
On Thu, Jul 27, 2017 at 9:31 AM, Masahiko Sawadawrote: > On Wed, Jul 26, 2017 at 10:29 PM, Robert Haas wrote: >> On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
On Wed, Jul 26, 2017 at 10:29 PM, Robert Haaswrote: > On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawadawrote: >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
On Thu, Jun 15, 2017 at 11:49 PM, Petr Jelinekwrote: > 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 check_subscription_rel_state_after_copy.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
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. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
On 13/06/17 18:33, Masahiko Sawada wrote: > On Wed, Jun 14, 2017 at 1:02 AM, Masahiko Sawada> wrote: >> On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinek >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
On Wed, Jun 14, 2017 at 1:02 AM, Masahiko Sawadawrote: > On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinek > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinekwrote: > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refreshing subscription relation state inside a transaction block
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[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 alter_subscription_and_transaction.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers