Subscription code improvements

Started by Petr Jelinekalmost 9 years ago36 messageshackers
Jump to latest
#1Petr Jelinek
petr@2ndquadrant.com

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]/messages/by-id/3e06c16c-f441-c606-985c-6d6239097f54@2ndquadrant.com).

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]/messages/by-id/CAD21AoBD4T2RwTiWD8YfXd+q+m9swX50YjqT5ibj2MzEBVnBhw@mail.gmail.com 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]: /messages/by-id/3e06c16c-f441-c606-985c-6d6239097f54@2ndquadrant.com
/messages/by-id/3e06c16c-f441-c606-985c-6d6239097f54@2ndquadrant.com
[2]: /messages/by-id/CAD21AoBD4T2RwTiWD8YfXd+q+m9swX50YjqT5ibj2MzEBVnBhw@mail.gmail.com
/messages/by-id/CAD21AoBD4T2RwTiWD8YfXd+q+m9swX50YjqT5ibj2MzEBVnBhw@mail.gmail.com

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

Attachments:

0001-Improve-messaging-during-logical-replication-worker-.patchtext/x-patch; name=0001-Improve-messaging-during-logical-replication-worker-.patchDownload+15-9
0002-Exit-in-sync-worker-if-relation-was-removed-during-s.patchtext/x-patch; name=0002-Exit-in-sync-worker-if-relation-was-removed-during-s.patchDownload+14-1
0003-Split-the-SetSubscriptionRelState-function-into-two.patchtext/x-patch; name=0003-Split-the-SetSubscriptionRelState-function-into-two.patchDownload+99-85
0004-Only-kill-sync-workers-at-commit-time-in-SUBSCRIPTIO.patchtext/x-patch; name=0004-Only-kill-sync-workers-at-commit-time-in-SUBSCRIPTIO.patchDownload+91-7
0005-Allow-syscache-access-to-subscriptions-in-database-l.patchtext/x-patch; name=0005-Allow-syscache-access-to-subscriptions-in-database-l.patchDownload+4-3
0006-Improve-locking-for-subscriptions-and-subscribed-rel.patchtext/x-patch; name=0006-Improve-locking-for-subscriptions-and-subscribed-rel.patchDownload+172-209
#2Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Petr Jelinek (#1)
Re: Subscription code improvements

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

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

#3Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Masahiko Sawada (#2)
Re: Subscription code improvements

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

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

#4Noah Misch
noah@leadboat.com
In reply to: Petr Jelinek (#1)
Re: Subscription code improvements

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]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com 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]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

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

#5Peter Eisentraut
peter_e@gmx.net
In reply to: Noah Misch (#4)
Re: Subscription code improvements

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

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

#6Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Petr Jelinek (#1)
Re: Subscription code improvements

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

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

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#6)
Re: Subscription code improvements

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

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

#8Stephen Frost
sfrost@snowman.net
In reply to: Tom Lane (#7)
Re: Subscription code improvements

* 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

#9Peter Eisentraut
peter_e@gmx.net
In reply to: Masahiko Sawada (#3)
Re: Subscription code improvements

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

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

#10Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Peter Eisentraut (#9)
Re: Subscription code improvements

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

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

#11Peter Eisentraut
peter_e@gmx.net
In reply to: Masahiko Sawada (#10)
Re: Subscription code improvements

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

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

#12Noah Misch
noah@leadboat.com
In reply to: Peter Eisentraut (#5)
Re: Subscription code improvements

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:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

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

#13Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Peter Eisentraut (#11)
Re: Subscription code improvements

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

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

#14Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Noah Misch (#12)
Re: Subscription code improvements

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:
/messages/by-id/20170404140717.GA2675809@tornado.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

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

#15Peter Eisentraut
peter_e@gmx.net
In reply to: Masahiko Sawada (#14)
Re: Subscription code improvements

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:
/messages/by-id/20170404140717.GA2675809@tornado.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

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

#16Peter Eisentraut
peter_e@gmx.net
In reply to: Masahiko Sawada (#13)
Re: Subscription code improvements

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

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

#17Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Peter Eisentraut (#16)
Re: Subscription code improvements

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

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

#18Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Stephen Frost (#8)
Re: Subscription code improvements

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

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

#19Peter Eisentraut
peter_e@gmx.net
In reply to: Masahiko Sawada (#18)
Re: Subscription code improvements

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

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

#20Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Peter Eisentraut (#19)
Re: Subscription code improvements

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

Attachments:

0001-Improve-messaging-during-logical-replication-worker-_v2.patchapplication/octet-stream; name=0001-Improve-messaging-during-logical-replication-worker-_v2.patchDownload+5-7
0002-Split-the-SetSubscriptionRelState-function-into-two_v2.patchapplication/octet-stream; name=0002-Split-the-SetSubscriptionRelState-function-into-two_v2.patchDownload+98-84
0003-Allow-syscache-access-to-subscriptions-in-database-l_v2.patchapplication/octet-stream; name=0003-Allow-syscache-access-to-subscriptions-in-database-l_v2.patchDownload+4-3
0004-Improve-locking-for-subscriptions-and-subscribed-rel_v2.patchapplication/octet-stream; name=0004-Improve-locking-for-subscriptions-and-subscribed-rel_v2.patchDownload+145-209
#21Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Masahiko Sawada (#20)
#22Michael Paquier
michael@paquier.xyz
In reply to: Masahiko Sawada (#21)
#23Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#22)
#24Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Stephen Frost (#23)
#25Peter Eisentraut
peter_e@gmx.net
In reply to: Masahiko Sawada (#24)
#26Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Peter Eisentraut (#25)
#27David Steele
david@pgmasters.net
In reply to: Masahiko Sawada (#26)
#28Masahiko Sawada
sawada.mshk@gmail.com
In reply to: David Steele (#27)
#29Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Masahiko Sawada (#20)
#30Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Masahiko Sawada (#20)
#31David Steele
david@pgmasters.net
In reply to: Masahiko Sawada (#28)
#32Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: David Steele (#31)
#33David Steele
david@pgmasters.net
In reply to: Alvaro Herrera (#32)
#34Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Alvaro Herrera (#29)
#35Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Alvaro Herrera (#30)
#36Peter Eisentraut
peter_e@gmx.net
In reply to: Masahiko Sawada (#35)