Thread: [HACKERS] DROP SUBSCRIPTION hangs if sub is disabled in the same transaction

Hello,

Currently, DROP SUBSCRIPTION on REL_10_STABLE would block forever in the
following setup:

node 1:
create table t (i int);
create publication p for table t;

node 2:
create table t (i int);
create subscription s CONNECTION 'port=5432' publication p;
begin;
alter subscription s disable ;
alter subscription s set (slot_name = none);
drop subscription s;
end;

It hangs in replorigin_drop because we wait until ReplicationState is
released. This should happen on exit of worker, but worker will not exit
until transaction commit because he doesn't see that the sub was
disabled.

Attached patch fixes this by stopping workers before RO drop, as
already done in case when we drop replication slot. I am not sure
whether this is a proper solution, but for now I don't see any problems
with early stop of workers: even if transaction later aborts, launcher
will happily restart them, isn't it?


--
Arseny Sher
Postgres Professional: https://postgrespro.ru
The Russian Postgres Company

Arseny Sher <a.sher@postgrespro.ru> writes:

> Attached patch fixes this by stopping workers before RO drop, as
> already done in case when we drop replication slot.

Sorry, here is the patch.
From 008d54dfe2e8ba610bb7c897cfbb4ee7a700aa2e Mon Sep 17 00:00:00 2001
From: Arseny Sher <sher-ars@yandex.ru>
Date: Mon, 4 Sep 2017 16:55:08 +0300
Subject: [PATCH] Stop LR workers before dropping replication origin.

Previously, executing ALTER SUBSCRIPTION DISABLE and DROP SUBSCRIPTION in one
transaction would hang forever, because worker didn't stop until transaction is
commited, and transaction couldn't commit before worker exit since the latter
occupies ReplicationState.
---
 src/backend/commands/subscriptioncmds.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 77620dfd42..5d608855a1 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -886,6 +886,10 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
     else
         slotname = NULL;
 
+    /* Get originid */
+    snprintf(originname, sizeof(originname), "pg_%u", subid);
+    originid = replorigin_by_name(originname, true);
+
     /*
      * Since dropping a replication slot is not transactional, the replication
      * slot stays dropped even if the transaction rolls back.  So we cannot
@@ -909,9 +913,10 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
     ReleaseSysCache(tup);
 
     /*
-     * If we are dropping the replication slot, stop all the subscription
-     * workers immediately, so that the slot becomes accessible.  Otherwise
-     * just schedule the stopping for the end of the transaction.
+     * We stop all subscription workers immediately in two cases:
+     * - We are dropping the replication slot, to make the slot accessible;
+     * - We are dropping repl origin tracking, to release ReplicationState;
+     * Otherwise just schedule the stopping for the end of the transaction.
      *
      * New workers won't be started because we hold an exclusive lock on the
      * subscription till the end of the transaction.
@@ -923,7 +928,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
     {
         LogicalRepWorker *w = (LogicalRepWorker *) lfirst(lc);
 
-        if (slotname)
+        if (slotname || originid != InvalidRepOriginId)
             logicalrep_worker_stop(w->subid, w->relid);
         else
             logicalrep_worker_stop_at_commit(w->subid, w->relid);
@@ -937,8 +942,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
     RemoveSubscriptionRel(subid, InvalidOid);
 
     /* Remove the origin tracking if exists. */
-    snprintf(originname, sizeof(originname), "pg_%u", subid);
-    originid = replorigin_by_name(originname, true);
     if (originid != InvalidRepOriginId)
         replorigin_drop(originid, false);
 
-- 
2.11.0



--
Arseny Sher
Postgres Professional: https://postgrespro.ru
The Russian Postgres Company

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

On Mon, Sep 4, 2017 at 11:43 PM, Arseny Sher <a.sher@postgrespro.ru> wrote:
> Arseny Sher <a.sher@postgrespro.ru> writes:
>
>> Attached patch fixes this by stopping workers before RO drop, as
>> already done in case when we drop replication slot.
>
> Sorry, here is the patch.
>

I could reproduce this issue, it's a bug. Added this to the open item.
The cause of this is commit 7e174fa7 which make subscription ddls kill
the apply worker only when transaction commit. I didn't look the patch
deeply yet but I'm not sure the approach that kills apply worker
before commit would be good.

Regards,

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

On Wed, Sep 06, 2017 at 03:28:47PM +0900, Masahiko Sawada wrote:
> On Mon, Sep 4, 2017 at 11:43 PM, Arseny Sher <a.sher@postgrespro.ru> wrote:
> > Arseny Sher <a.sher@postgrespro.ru> writes:
> >
> >> Attached patch fixes this by stopping workers before RO drop, as
> >> already done in case when we drop replication slot.
> >
> > Sorry, here is the patch.
> >
> 
> I could reproduce this issue, it's a bug. Added this to the open item.
> The cause of this is commit 7e174fa7 which make subscription ddls kill
> the apply worker only when transaction commit. I didn't look the patch
> deeply yet but I'm not sure the approach that kills apply worker
> before commit would be good.

[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 Thu, Sep 07, 2017 at 04:53:12AM +0000, Noah Misch wrote:
> On Wed, Sep 06, 2017 at 03:28:47PM +0900, Masahiko Sawada wrote:
> > On Mon, Sep 4, 2017 at 11:43 PM, Arseny Sher <a.sher@postgrespro.ru> wrote:
> > > Arseny Sher <a.sher@postgrespro.ru> writes:
> > >
> > >> Attached patch fixes this by stopping workers before RO drop, as
> > >> already done in case when we drop replication slot.
> > >
> > > Sorry, here is the patch.
> > >
> > 
> > I could reproduce this issue, it's a bug. Added this to the open item.
> > The cause of this is commit 7e174fa7 which make subscription ddls kill
> > the apply worker only when transaction commit. I didn't look the patch
> > deeply yet but I'm not sure the approach that kills apply worker
> > before commit would be good.
> 
> [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

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

On 9/10/17 12:14, Noah Misch wrote:
> 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'm looking into this now and will report tomorrow.

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

On Wed, Sep 6, 2017 at 3:28 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Mon, Sep 4, 2017 at 11:43 PM, Arseny Sher <a.sher@postgrespro.ru> wrote:
>> Arseny Sher <a.sher@postgrespro.ru> writes:
>>
>>> Attached patch fixes this by stopping workers before RO drop, as
>>> already done in case when we drop replication slot.
>>
>> Sorry, here is the patch.
>>
>
> I could reproduce this issue, it's a bug. Added this to the open item.
> The cause of this is commit 7e174fa7 which make subscription ddls kill
> the apply worker only when transaction commit. I didn't look the patch
> deeply yet but I'm not sure the approach that kills apply worker
> before commit would be good.
>

FWIW, perhaps we can change the replication origin management so that
DROP SUBSCRIPTION doesn't drop the replication origin and the apply
worker itself removes it when exit. When an apply worker exits it
removes the replication origin if the corresponding subscription had
been removed.

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

Masahiko Sawada <sawada.mshk@gmail.com> writes:

> FWIW, perhaps we can change the replication origin management so that
> DROP SUBSCRIPTION doesn't drop the replication origin and the apply
> worker itself removes it when exit. When an apply worker exits it
> removes the replication origin if the corresponding subscription had
> been removed.

I don't think this is reliable -- what if worker suddenly dies without
accomplishing the job? It is enough to kill the worker during execution
of (possibly long) transaction with DROP SUBSCRIPTION to meet such
situation. Dropping replication origin in one transaction with DROP
SUBSCRIPTION seems natural to me.

--
Arseny Sher


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

On 9/4/17 10:41, Arseny Sher wrote:
> node 2:
> create table t (i int);
> create subscription s CONNECTION 'port=5432' publication p;
> begin;
> alter subscription s disable ;
> alter subscription s set (slot_name = none);
> drop subscription s;
> end;
> 
> It hangs in replorigin_drop because we wait until ReplicationState is
> released. This should happen on exit of worker, but worker will not exit
> until transaction commit because he doesn't see that the sub was
> disabled.

I think we are whacking things around a in circle now.  First we moved
the worker killing to the end of the transaction to make subscription
DDL transaction-capable.  Then we changed replication origin dropping to
wait until the worker detaches.  If you do both of these things at once,
you get this circular dependency.

We can break this in any number of ways:

- (your patch) Kill workers right away after ALTER SUBSCRIPTION DISABLE,
thus breaking the appearance of transactional DDL somewhat.

- Revert to the old behavior that the replication origin dropping fails
if it is in use.  Then you would get an error instead of hanging.  But
that was previously also reported as a bug.

- Disallow DROP SUBSCRIPTION in a transaction under certain
circumstances, for example if a transaction has previously manipulated
the same subscription.

- Have DROP SUBSCRIPTION attempt to kill workers if the subscription is
disabled (and possibly, was changed in the same transaction), which
would address this scenario very narrowly.

Maybe there are more ideas.  But I think we have to pick a poison.

Thoughts?

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

On 9/11/17 14:26, Peter Eisentraut wrote:
> On 9/10/17 12:14, Noah Misch wrote:
>> 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'm looking into this now and will report tomorrow.

I need some community feedback on the possible solutions.  I will wait
until Thursday.

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I think we are whacking things around a in circle now.  First we moved
> the worker killing to the end of the transaction to make subscription
> DDL transaction-capable.  Then we changed replication origin dropping to
> wait until the worker detaches.  If you do both of these things at once,
> you get this circular dependency.

> We can break this in any number of ways:

> - (your patch) Kill workers right away after ALTER SUBSCRIPTION DISABLE,
> thus breaking the appearance of transactional DDL somewhat.

> - Revert to the old behavior that the replication origin dropping fails
> if it is in use.  Then you would get an error instead of hanging.  But
> that was previously also reported as a bug.

> - Disallow DROP SUBSCRIPTION in a transaction under certain
> circumstances, for example if a transaction has previously manipulated
> the same subscription.

> - Have DROP SUBSCRIPTION attempt to kill workers if the subscription is
> disabled (and possibly, was changed in the same transaction), which
> would address this scenario very narrowly.

ISTM the second of those (refuse to drop an in-use subscription) is
by far the least surprising behavior.  However, I wonder if there aren't
race conditions involved here.  What if we haven't yet committed a
DROP SUBSCRIPTION, and some new worker starts up after we look for
workers?

If there aren't variants of that that will break all four options,
it's not very obvious why not.
        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

On Wed, Sep 13, 2017 at 12:48 AM, Arseny Sher <a.sher@postgrespro.ru> wrote:
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
>
>> FWIW, perhaps we can change the replication origin management so that
>> DROP SUBSCRIPTION doesn't drop the replication origin and the apply
>> worker itself removes it when exit. When an apply worker exits it
>> removes the replication origin if the corresponding subscription had
>> been removed.
>

After thought, I think we can change it like followings.
* If the replication origin is not acquired, DROP SUBSCRIPTION can drop it.
* If the replication origin is acquired by someone DROP SUBSCRIPTION
takes over a job of dropping it to the apply worker.
* The apply worker drops the replication origin when exit if the apply
worker has to drop it.

> I don't think this is reliable -- what if worker suddenly dies without
> accomplishing the job?

The apply worker will be launched by the launcher later. If DROP
SUBSCRIPTION is issued before the apply worker launches again, DROP
SUBSCRIPTION itself can remove the replication origin.

Attached a very rough patch for reference. It's very ugly but we can
deal with this case.

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> We can break this in any number of ways:
>
> - (your patch) Kill workers right away after ALTER SUBSCRIPTION DISABLE,
> thus breaking the appearance of transactional DDL somewhat.
> ...
> - Have DROP SUBSCRIPTION attempt to kill workers if the subscription is
> disabled (and possibly, was changed in the same transaction), which
> would address this scenario very narrowly.

Actually, my patch is closer to the last variant. I proposed to kill the
workers in DROP SUBSCRIPTION, and only if we are dropping replication
origin (which is probably always the case, though). I agree that it is
somewhat narrow and still slightly violates transactionality of DROP
SUBSCRIPTION, meaning that its changes (stopped workers) are seen before
the commit.

However, do we care much about that? Is there any chance that users will
rely on living apply workers after DROP SUBSCRIPTION, but before the
transaction commit? In which situation this might be useful?

On the other hand, forbidding to execute disable sub and drop sub in one
transaction makes it impossible e.g. to drop subscription in trigger as
long as Postgres doesn't have autonomous transactions.


Tom Lane <tgl@sss.pgh.pa.us> writes:
> ISTM the second of those (refuse to drop an in-use subscription) is
> by far the least surprising behavior.  However, I wonder if there aren't
> race conditions involved here.  What if we haven't yet committed a
> DROP SUBSCRIPTION, and some new worker starts up after we look for
> workers?

We hold a lock on subscription till the end of transaction, so workers
won't start.

> If there aren't variants of that that will break all four options,
> it's not very obvious why not.

I see it this way:
* We want effect of drop sub invisible till commit, so we can't stop workers before commit.
* Drop of replication origin needs to be executed in one transaction with drop sub, it writes to WAL and so must be
executedbefore commit.
 
* Apply worker needs RO for its work, it owns origin for the whole lifetime.

Something should be given up here. One more idea that was not yet
mentioned is to abandon attempts to drop subs and ROs simultenously and
just garbage-collect replication origins periodically, but that doesn't
sound as an elegant solution.


Masahiko Sawada <sawada.mshk@gmail.com> writes:

>> I don't think this is reliable -- what if worker suddenly dies without
>> accomplishing the job?
>
> The apply worker will be launched by the launcher later. If DROP
> SUBSCRIPTION is issued before the apply worker launches again, DROP
> SUBSCRIPTION itself can remove the replication origin.

Why launcher would restart the worker if we already destroyed the
subscription? Consider the sequence of actions:

* We check in DROP SUBSCRIPTION that worker alive and don't remove RO.
* DROP SUBSCRIPTION commits.
* Worker is killed by some villain before it had the chance to drop RO. It might be killed even before drop sub commit,
butafter the check, we are left with orphan RO anyway.
 

--
Arseny Sher


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

Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

> > - Disallow DROP SUBSCRIPTION in a transaction under certain
> > circumstances, for example if a transaction has previously manipulated
> > the same subscription.

> ISTM the second of those (refuse to drop an in-use subscription) is
> by far the least surprising behavior.

+1 for that option.  IIRC this has precedent for other object types such
as tables, where we refuse some action if we have already operated on
the table in the same transaction.

-- 
Á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

On Wed, Sep 13, 2017 at 8:00 PM, Arseny Sher <a.sher@postgrespro.ru> wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> We can break this in any number of ways:
>>
>> - (your patch) Kill workers right away after ALTER SUBSCRIPTION DISABLE,
>> thus breaking the appearance of transactional DDL somewhat.
>> ...
>> - Have DROP SUBSCRIPTION attempt to kill workers if the subscription is
>> disabled (and possibly, was changed in the same transaction), which
>> would address this scenario very narrowly.
>
> Actually, my patch is closer to the last variant. I proposed to kill the
> workers in DROP SUBSCRIPTION, and only if we are dropping replication
> origin (which is probably always the case, though). I agree that it is
> somewhat narrow and still slightly violates transactionality of DROP
> SUBSCRIPTION, meaning that its changes (stopped workers) are seen before
> the commit.
>
> However, do we care much about that? Is there any chance that users will
> rely on living apply workers after DROP SUBSCRIPTION, but before the
> transaction commit? In which situation this might be useful?
>
> On the other hand, forbidding to execute disable sub and drop sub in one
> transaction makes it impossible e.g. to drop subscription in trigger as
> long as Postgres doesn't have autonomous transactions.
>
>
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> ISTM the second of those (refuse to drop an in-use subscription) is
>> by far the least surprising behavior.  However, I wonder if there aren't
>> race conditions involved here.  What if we haven't yet committed a
>> DROP SUBSCRIPTION, and some new worker starts up after we look for
>> workers?
>
> We hold a lock on subscription till the end of transaction, so workers
> won't start.
>
>> If there aren't variants of that that will break all four options,
>> it's not very obvious why not.
>
> I see it this way:
> * We want effect of drop sub invisible till commit, so we can't stop
>   workers before commit.
> * Drop of replication origin needs to be executed in one transaction with
>   drop sub, it writes to WAL and so must be executed before commit.
> * Apply worker needs RO for its work, it owns origin for the whole
>   lifetime.
>
> Something should be given up here. One more idea that was not yet
> mentioned is to abandon attempts to drop subs and ROs simultenously and
> just garbage-collect replication origins periodically, but that doesn't
> sound as an elegant solution.
>
>
> Masahiko Sawada <sawada.mshk@gmail.com> writes:
>
>>> I don't think this is reliable -- what if worker suddenly dies without
>>> accomplishing the job?
>>
>> The apply worker will be launched by the launcher later. If DROP
>> SUBSCRIPTION is issued before the apply worker launches again, DROP
>> SUBSCRIPTION itself can remove the replication origin.
>
> Why launcher would restart the worker if we already destroyed the
> subscription?

Ah, the apply worker will not launch in that case.

> Consider the sequence of actions:
>
> * We check in DROP SUBSCRIPTION that worker alive and don't remove RO.
> * DROP SUBSCRIPTION commits.
> * Worker is killed by some villain before it had the chance to drop RO.
>   It might be killed even before drop sub commit, but after the check,
>   we are left with orphan RO anyway.

Hmm yes, we could left with orphan the replication origin. It's not a
good solution.

The second option makes subscription management more complex for
users. Whenever user wants to drop subscription in use, they have to
disable it before dropping the subscription, while CREATE SUBSCRIPTION
starts the logical replication without ALTER SUBSCRIPTION ENABLE. I
guess disallowing DROP SUBSCRIPTION in a transaction would rather be
more useful for users because there would not be a lot of users who
want to manage subscriptions transactionally.

Regards,

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


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

On Thu, Sep 14, 2017 at 12:04 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Wed, Sep 13, 2017 at 8:00 PM, Arseny Sher <a.sher@postgrespro.ru> wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>> We can break this in any number of ways:
>>>
>>> - (your patch) Kill workers right away after ALTER SUBSCRIPTION DISABLE,
>>> thus breaking the appearance of transactional DDL somewhat.
>>> ...
>>> - Have DROP SUBSCRIPTION attempt to kill workers if the subscription is
>>> disabled (and possibly, was changed in the same transaction), which
>>> would address this scenario very narrowly.
>>
>> Actually, my patch is closer to the last variant. I proposed to kill the
>> workers in DROP SUBSCRIPTION, and only if we are dropping replication
>> origin (which is probably always the case, though). I agree that it is
>> somewhat narrow and still slightly violates transactionality of DROP
>> SUBSCRIPTION, meaning that its changes (stopped workers) are seen before
>> the commit.
>>
>> However, do we care much about that? Is there any chance that users will
>> rely on living apply workers after DROP SUBSCRIPTION, but before the
>> transaction commit? In which situation this might be useful?
>>
>> On the other hand, forbidding to execute disable sub and drop sub in one
>> transaction makes it impossible e.g. to drop subscription in trigger as
>> long as Postgres doesn't have autonomous transactions.
>>
>>
>> Tom Lane <tgl@sss.pgh.pa.us> writes:
>>> ISTM the second of those (refuse to drop an in-use subscription) is
>>> by far the least surprising behavior.  However, I wonder if there aren't
>>> race conditions involved here.  What if we haven't yet committed a
>>> DROP SUBSCRIPTION, and some new worker starts up after we look for
>>> workers?
>>
>> We hold a lock on subscription till the end of transaction, so workers
>> won't start.
>>
>>> If there aren't variants of that that will break all four options,
>>> it's not very obvious why not.
>>
>> I see it this way:
>> * We want effect of drop sub invisible till commit, so we can't stop
>>   workers before commit.
>> * Drop of replication origin needs to be executed in one transaction with
>>   drop sub, it writes to WAL and so must be executed before commit.
>> * Apply worker needs RO for its work, it owns origin for the whole
>>   lifetime.
>>
>> Something should be given up here. One more idea that was not yet
>> mentioned is to abandon attempts to drop subs and ROs simultenously and
>> just garbage-collect replication origins periodically, but that doesn't
>> sound as an elegant solution.
>>
>>
>> Masahiko Sawada <sawada.mshk@gmail.com> writes:
>>
>>>> I don't think this is reliable -- what if worker suddenly dies without
>>>> accomplishing the job?
>>>
>>> The apply worker will be launched by the launcher later. If DROP
>>> SUBSCRIPTION is issued before the apply worker launches again, DROP
>>> SUBSCRIPTION itself can remove the replication origin.
>>
>> Why launcher would restart the worker if we already destroyed the
>> subscription?
>
> Ah, the apply worker will not launch in that case.
>
>> Consider the sequence of actions:
>>
>> * We check in DROP SUBSCRIPTION that worker alive and don't remove RO.
>> * DROP SUBSCRIPTION commits.
>> * Worker is killed by some villain before it had the chance to drop RO.
>>   It might be killed even before drop sub commit, but after the check,
>>   we are left with orphan RO anyway.
>
> Hmm yes, we could left with orphan the replication origin. It's not a
> good solution.
>
> The second option makes subscription management more complex for
> users. Whenever user wants to drop subscription in use, they have to
> disable it before dropping the subscription, while CREATE SUBSCRIPTION
> starts the logical replication without ALTER SUBSCRIPTION ENABLE. I
> guess disallowing DROP SUBSCRIPTION in a transaction would rather be
> more useful for users because there would not be a lot of users who
> want to manage subscriptions transactionally.
>

Sorry, "The second option" meant the second option of options listed by Peter.

Regards,

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


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

On 9/13/17 09:56, Alvaro Herrera wrote:
> Tom Lane wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> 
>>> - Disallow DROP SUBSCRIPTION in a transaction under certain
>>> circumstances, for example if a transaction has previously manipulated
>>> the same subscription.
> 
>> ISTM the second of those (refuse to drop an in-use subscription) is
>> by far the least surprising behavior.
> 
> +1 for that option.  IIRC this has precedent for other object types such
> as tables, where we refuse some action if we have already operated on
> the table in the same transaction.

What are some examples of such behavior?

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

Peter Eisentraut wrote:
> On 9/13/17 09:56, Alvaro Herrera wrote:
> > Tom Lane wrote:
> >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > 
> >>> - Disallow DROP SUBSCRIPTION in a transaction under certain
> >>> circumstances, for example if a transaction has previously manipulated
> >>> the same subscription.
> > 
> >> ISTM the second of those (refuse to drop an in-use subscription) is
> >> by far the least surprising behavior.
> > 
> > +1 for that option.  IIRC this has precedent for other object types such
> > as tables, where we refuse some action if we have already operated on
> > the table in the same transaction.
> 
> What are some examples of such behavior?

Search for CheckTableNotInUse() callers.

/me hates that the error messages are split across lines

-- 
Á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

On 9/14/17 08:23, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> On 9/13/17 09:56, Alvaro Herrera wrote:
>>> Tom Lane wrote:
>>>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>>
>>>>> - Disallow DROP SUBSCRIPTION in a transaction under certain
>>>>> circumstances, for example if a transaction has previously manipulated
>>>>> the same subscription.
>>>
>>>> ISTM the second of those (refuse to drop an in-use subscription) is
>>>> by far the least surprising behavior.
>>>
>>> +1 for that option.  IIRC this has precedent for other object types such
>>> as tables, where we refuse some action if we have already operated on
>>> the table in the same transaction.
>>
>> What are some examples of such behavior?
> 
> Search for CheckTableNotInUse() callers.

That one uses the relcache refcount, so we can't use that mechanism
here.  I think we need something based on xmin.  The enum code has
something like it, but I don't understand the details.

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

On 9/13/17 07:00, Arseny Sher wrote:
> On the other hand, forbidding to execute disable sub and drop sub in one
> transaction makes it impossible e.g. to drop subscription in trigger

Disabling a subscription before dropping it isn't very useful, is it?
So I'm not sure this is an important use case we need to optimize.  We
just need to prevent it from hanging.

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

On 9/12/17 15:51, Tom Lane wrote:
> ISTM the second of those (refuse to drop an in-use subscription) is
> by far the least surprising behavior.  However, I wonder if there aren't
> race conditions involved here.  What if we haven't yet committed a
> DROP SUBSCRIPTION, and some new worker starts up after we look for
> workers?

DROP SUBSCRIPTION takes an AccessExclusiveLock on pg_subscription to
prevent that.

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

> On 9/13/17 07:00, Arseny Sher wrote:
>> On the other hand, forbidding to execute disable sub and drop sub in one
>> transaction makes it impossible e.g. to drop subscription in trigger
>
> Disabling a subscription before dropping it isn't very useful, is it?
> So I'm not sure this is an important use case we need to optimize.  We
> just need to prevent it from hanging.

Not quite so. Currently it is forbidded to set subscription's slot to
NONE on enabled sub, and the only way to drop sub without touching the
slot is to run ALTER SUBSCRIPTION SET (SLOT_NAME = NONE) beforehand.
Practically this means that we have to go through disable sub -> alter
sub -> drop sub pipeline if we want to left the slot alone or just would
like to drop sub in transaction block -- with replication slot set the
latter is impossible.

--
Arseny Sher


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

On 9/14/17 15:47, Arseny Sher wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> 
>> On 9/13/17 07:00, Arseny Sher wrote:
>>> On the other hand, forbidding to execute disable sub and drop sub in one
>>> transaction makes it impossible e.g. to drop subscription in trigger
>>
>> Disabling a subscription before dropping it isn't very useful, is it?
>> So I'm not sure this is an important use case we need to optimize.  We
>> just need to prevent it from hanging.
> 
> Not quite so. Currently it is forbidded to set subscription's slot to
> NONE on enabled sub, and the only way to drop sub without touching the
> slot is to run ALTER SUBSCRIPTION SET (SLOT_NAME = NONE) beforehand.
> Practically this means that we have to go through disable sub -> alter
> sub -> drop sub pipeline if we want to left the slot alone or just would
> like to drop sub in transaction block -- with replication slot set the
> latter is impossible.

OK, good point.

Here is a simple patch that fixes this, based on my original proposal
point #4.

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

On 9/12/17 15:37, Peter Eisentraut wrote:
> On 9/11/17 14:26, Peter Eisentraut wrote:
>> On 9/10/17 12:14, Noah Misch wrote:
>>> 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'm looking into this now and will report tomorrow.
> 
> I need some community feedback on the possible solutions.  I will wait
> until Thursday.

I have posted a patch, which I will leave for comments until Sunday (so
that it could get into a tarball on Monday).

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

> Here is a simple patch that fixes this, based on my original proposal
> point #4.

I checked, it passes the tests and solves the problem. However, isn't
this

+        if (slotname || !subenabled)

is a truism? Is it possible that subscription has no slot but still
enabled?

Besides, we can avoid stopping the workers if subscription has no
associated replication origin, though this probably means that
subscription was broken by user and is not worth it.

--
Arseny Sher


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

On 9/15/17 13:35, Arseny Sher wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> 
>> Here is a simple patch that fixes this, based on my original proposal
>> point #4.
> 
> I checked, it passes the tests and solves the problem. However, isn't
> this
> 
> +        if (slotname || !subenabled)
> 
> is a truism? Is it possible that subscription has no slot but still
> enabled?

Yeah, we could just remove the _at_commit() branch entirely.  That would
effectively undo the change in 7e174fa793a2df89fe03d002a5087ef67abcdde8,
but I don't see any other choice for now.  And the practical impact
would be quite limited.

> Besides, we can avoid stopping the workers if subscription has no
> associated replication origin, though this probably means that
> subscription was broken by user and is not worth it.

Right, it seems not worth addressing this case separately.

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

On Sat, Sep 16, 2017 at 9:52 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 9/15/17 13:35, Arseny Sher wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>
>>> Here is a simple patch that fixes this, based on my original proposal
>>> point #4.
>>
>> I checked, it passes the tests and solves the problem. However, isn't
>> this
>>
>> +             if (slotname || !subenabled)
>>
>> is a truism? Is it possible that subscription has no slot but still
>> enabled?
>
> Yeah, we could just remove the _at_commit() branch entirely.  That would
> effectively undo the change in 7e174fa793a2df89fe03d002a5087ef67abcdde8,
> but I don't see any other choice for now.  And the practical impact
> would be quite limited.
>

Yeah, we can remove only _at_commit() branch, but other part of the
commit is still valid for ALTER SUBSCRIPTION DISABLE.

>> Besides, we can avoid stopping the workers if subscription has no
>> associated replication origin, though this probably means that
>> subscription was broken by user and is not worth it.
>
> Right, it seems not worth addressing this case separately.
>

Once we got this patch, DROP SUBSCRIPTION is not transactional either
if dropping a replication slot or if the subscription got disabled in
a transaction block. But we disallow to do DROP SUBSCRIPTION in a
transaction block only in the former case. In the latter case, we
adopted such non-transactional behaviour. Since these behaviours would
be complex for users I attached the documentation patch explaining it.

Regards,

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

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

On 9/16/17 08:52, Peter Eisentraut wrote:
> On 9/15/17 13:35, Arseny Sher wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>
>>> Here is a simple patch that fixes this, based on my original proposal
>>> point #4.
>> I checked, it passes the tests and solves the problem. However, isn't
>> this
>>
>> +        if (slotname || !subenabled)
>>
>> is a truism? Is it possible that subscription has no slot but still
>> enabled?
> Yeah, we could just remove the _at_commit() branch entirely.  That would
> effectively undo the change in 7e174fa793a2df89fe03d002a5087ef67abcdde8,
> but I don't see any other choice for now.  And the practical impact
> would be quite limited.

Fix committed based on this discussion.

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

On Sun, Sep 17, 2017 at 2:01 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Sat, Sep 16, 2017 at 9:52 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 9/15/17 13:35, Arseny Sher wrote:
>>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>>
>>>> Here is a simple patch that fixes this, based on my original proposal
>>>> point #4.
>>>
>>> I checked, it passes the tests and solves the problem. However, isn't
>>> this
>>>
>>> +             if (slotname || !subenabled)
>>>
>>> is a truism? Is it possible that subscription has no slot but still
>>> enabled?
>>
>> Yeah, we could just remove the _at_commit() branch entirely.  That would
>> effectively undo the change in 7e174fa793a2df89fe03d002a5087ef67abcdde8,
>> but I don't see any other choice for now.  And the practical impact
>> would be quite limited.
>>
>
> Yeah, we can remove only _at_commit() branch, but other part of the
> commit is still valid for ALTER SUBSCRIPTION DISABLE.
>
>>> Besides, we can avoid stopping the workers if subscription has no
>>> associated replication origin, though this probably means that
>>> subscription was broken by user and is not worth it.
>>
>> Right, it seems not worth addressing this case separately.
>>
>
> Once we got this patch, DROP SUBSCRIPTION is not transactional either
> if dropping a replication slot or if the subscription got disabled in
> a transaction block. But we disallow to do DROP SUBSCRIPTION in a
> transaction block only in the former case. In the latter case, we
> adopted such non-transactional behaviour. Since these behaviours would
> be complex for users I attached the documentation patch explaining it.
>

Hmm, isn't there necessary to care and mention about this kind of
inconsistent behavior in docs?

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

Masahiko Sawada <sawada.mshk@gmail.com> writes:

> On Sun, Sep 17, 2017 at 2:01 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

>> +  <para>
>> +   Since dropping a replication slot is not transactional, we cannot run
>> +   <command>DROP SUBSCRIPTION</command> inside a transaction block if dropping
>> +   the replication slot. Also, <command>DROP SUBSCRIPTOIN</command> stops the
>> +   workers if the subscription got disabled in a transaction block even if
>> +   the transaction rolls back.
>> +  </para>
>
> Hmm, isn't there necessary to care and mention about this kind of
> inconsistent behavior in docs?

Well, docs already say that dropping sub with replication slot is
non-transactional, see 'Description' section of DROP SUBSCRIPTION. As
for the second sentence, normally launcher will restart the worker
immediately after ABORT: old worker wakes up the launcher on exit, the
launcher waits on pg_subscription lock and restarts the worker instantly
after the rollback. If we tell users that the workers are stopped after
DROP SUBSCRIPTION rollback, we should also say that they will be
restarted soon.

--
Arseny Sher


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

On 9/21/17 04:43, Masahiko Sawada wrote:
>> Once we got this patch, DROP SUBSCRIPTION is not transactional either
>> if dropping a replication slot or if the subscription got disabled in
>> a transaction block. But we disallow to do DROP SUBSCRIPTION in a
>> transaction block only in the former case. In the latter case, we
>> adopted such non-transactional behaviour. Since these behaviours would
>> be complex for users I attached the documentation patch explaining it.
>>
> Hmm, isn't there necessary to care and mention about this kind of
> inconsistent behavior in docs?

I have added documentation that certain forms of CREATE/DROP
SUBSCRIPTION cannot be run inside a transaction block, which we have
done for other such commands.

I don't think we need to go into further detail.  We don't guarantee
continuous connections.  If a worker is stopped and restarted in the
background as an implementation detail, then the user is not impacted.

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

On Sat, Sep 23, 2017 at 4:06 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 9/21/17 04:43, Masahiko Sawada wrote:
>>> Once we got this patch, DROP SUBSCRIPTION is not transactional either
>>> if dropping a replication slot or if the subscription got disabled in
>>> a transaction block. But we disallow to do DROP SUBSCRIPTION in a
>>> transaction block only in the former case. In the latter case, we
>>> adopted such non-transactional behaviour. Since these behaviours would
>>> be complex for users I attached the documentation patch explaining it.
>>>
>> Hmm, isn't there necessary to care and mention about this kind of
>> inconsistent behavior in docs?
>
> I have added documentation that certain forms of CREATE/DROP
> SUBSCRIPTION cannot be run inside a transaction block, which we have
> done for other such commands.

Thank you!

> I don't think we need to go into further detail.  We don't guarantee
> continuous connections.  If a worker is stopped and restarted in the
> background as an implementation detail, then the user is not impacted.

Agreed.

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