Thread: [HACKERS] Subscription code improvements
Hi, I have done some review of subscription handling (well self-review) and here is the result of that (It's slightly improved version from another thread [1]). I split it into several patches: 0001 - Makes subscription worker exit nicely when there is subscription missing (ie was removed while it was starting) and also makes disabled message look same as the message when disabled state was detected while worker is running. This is mostly just nicer behavior for user (ie no unexpected errors in log when you just disabled the subscription). 0002 - This is bugfix - the sync worker should exit when waiting for apply and apply dies otherwise there is possibility of not being correctly synchronized. 0003 - Splits the schizophrenic SetSubscriptionRelState function into two which don't try to do broken upsert and report proper errors instead. 0004 - Solves the issue which Masahiko Sawada reported [2] about ALTER SUBSCRIPTION handling of workers not being transactional - we now do killing of workers transactionally (and we do the same when possible in DROP SUBSCRIPTION). 0005 - Bugfix to allow syscache access to subscription without being connected to database - this should work since subscription is pinned catalog, it wasn't caught because nothing in core is using it (yet). 0006 - Makes the locking of subscriptions more granular (this depends on 0005). This removes the ugly AccessExclusiveLock on the pg_subscription catalog from DROP SUBSCRIPTION and also solves some potential race conditions between launcher, ALTER SUBSCRIPTION and process_syncing_tables_for_apply(). Also simplifies memory handling in launcher as well as logicalrep_worker_stop() function. This basically makes subscriptions behave like every other object in the database in terms of locking. Only the 0002, 0004 and 0005 are actual bug fixes, but I'd still like to get it all into PG10 as especially the locking now behaves really differently than everything else and that does not seem like a good idea. [1] https://www.postgresql.org/message-id/flat/3e06c16c-f441-c606-985c-6d6239097f54@2ndquadrant.com [2] https://www.postgresql.org/message-id/flat/CAD21AoBD4T2RwTiWD8YfXd+q+m9swX50YjqT5ibj2MzEBVnBhw@mail.gmail.com -- 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
Attachment
- 0001-Improve-messaging-during-logical-replication-worker-.patch
- 0002-Exit-in-sync-worker-if-relation-was-removed-during-s.patch
- 0003-Split-the-SetSubscriptionRelState-function-into-two.patch
- 0004-Only-kill-sync-workers-at-commit-time-in-SUBSCRIPTIO.patch
- 0005-Allow-syscache-access-to-subscriptions-in-database-l.patch
- 0006-Improve-locking-for-subscriptions-and-subscribed-rel.patch
On Sat, Jul 8, 2017 at 5:19 AM, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > Hi, > > I have done some review of subscription handling (well self-review) and > here is the result of that (It's slightly improved version from another > thread [1]). Thank you for working on this and patches! > I split it into several patches: > > 0001 - Makes subscription worker exit nicely when there is subscription > missing (ie was removed while it was starting) and also makes disabled > message look same as the message when disabled state was detected while > worker is running. This is mostly just nicer behavior for user (ie no > unexpected errors in log when you just disabled the subscription). > > 0002 - This is bugfix - the sync worker should exit when waiting for > apply and apply dies otherwise there is possibility of not being > correctly synchronized. > > 0003 - Splits the schizophrenic SetSubscriptionRelState function into > two which don't try to do broken upsert and report proper errors instead. > > 0004 - Solves the issue which Masahiko Sawada reported [2] about ALTER > SUBSCRIPTION handling of workers not being transactional - we now do > killing of workers transactionally (and we do the same when possible in > DROP SUBSCRIPTION). > > 0005 - Bugfix to allow syscache access to subscription without being > connected to database - this should work since subscription is pinned > catalog, it wasn't caught because nothing in core is using it (yet). > > 0006 - Makes the locking of subscriptions more granular (this depends on > 0005). This removes the ugly AccessExclusiveLock on the pg_subscription > catalog from DROP SUBSCRIPTION and also solves some potential race > conditions between launcher, ALTER SUBSCRIPTION and > process_syncing_tables_for_apply(). Also simplifies memory handling in > launcher as well as logicalrep_worker_stop() function. This basically > makes subscriptions behave like every other object in the database in > terms of locking. > > Only the 0002, 0004 and 0005 are actual bug fixes, but I'd still like to > get it all into PG10 as especially the locking now behaves really > differently than everything else and that does not seem like a good idea. > I'm now planing to review 0002, 0004 and 0005 patches first as they are bug fixes. Should we add them to the open item list? Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Wed, Jul 12, 2017 at 11:19 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Sat, Jul 8, 2017 at 5:19 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> Hi,
>>
>> I have done some review of subscription handling (well self-review) and
>> here is the result of that (It's slightly improved version from another
>> thread [1]).
>
> Thank you for working on this and patches!
>
>> I split it into several patches:
>>
>> 0001 - Makes subscription worker exit nicely when there is subscription
>> missing (ie was removed while it was starting) and also makes disabled
>> message look same as the message when disabled state was detected while
>> worker is running. This is mostly just nicer behavior for user (ie no
>> unexpected errors in log when you just disabled the subscription).
>>
>> 0002 - This is bugfix - the sync worker should exit when waiting for
>> apply and apply dies otherwise there is possibility of not being
>> correctly synchronized.
>>
>> 0003 - Splits the schizophrenic SetSubscriptionRelState function into
>> two which don't try to do broken upsert and report proper errors instead.
>>
>> 0004 - Solves the issue which Masahiko Sawada reported [2] about ALTER
>> SUBSCRIPTION handling of workers not being transactional - we now do
>> killing of workers transactionally (and we do the same when possible in
>> DROP SUBSCRIPTION).
>>
>> 0005 - Bugfix to allow syscache access to subscription without being
>> connected to database - this should work since subscription is pinned
>> catalog, it wasn't caught because nothing in core is using it (yet).
>>
>> 0006 - Makes the locking of subscriptions more granular (this depends on
>> 0005). This removes the ugly AccessExclusiveLock on the pg_subscription
>> catalog from DROP SUBSCRIPTION and also solves some potential race
>> conditions between launcher, ALTER SUBSCRIPTION and
>> process_syncing_tables_for_apply(). Also simplifies memory handling in
>> launcher as well as logicalrep_worker_stop() function. This basically
>> makes subscriptions behave like every other object in the database in
>> terms of locking.
>>
>> Only the 0002, 0004 and 0005 are actual bug fixes, but I'd still like to
>> get it all into PG10 as especially the locking now behaves really
>> differently than everything else and that does not seem like a good idea.
>>
>
> I'm now planing to review 0002, 0004 and 0005 patches first as they
> are bug fixes.
I've reviewed 0002, 0004 and 0005 patches briefly. Here are some comments.
--
0002-Exit-in-sync-worker-if-relation-was-removed-during-s.patch
As Petr mentioned, if the table subscription is removed before the
table sync worker gets the subscription relation entry,
GetSubscriptionRelState could returns SUBREL_STATE_UNKNOWN and this
issue happens.
Actually we now can handle this case properly by commit
8dc7c338129d22a52d4afcf2f83a73041119efda. So this seems to be an
improvement and not be a release blocker. Therefore for this patch, I
think it's better to do ereport in the
switch(MyLogicalRepWorker->relstate) block.
BTW, since missing_ok of GetSubscriptionRelState() is always set to
true I think that we can remove it.
Also because the reason I mention at later part this fix might not be necessary.
--
0004-Only-kill-sync-workers-at-commit-time-in-SUBSCRIPTIO.patch
+ /*
+ * If we are dropping slot, stop all the subscription workers
immediately
+ * so that the slot is accessible, otherwise just shedule the
stop at the
+ * end of the transaction.
+ *
+ * New workers won't be started because we hold exclusive lock on the
+ * subscription till the end of transaction.
+ */
+ LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
+ subworkers = logicalrep_sub_workers_find(subid, false);
+ LWLockRelease(LogicalRepWorkerLock);
+ foreach (lc, subworkers)
+ {
+ LogicalRepWorker *w = (LogicalRepWorker *) lfirst(lc);
+ if (sub->slotname)
+ logicalrep_worker_stop(w->subid, w->relid);
+ else
+ logicalrep_worker_stop_at_commit(w->subid, w->relid);
+ }
+ list_free(subworkers);
I think if we kill the all workers including table sync workers then
the fix by 0002 patch is not necessary actually, because the table
sync worker will not see that the subscription relation state has been
removed.
Also, logicalrep_sub_workers_find() function is defined in 0006 patch
but it would be better to move it to 0004 patch.
--
0005-Allow-syscache-access-to-subscriptions-in-database-l.patch
Do we need to update the comment at the top of IndexScanOK()?
To summary, I think we now have only one issue; ALTER SUBSCRIPTION is
not transactional, 0004 patch is addressing this issue . 0002 patch
seems an improvement patch to me, and it might be resolved by 0004
patch. 0005 patch is required by 0006 patch which is an improvement
patch. Am I missing something?
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Fri, Jul 07, 2017 at 10:19:19PM +0200, Petr Jelinek wrote: > I have done some review of subscription handling (well self-review) and > here is the result of that (It's slightly improved version from another > thread [1]). > Only the 0002, 0004 and 0005 are actual bug fixes, but I'd still like to > get it all into PG10 as especially the locking now behaves really > differently than everything else and that does not seem like a good idea. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Peter, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On 8/1/17 00:17, Noah Misch wrote: > The above-described topic is currently a PostgreSQL 10 open item. Peter, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. I'm looking into this now and will report by Friday. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Petr Jelinek wrote: > I split it into several patches: I wish you'd stop splitting error message strings across multiple lines. I've been trapped by a faulty grep not matching a split error message a number of times :-( I know by now to remove words until I get a match, but it seems an unnecessary trap for the unwary. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I wish you'd stop splitting error message strings across multiple lines.
> I've been trapped by a faulty grep not matching a split error message a
> number of times :-( I know by now to remove words until I get a match,
> but it seems an unnecessary trap for the unwary.
Yeah, that's my number one reason for not splitting error messages, too.
It's particularly nasty if similar strings appear in multiple places and
they're not all split alike, as you can get misled into thinking that a
reported error must have occurred in a place you found, rather than
someplace you didn't.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > I wish you'd stop splitting error message strings across multiple lines. > > I've been trapped by a faulty grep not matching a split error message a > > number of times :-( I know by now to remove words until I get a match, > > but it seems an unnecessary trap for the unwary. > > Yeah, that's my number one reason for not splitting error messages, too. > It's particularly nasty if similar strings appear in multiple places and > they're not all split alike, as you can get misled into thinking that a > reported error must have occurred in a place you found, rather than > someplace you didn't. +1. Thanks! Stephen
On 7/13/17 23:53, Masahiko Sawada wrote: > To summary, I think we now have only one issue; ALTER SUBSCRIPTION is > not transactional, 0004 patch is addressing this issue . We have two competing patches for this issue. This patch moves the killing to the end of the DDL transaction. Your earlier patch makes the tablesync work itself responsible for exiting. Do you wish to comment which direction to pursue? (Doing both might also be an option?) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Aug 4, 2017 at 2:17 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 7/13/17 23:53, Masahiko Sawada wrote: >> To summary, I think we now have only one issue; ALTER SUBSCRIPTION is >> not transactional, 0004 patch is addressing this issue . > > We have two competing patches for this issue. This patch moves the > killing to the end of the DDL transaction. Your earlier patch makes the > tablesync work itself responsible for exiting. Do you wish to comment > which direction to pursue? (Doing both might also be an option?) > To make ALTER SUBSCRIPTION REFRESH being transactional, I prefer Petr's proposal. Because it can make ALTER SUBSCRIPTION and DROP SUBSCRIPTION stop the table sync workers that are in progress of copying data. I'm not sure killing table sync workers in DDL commands would be acceptable but since it can free unnecessary slots of logical replication workers and replication slots I'd prefer this idea. However, even with this patch there is still an issue that NOTICE messages "removed subscription for table public.t1" can be appeared even if we rollback ALTER SUBSCRIPTION REFRESH command as I mentioned on earlier thread. Since I think this behaviour will confuse users who check server logs I'd like to deal with it, I don't have a good idea though. Also, I think we can incorporate the idea of my earlier proposal with some changes (i.g. I'd choose the third option). In current implementation, in case where a subscription relation state is accidentally removed while the corresponding table sync worker is progress of copying data, it cannot exit from a loop in wait_for_worker_state_change unless the apply worker dies. So to be more robust, table sync workers can finish with an error if its subscription relation state has disappeared. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On 8/4/17 12:02, Masahiko Sawada wrote: > To make ALTER SUBSCRIPTION REFRESH being transactional, I prefer > Petr's proposal. Because it can make ALTER SUBSCRIPTION and DROP > SUBSCRIPTION stop the table sync workers that are in progress of > copying data. I'm not sure killing table sync workers in DDL commands > would be acceptable but since it can free unnecessary slots of logical > replication workers and replication slots I'd prefer this idea. OK, I have committed the 0004 patch. > However, even with this patch there is still an issue that NOTICE > messages "removed subscription for table public.t1" can be appeared > even if we rollback ALTER SUBSCRIPTION REFRESH command as I mentioned > on earlier thread. Since I think this behaviour will confuse users who > check server logs I'd like to deal with it, I don't have a good idea > though. Maybe we can just remove those messages? We don't get messages when we create a subscription about which tables are part of it. So why do we need such messages when we refresh a subscription? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Aug 02, 2017 at 04:09:43PM -0400, Peter Eisentraut wrote: > On 8/1/17 00:17, Noah Misch wrote: > > The above-described topic is currently a PostgreSQL 10 open item. Peter, > > since you committed the patch believed to have created it, you own this open > > item. If some other commit is more relevant or if this does not belong as a > > v10 open item, please let us know. Otherwise, please observe the policy on > > open item ownership[1] and send a status update within three calendar days of > > this message. Include a date for your subsequent status update. Testers may > > discover new open items at any time, and I want to plan to get them all fixed > > well in advance of shipping v10. Consequently, I will appreciate your efforts > > toward speedy resolution. Thanks. > > I'm looking into this now and will report by Friday. This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
On Sat, Aug 5, 2017 at 10:29 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 8/4/17 12:02, Masahiko Sawada wrote: >> To make ALTER SUBSCRIPTION REFRESH being transactional, I prefer >> Petr's proposal. Because it can make ALTER SUBSCRIPTION and DROP >> SUBSCRIPTION stop the table sync workers that are in progress of >> copying data. I'm not sure killing table sync workers in DDL commands >> would be acceptable but since it can free unnecessary slots of logical >> replication workers and replication slots I'd prefer this idea. > > OK, I have committed the 0004 patch. Thank you! > >> However, even with this patch there is still an issue that NOTICE >> messages "removed subscription for table public.t1" can be appeared >> even if we rollback ALTER SUBSCRIPTION REFRESH command as I mentioned >> on earlier thread. Since I think this behaviour will confuse users who >> check server logs I'd like to deal with it, I don't have a good idea >> though. > > Maybe we can just remove those messages? > > We don't get messages when we create a subscription about which tables > are part of it. So why do we need such messages when we refresh a > subscription? I think that the messages is useful when we add/remove tables to/from the publication and then refresh the subscription, so we might want to change it to DEBUG rather than elimination. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Sun, Aug 6, 2017 at 7:44 AM, Noah Misch <noah@leadboat.com> wrote: > On Wed, Aug 02, 2017 at 04:09:43PM -0400, Peter Eisentraut wrote: >> On 8/1/17 00:17, Noah Misch wrote: >> > The above-described topic is currently a PostgreSQL 10 open item. Peter, >> > since you committed the patch believed to have created it, you own this open >> > item. If some other commit is more relevant or if this does not belong as a >> > v10 open item, please let us know. Otherwise, please observe the policy on >> > open item ownership[1] and send a status update within three calendar days of >> > this message. Include a date for your subsequent status update. Testers may >> > discover new open items at any time, and I want to plan to get them all fixed >> > well in advance of shipping v10. Consequently, I will appreciate your efforts >> > toward speedy resolution. Thanks. >> >> I'm looking into this now and will report by Friday. > > This PostgreSQL 10 open item is past due for your status update. Kindly send > a status update within 24 hours, and include a date for your subsequent status > update. Refer to the policy on open item ownership: > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com I think this open item has closed by the commit 7e174fa793a2df89fe03d002a5087ef67abcdde8 ? Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On 8/7/17 00:32, Masahiko Sawada wrote: > On Sun, Aug 6, 2017 at 7:44 AM, Noah Misch <noah@leadboat.com> wrote: >> On Wed, Aug 02, 2017 at 04:09:43PM -0400, Peter Eisentraut wrote: >>> On 8/1/17 00:17, Noah Misch wrote: >>>> The above-described topic is currently a PostgreSQL 10 open item. Peter, >>>> since you committed the patch believed to have created it, you own this open >>>> item. If some other commit is more relevant or if this does not belong as a >>>> v10 open item, please let us know. Otherwise, please observe the policy on >>>> open item ownership[1] and send a status update within three calendar days of >>>> this message. Include a date for your subsequent status update. Testers may >>>> discover new open items at any time, and I want to plan to get them all fixed >>>> well in advance of shipping v10. Consequently, I will appreciate your efforts >>>> toward speedy resolution. Thanks. >>> >>> I'm looking into this now and will report by Friday. >> >> This PostgreSQL 10 open item is past due for your status update. Kindly send >> a status update within 24 hours, and include a date for your subsequent status >> update. Refer to the policy on open item ownership: >> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com > > I think this open item has closed by the commit > 7e174fa793a2df89fe03d002a5087ef67abcdde8 ? Yes. I have updated the wiki page now. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 8/7/17 00:27, Masahiko Sawada wrote: >>> However, even with this patch there is still an issue that NOTICE >>> messages "removed subscription for table public.t1" can be appeared >>> even if we rollback ALTER SUBSCRIPTION REFRESH command as I mentioned >>> on earlier thread. Since I think this behaviour will confuse users who >>> check server logs I'd like to deal with it, I don't have a good idea >>> though. >> >> Maybe we can just remove those messages? >> >> We don't get messages when we create a subscription about which tables >> are part of it. So why do we need such messages when we refresh a >> subscription? > > I think that the messages is useful when we add/remove tables to/from > the publication and then refresh the subscription, so we might want to > change it to DEBUG rather than elimination. Good idea. Done that way. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Aug 7, 2017 at 10:22 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 8/7/17 00:27, Masahiko Sawada wrote: >>>> However, even with this patch there is still an issue that NOTICE >>>> messages "removed subscription for table public.t1" can be appeared >>>> even if we rollback ALTER SUBSCRIPTION REFRESH command as I mentioned >>>> on earlier thread. Since I think this behaviour will confuse users who >>>> check server logs I'd like to deal with it, I don't have a good idea >>>> though. >>> >>> Maybe we can just remove those messages? >>> >>> We don't get messages when we create a subscription about which tables >>> are part of it. So why do we need such messages when we refresh a >>> subscription? >> >> I think that the messages is useful when we add/remove tables to/from >> the publication and then refresh the subscription, so we might want to >> change it to DEBUG rather than elimination. > > Good idea. Done that way. > Thank you! Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Petr, On Thu, Aug 3, 2017 at 5:24 AM, Stephen Frost <sfrost@snowman.net> wrote: > * Tom Lane (tgl@sss.pgh.pa.us) wrote: >> Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> > I wish you'd stop splitting error message strings across multiple lines. >> > I've been trapped by a faulty grep not matching a split error message a >> > number of times :-( I know by now to remove words until I get a match, >> > but it seems an unnecessary trap for the unwary. >> >> Yeah, that's my number one reason for not splitting error messages, too. >> It's particularly nasty if similar strings appear in multiple places and >> they're not all split alike, as you can get misled into thinking that a >> reported error must have occurred in a place you found, rather than >> someplace you didn't. > > +1. > Are you planning to work on remaining patches 0005 and 0006 that improve the subscription codes in PG11 cycle? If not, I will take over them and work on the next CF. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On 8/8/17 05:58, Masahiko Sawada wrote: > Are you planning to work on remaining patches 0005 and 0006 that > improve the subscription codes in PG11 cycle? If not, I will take over > them and work on the next CF. Based on your assessment, the remaining patches were not required bug fixes. So I think preparing them for the next commit fest would be great. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 8/8/17 05:58, Masahiko Sawada wrote: >> Are you planning to work on remaining patches 0005 and 0006 that >> improve the subscription codes in PG11 cycle? If not, I will take over >> them and work on the next CF. > > Based on your assessment, the remaining patches were not required bug > fixes. So I think preparing them for the next commit fest would be great. > Thank you for the comment. After more thought, since 0001 and 0003 patches on the first mail also improve the subscription codes and are worth to be considered, I picked total 4 patches up and updated them. I'm planning to work on these patches in the next CF. 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
Attachment
On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> On 8/8/17 05:58, Masahiko Sawada wrote: >>> Are you planning to work on remaining patches 0005 and 0006 that >>> improve the subscription codes in PG11 cycle? If not, I will take over >>> them and work on the next CF. >> >> Based on your assessment, the remaining patches were not required bug >> fixes. So I think preparing them for the next commit fest would be great. >> > > Thank you for the comment. > > After more thought, since 0001 and 0003 patches on the first mail also > improve the subscription codes and are worth to be considered, I > picked total 4 patches up and updated them. I'm planning to work on > these patches in the next CF. > Added this item to the next commit fest. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Fri, Aug 18, 2017 at 2:09 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: >> On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut >> <peter.eisentraut@2ndquadrant.com> wrote: >>> On 8/8/17 05:58, Masahiko Sawada wrote: >>>> Are you planning to work on remaining patches 0005 and 0006 that >>>> improve the subscription codes in PG11 cycle? If not, I will take over >>>> them and work on the next CF. >>> >>> Based on your assessment, the remaining patches were not required bug >>> fixes. So I think preparing them for the next commit fest would be great. >>> >> >> Thank you for the comment. >> >> After more thought, since 0001 and 0003 patches on the first mail also >> improve the subscription codes and are worth to be considered, I >> picked total 4 patches up and updated them. I'm planning to work on >> these patches in the next CF. >> > > Added this item to the next commit fest. The patch set fails to apply. Please provide a rebased version. I am moving this entry to next CF with waiting on author as status. -- Michael
Greetings, * Michael Paquier (michael.paquier@gmail.com) wrote: > On Fri, Aug 18, 2017 at 2:09 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > >> On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut > >> <peter.eisentraut@2ndquadrant.com> wrote: > >>> On 8/8/17 05:58, Masahiko Sawada wrote: > >>>> Are you planning to work on remaining patches 0005 and 0006 that > >>>> improve the subscription codes in PG11 cycle? If not, I will take over > >>>> them and work on the next CF. > >>> > >>> Based on your assessment, the remaining patches were not required bug > >>> fixes. So I think preparing them for the next commit fest would be great. > >>> > >> > >> Thank you for the comment. > >> > >> After more thought, since 0001 and 0003 patches on the first mail also > >> improve the subscription codes and are worth to be considered, I > >> picked total 4 patches up and updated them. I'm planning to work on > >> these patches in the next CF. > >> > > > > Added this item to the next commit fest. > > The patch set fails to apply. Please provide a rebased version. I am > moving this entry to next CF with waiting on author as status. Masahiko Sawada, this patch is still in Waiting on Author and hasn't progressed in a very long time. Is there any chance you'll be able to provide an updated patch soon for review? Or should this patch be closed out? Thanks! Stephen
Attachment
On Tue, Jan 23, 2018 at 7:58 AM, Stephen Frost <sfrost@snowman.net> wrote: > Greetings, > > * Michael Paquier (michael.paquier@gmail.com) wrote: >> On Fri, Aug 18, 2017 at 2:09 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: >> > On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: >> >> On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut >> >> <peter.eisentraut@2ndquadrant.com> wrote: >> >>> On 8/8/17 05:58, Masahiko Sawada wrote: >> >>>> Are you planning to work on remaining patches 0005 and 0006 that >> >>>> improve the subscription codes in PG11 cycle? If not, I will take over >> >>>> them and work on the next CF. >> >>> >> >>> Based on your assessment, the remaining patches were not required bug >> >>> fixes. So I think preparing them for the next commit fest would be great. >> >>> >> >> >> >> Thank you for the comment. >> >> >> >> After more thought, since 0001 and 0003 patches on the first mail also >> >> improve the subscription codes and are worth to be considered, I >> >> picked total 4 patches up and updated them. I'm planning to work on >> >> these patches in the next CF. >> >> >> > >> > Added this item to the next commit fest. >> >> The patch set fails to apply. Please provide a rebased version. I am >> moving this entry to next CF with waiting on author as status. > > Masahiko Sawada, this patch is still in Waiting on Author and hasn't > progressed in a very long time. Is there any chance you'll be able to > provide an updated patch soon for review? Or should this patch be > closed out? > Thank you for notification. Since it seems to me that no one is interested in this patch, it would be better to close out this patch. This patch is a refactoring patch but subscription code seems to work fine so far. If a problem appears around subscriptions, I might propose it again. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On 1/24/18 02:33, Masahiko Sawada wrote: > Thank you for notification. Since it seems to me that no one is > interested in this patch, it would be better to close out this patch. > This patch is a refactoring patch but subscription code seems to work > fine so far. If a problem appears around subscriptions, I might > propose it again. I have looked at the patches again. They seem generally reasonable, but I don't see quite why we want or need them. More details would help review them. Do they fix any bugs, or are they just rearranging code? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 26, 2018 at 11:41 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 1/24/18 02:33, Masahiko Sawada wrote: >> Thank you for notification. Since it seems to me that no one is >> interested in this patch, it would be better to close out this patch. >> This patch is a refactoring patch but subscription code seems to work >> fine so far. If a problem appears around subscriptions, I might >> propose it again. > > I have looked at the patches again. Thank you for looking at this. > They seem generally reasonable, but > I don't see quite why we want or need them. More details would help > review them. Do they fix any bugs, or are they just rearranging code? > 0002 patch rearranges the code. Currently SetSubscriptionRelState() not only update but also register a record, and it is controlled by update_only argument. This patch splits SetSubscriptionRelState() into AddSubscriptionRelState() and UpdateSubscriptionRelstate(). 0001, 0003 and 0004 patch are improvement patches. 0001 patch improves messaging during worker startup. 0004 patch, which requires 0003 patch, patch reduce the lock level for DROP SUBSCRIPTION to RowExclusiveLock. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Hi Masahiko, On 1/30/18 5:00 AM, Masahiko Sawada wrote: > On Fri, Jan 26, 2018 at 11:41 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> On 1/24/18 02:33, Masahiko Sawada wrote: >>> Thank you for notification. Since it seems to me that no one is >>> interested in this patch, it would be better to close out this patch. >>> This patch is a refactoring patch but subscription code seems to work >>> fine so far. If a problem appears around subscriptions, I might >>> propose it again. This patch is still in the Waiting for Author state but it looks like you intended to close it. Should I do so now? Thanks, -- -David david@pgmasters.net
On Tue, Mar 6, 2018 at 11:17 PM, David Steele <david@pgmasters.net> wrote: > Hi Masahiko, > > On 1/30/18 5:00 AM, Masahiko Sawada wrote: >> On Fri, Jan 26, 2018 at 11:41 AM, Peter Eisentraut >> <peter.eisentraut@2ndquadrant.com> wrote: >>> On 1/24/18 02:33, Masahiko Sawada wrote: >>>> Thank you for notification. Since it seems to me that no one is >>>> interested in this patch, it would be better to close out this patch. >>>> This patch is a refactoring patch but subscription code seems to work >>>> fine so far. If a problem appears around subscriptions, I might >>>> propose it again. > > This patch is still in the Waiting for Author state but it looks like > you intended to close it. Should I do so now? > Uh, actually three(0001 - 0002) of four patches applies cleanly to current HEAD, and I think we can regards them as "Needs Review". Only 0003 and 0004 patch are under "Waiting on Author". Since 0001 and 0002 are very simple I hope these patch get reviewed. It was a my fault to get different patches into one CF entry. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
0001:
there are a bunch of other messages of the same ilk in the file. I
don't like how the current messages are worded; maybe Peter or Petr had
some reason why they're like that, but I would have split out the reason
for not starting or stopping into errdetail. Something like
errmsg("logical replication apply worker for subscription \"%s\" will not start", ...)
errdetail("Subscription has been disabled during startup.")
But I think we should change all these messages in unison rather than
only one of them.
Now, your patch changes "will not start" to "will stop". But is that
correct? ISTM that this happens when a worker is starting and
determines that it is not needed. So "will not start" is more correct.
"Will stop" would be correct if the worker had been running for some
time and suddenly decided to terminate, but that doesn't seem to be the
case, unless I misread the code.
Your patch also changes "disabled during startup" to just "disabled".
Is that a useful change? ISTM that if the subscription had been
disabled prior to startup, then the worker would not have started at
all, so we would not be seeing this message in the first place. Again,
maybe I am misreading the code? Please explain.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
0002 looks like a good improvement to me. The existing routine is messy, and apparently it's so just to save one LockSharedObject plus cache lookup; IMO it's not worth it. Patched code looks simpler. If there are cases where having the combined behavior is useful, it's not clear what they are. (If I understand correctly, the reason is that a sync worker could try to insert-or-update the row after some other process deleted it [because of removing the table from subscription?] ... but that seems to work out *simpler* with the new code. So what's up?) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/7/18 7:41 AM, Masahiko Sawada wrote: > On Tue, Mar 6, 2018 at 11:17 PM, David Steele <david@pgmasters.net> wrote: >> Hi Masahiko, >> >> On 1/30/18 5:00 AM, Masahiko Sawada wrote: >>> On Fri, Jan 26, 2018 at 11:41 AM, Peter Eisentraut >>> <peter.eisentraut@2ndquadrant.com> wrote: >>>> On 1/24/18 02:33, Masahiko Sawada wrote: >>>>> Thank you for notification. Since it seems to me that no one is >>>>> interested in this patch, it would be better to close out this patch. >>>>> This patch is a refactoring patch but subscription code seems to work >>>>> fine so far. If a problem appears around subscriptions, I might >>>>> propose it again. >> >> This patch is still in the Waiting for Author state but it looks like >> you intended to close it. Should I do so now? > > Uh, actually three(0001 - 0002) of four patches applies cleanly to > current HEAD, and I think we can regards them as "Needs Review". Only > 0003 and 0004 patch are under "Waiting on Author". Since 0001 and 0002 > are very simple I hope these patch get reviewed. It was a my fault to > get different patches into one CF entry. I rather doubt you are going to attract any review as long is this stays in Waiting on Author state -- which I notice it has been in since the end of the November CF. That is reason enough to return it since we have been taking a pretty firm stance on patches that have not been updated for the CF. I'm marking this submission Returned with Feedback. Regards, -- -David david@pgmasters.net
David Steele wrote: > I'm marking this submission Returned with Feedback. Not yet please. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/7/18 9:37 AM, Alvaro Herrera wrote: > David Steele wrote: > >> I'm marking this submission Returned with Feedback. > > Not yet please. Back to Waiting on Author state. Regards, -- -David david@pgmasters.net
On Wed, Mar 7, 2018 at 11:13 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 0001:
>
> there are a bunch of other messages of the same ilk in the file. I
> don't like how the current messages are worded; maybe Peter or Petr had
> some reason why they're like that, but I would have split out the reason
> for not starting or stopping into errdetail. Something like
>
> errmsg("logical replication apply worker for subscription \"%s\" will not start", ...)
> errdetail("Subscription has been disabled during startup.")
>
> But I think we should change all these messages in unison rather than
> only one of them.
>
> Now, your patch changes "will not start" to "will stop". But is that
> correct? ISTM that this happens when a worker is starting and
> determines that it is not needed. So "will not start" is more correct.
> "Will stop" would be correct if the worker had been running for some
> time and suddenly decided to terminate, but that doesn't seem to be the
> case, unless I misread the code.
>
> Your patch also changes "disabled during startup" to just "disabled".
> Is that a useful change? ISTM that if the subscription had been
> disabled prior to startup, then the worker would not have started at
> all, so we would not be seeing this message in the first place. Again,
> maybe I am misreading the code? Please explain.
I think you're not misreading the code. I agree with you. "will not
start" and "disabled during startup" would be more appropriate here.
But Petr might have other opinion on this. Also I noticed I overlooked
one change of v1 patch proposed by Petr. Attached a new patch
incorporated your review comment and the hunk I overlooked.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachment
On Wed, Mar 7, 2018 at 11:13 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 0002 looks like a good improvement to me. The existing routine is
> messy, and apparently it's so just to save one LockSharedObject plus
> cache lookup; IMO it's not worth it. Patched code looks simpler. If
> there are cases where having the combined behavior is useful, it's not
> clear what they are. (If I understand correctly, the reason is that a
> sync worker could try to insert-or-update the row after some other
> process deleted it [because of removing the table from subscription?]
> ... but that seems to work out *simpler* with the new code. So what's
> up?)
>
The function calling SetSubscriptionRelState with update_only=false (i.g. going to do insert-or-update) is two function: CreateSubscription() and AlterSubscription_refresh(). AFAICS these two function actually doesn't need such insert-or-update functionality because it doesn't happen that a backend process creates/alters the same name subscription which already exists. Since CreateSubscirption() inserts a heap into the system catalog one transaction ends up with the error of key already exists if two process tries to create the same name subscription . Similarly for AlterSubscription_refresh(), since we acquire the AccessExclusiveLock on the subscription object before getting the new table list in the publication the updating a existing entry doesn't happen. So this patch changes SetsubscriptionRelState() with update_only=fasle to AddSubscriptionRelState() and others to UpdateSubscriptionRelState().
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
I have committed the patches 0001 Improve messaging during logical replication worker startup 0002 Split the SetSubscriptionRelState function into two 0004 doesn't apply anymore, 0003 doesn't seem to be very useful without 0004, as I understand it. It's also a bit more than I'm comfortable reviewing or committing right now. So maybe move those to the next commitfest or close the entry as returned with feedback? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services