Re: [HACKERS] Subscription code improvements

[Masahiko Sawada]

On Thu, Aug 17, 2017 at 8:17 PM, Masahiko Sawada  wrote:
> On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut
>  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


-- 
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] Subscription code improvements

[Masahiko Sawada]

On Thu, Aug 17, 2017 at 9:10 AM, Peter Eisentraut
 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


0001-Improve-messaging-during-logical-replication-worker-_v2.patch
Description: Binary data


0002-Split-the-SetSubscriptionRelState-function-into-two_v2.patch
Description: Binary data


0003-Allow-syscache-access-to-subscriptions-in-database-l_v2.patch
Description: Binary data


0004-Improve-locking-for-subscriptions-and-subscribed-rel_v2.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] Subscription code improvements

[Peter Eisentraut]

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

Re: [HACKERS] Subscription code improvements

[Masahiko Sawada]

Petr,

On Thu, Aug 3, 2017 at 5:24 AM, Stephen Frost  wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Alvaro Herrera  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

Re: [HACKERS] Subscription code improvements

[Masahiko Sawada]

On Mon, Aug 7, 2017 at 10:22 PM, Peter Eisentraut
 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

Re: [HACKERS] Subscription code improvements

[Peter Eisentraut]

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

Re: [HACKERS] Subscription code improvements

[Peter Eisentraut]

On 8/7/17 00:32, Masahiko Sawada wrote:
> On Sun, Aug 6, 2017 at 7:44 AM, Noah Misch  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


-- 
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] Subscription code improvements

[Masahiko Sawada]

On Sun, Aug 6, 2017 at 7:44 AM, Noah Misch  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


-- 
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] Subscription code improvements

[Masahiko Sawada]

On Sat, Aug 5, 2017 at 10:29 AM, Peter Eisentraut
 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

Re: [HACKERS] Subscription code improvements

[Noah Misch]

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


-- 
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] Subscription code improvements

[Peter Eisentraut]

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

Re: [HACKERS] Subscription code improvements

[Masahiko Sawada]

On Fri, Aug 4, 2017 at 2:17 AM, Peter Eisentraut
 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

Re: [HACKERS] Subscription code improvements

[Peter Eisentraut]

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

Re: [HACKERS] Subscription code improvements

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Alvaro Herrera  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


signature.asc
Description: Digital signature

Re: [HACKERS] Subscription code improvements

[Tom Lane]

Alvaro Herrera  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

Re: [HACKERS] Subscription code improvements

[Alvaro Herrera]

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 Herrerahttps://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] Subscription code improvements

[Peter Eisentraut]

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

Re: [HACKERS] Subscription code improvements

[Noah Misch]

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


-- 
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] Subscription code improvements

[Masahiko Sawada]

On Wed, Jul 12, 2017 at 11:19 AM, Masahiko Sawada  wrote:
> On Sat, Jul 8, 2017 at 5:19 AM, Petr Jelinek
>  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

Re: [HACKERS] Subscription code improvements

[Masahiko Sawada]

On Sat, Jul 8, 2017 at 5:19 AM, Petr Jelinek
 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

[HACKERS] Subscription code improvements

[Petr Jelinek]

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-6d6239097...@2ndquadrant.com
[2]
https://www.postgresql.org/message-id/flat/cad21aobd4t2rwtiwd8yfxd+q+m9swx50yjqt5ibj2mzebvn...@mail.gmail.com

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 4c1ef64493ee930dfde3aa787c454a5d68ac3efd Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Thu, 6 Jul 2017 23:42:56 +0200
Subject: [PATCH 1/6] Improve messaging during logical replication worker
 startup

---
 src/backend/replication/logical/worker.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 0d48dfa..2e4099c 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -1527,24 +1527,31 @@ ApplyWorkerMain(Datum main_arg)
 		 ALLOCSET_DEFAULT_SIZES);
 	StartTransactionCommand();
 	oldctx = MemoryContextSwitchTo(ApplyContext);
-	MySubscription = GetSubscription(MyLogicalRepWorker->subid, false);
+	MySubscription = GetSubscription(MyLogicalRepWorker->subid, true);
+	if (!MySubscription)
+	{
+		ereport(LOG,
+(errmsg("logical replication apply worker for subscription %u will "
+		"stop because the subscription was removed",
+		MyLogicalRepWorker->subid)));
+		proc_exit(0);
+	}
 	MySubscriptionValid = true;
 	MemoryContextSwitchTo(oldctx);
 
-	/* Setup synchronous commit according to the user's wishes */
-	SetConfigOption("synchronous_commit", MySubscription->synccommit,
-	PGC_BACKEND, PGC_S_OVERRIDE);
-
 	if (!MySubscription->enabled)
 	{
 		ereport(LOG,
-(errmsg("logical replication apply worker for subscription \"%s\" will not "
-		"start because the subscription was disabled during startup",
+(errmsg("logical replication apply worker for subscription \"%s\" will "
+		"stop because the subscription was disabled",
 		MySubscription->name)));
-
 		proc_exit(0);
 	}
 
+	/* Setup synchronous commit according to the user's wishes */
+	SetConfigOption("synchronous_commit", MySubscription->synccommit,
+	PGC_BACKEND, PGC_S_OVERRIDE);
+
 	/* Keep us informed about subscription changes. */
 	CacheRegisterSyscacheCallback(SUBSCRIPTIONOID,
   subscription_change_cb,
-- 
2.7.4

From b5d4d9a130658859bcf6e21ca3bed131dbdddb57 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Fri, 7 Jul 2017 00:04:43 +0200
Subject: [PATCH 2/6] Exit in sync worker if relation was removed during
 startup

---
 src/backend/replication/logical/tablesync.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 32abf5b..9fbdd8c 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -824,6 +824,20 @@