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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
Attachments:
0001-Stop-LR-workers-before-dropping-replication-origin.patchtext/x-diffDownload
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
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
--
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 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]/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
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] /messages/by-id/20170404140717.GA2675809@tornado.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:
/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
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:
/messages/by-id/20170404140717.GA2675809@tornado.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:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.comI'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
Attachments:
very_rough_repl_origin.patchtext/x-patch; charset=US-ASCII; name=very_rough_repl_origin.patchDownload
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 2ef414e..9ed773e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -940,7 +940,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
snprintf(originname, sizeof(originname), "pg_%u", subid);
originid = replorigin_by_name(originname, true);
if (originid != InvalidRepOriginId)
- replorigin_drop(originid, false);
+ replorigin_drop(originid, true, true, true);
/*
* If there is no slot associated with the subscription, we can finish
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index edc6efb..05423a7 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -129,6 +129,8 @@ typedef struct ReplicationState
*/
ConditionVariable origin_cv;
+ bool drop_by_worker;
+
/*
* Lock protecting remote_lsn and local_lsn.
*/
@@ -329,7 +331,7 @@ replorigin_create(char *roname)
* Needs to be called in a transaction.
*/
void
-replorigin_drop(RepOriginId roident, bool nowait)
+replorigin_drop(RepOriginId roident, bool nowait, bool need_lock, bool takeover)
{
HeapTuple tuple;
Relation rel;
@@ -342,7 +344,8 @@ replorigin_drop(RepOriginId roident, bool nowait)
restart:
tuple = NULL;
/* cleanup the slot state info */
- LWLockAcquire(ReplicationOriginLock, LW_EXCLUSIVE);
+ if (need_lock)
+ LWLockAcquire(ReplicationOriginLock, LW_EXCLUSIVE);
for (i = 0; i < max_replication_slots; i++)
{
@@ -355,6 +358,22 @@ restart:
{
ConditionVariable *cv;
+ if (takeover)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("could not drop replication origin with OID %d, in use by PID %d, takeover",
+ state->roident,
+ state->acquired_by)));
+ state->drop_by_worker = true;
+ if (need_lock)
+ LWLockRelease(ReplicationOriginLock);
+
+ /* now release lock again */
+ heap_close(rel, ExclusiveLock);
+ return;
+ }
+
if (nowait)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
@@ -363,7 +382,8 @@ restart:
state->acquired_by)));
cv = &state->origin_cv;
- LWLockRelease(ReplicationOriginLock);
+ if (need_lock)
+ LWLockRelease(ReplicationOriginLock);
ConditionVariablePrepareToSleep(cv);
ConditionVariableSleep(cv, WAIT_EVENT_REPLICATION_ORIGIN_DROP);
ConditionVariableCancelSleep();
@@ -384,10 +404,12 @@ restart:
state->roident = InvalidRepOriginId;
state->remote_lsn = InvalidXLogRecPtr;
state->local_lsn = InvalidXLogRecPtr;
+ state->drop_by_worker = false;
break;
}
}
- LWLockRelease(ReplicationOriginLock);
+ if (need_lock)
+ LWLockRelease(ReplicationOriginLock);
tuple = SearchSysCache1(REPLORIGIDENT, ObjectIdGetDatum(roident));
if (!HeapTupleIsValid(tuple))
@@ -785,6 +807,7 @@ replorigin_redo(XLogReaderState *record)
state->roident = InvalidRepOriginId;
state->remote_lsn = InvalidXLogRecPtr;
state->local_lsn = InvalidXLogRecPtr;
+ state->drop_by_worker = false;
break;
}
}
@@ -987,6 +1010,15 @@ ReplicationOriginExitCleanup(int code, Datum arg)
cv = &session_replication_state->origin_cv;
session_replication_state->acquired_by = 0;
+
+ if (session_replication_state->drop_by_worker)
+ {
+ replorigin_session_origin = InvalidRepOriginId;
+ StartTransactionCommand();
+ replorigin_drop(session_replication_state->roident, false, false, false);
+ CommitTransactionCommand();
+ }
+
session_replication_state = NULL;
}
@@ -1075,6 +1107,7 @@ replorigin_session_setup(RepOriginId node)
Assert(session_replication_state->remote_lsn == InvalidXLogRecPtr);
Assert(session_replication_state->local_lsn == InvalidXLogRecPtr);
session_replication_state->roident = node;
+ session_replication_state->drop_by_worker = false;
}
@@ -1109,6 +1142,7 @@ replorigin_session_reset(void)
LWLockAcquire(ReplicationOriginLock, LW_EXCLUSIVE);
session_replication_state->acquired_by = 0;
+ session_replication_state->drop_by_worker = false;
cv = &session_replication_state->origin_cv;
session_replication_state = NULL;
@@ -1205,7 +1239,7 @@ pg_replication_origin_drop(PG_FUNCTION_ARGS)
roident = replorigin_by_name(name, false);
Assert(OidIsValid(roident));
- replorigin_drop(roident, true);
+ replorigin_drop(roident, true, true, false);
pfree(name);
diff --git a/src/include/replication/origin.h b/src/include/replication/origin.h
index a9595c3..6db5d8b 100644
--- a/src/include/replication/origin.h
+++ b/src/include/replication/origin.h
@@ -41,7 +41,8 @@ extern PGDLLIMPORT TimestampTz replorigin_session_origin_timestamp;
/* API for querying & manipulating replication origins */
extern RepOriginId replorigin_by_name(char *name, bool missing_ok);
extern RepOriginId replorigin_create(char *name);
-extern void replorigin_drop(RepOriginId roident, bool nowait);
+extern void replorigin_drop(RepOriginId roident, bool nowait, bool need_lock,
+ bool takeover);
extern bool replorigin_by_oid(RepOriginId roident, bool missing_ok,
char **roname);
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? 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.
--
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 triggerDisabling 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 triggerDisabling 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
Attachments:
0001-Fix-DROP-SUBSCRIPTION-hang.patchtext/plain; charset=UTF-8; name=0001-Fix-DROP-SUBSCRIPTION-hang.patch; x-mac-creator=0; x-mac-type=0Download
From 0474d91ee46f5974fa2e814036b7ac92819f615c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Fri, 15 Sep 2017 09:41:55 -0400
Subject: [PATCH] Fix DROP SUBSCRIPTION hang
When ALTER SUBSCRIPTION DISABLE is run in the same transaction before
DROP SUBSCRIPTION, the latter will hang because workers will still be
running, not having seen the DISABLE committed, and DROP SUBSCRIPTION
will wait until the workers have vacated the replication origin slots.
To fix, kill the workers also if dropping a subscription that is
disabled. This will address this specific scenario. If the
subscription was already disabled before the current transaction, then
there shouldn't be any workers left and this won't make a difference.
Reported-by: Arseny Sher <a.sher@postgrespro.ru>
Discussion: https://www.postgresql.org/message-id/flat/87mv6av84w.fsf%40ars-thinkpad
---
src/backend/commands/subscriptioncmds.c | 18 ++++++++++--
src/test/subscription/t/007_ddl.pl | 51 +++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+), 3 deletions(-)
create mode 100644 src/test/subscription/t/007_ddl.pl
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 2ef414e084..9f85ae937e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -816,6 +816,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
char *subname;
char *conninfo;
char *slotname;
+ bool subenabled;
List *subworkers;
ListCell *lc;
char originname[NAMEDATALEN];
@@ -886,6 +887,9 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
else
slotname = NULL;
+ /* Get subenabled */
+ subenabled = ((Form_pg_subscription) GETSTRUCT(tup))->subenabled;
+
/*
* Since dropping a replication slot is not transactional, the replication
* slot stays dropped even if the transaction rolls back. So we cannot
@@ -910,8 +914,16 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
/*
* 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.
+ * workers immediately, so that the slot becomes accessible.
+ *
+ * Also, if the subscription is disabled, stop the workers. This is
+ * necessary if the subscription was disabled in the same transaction, in
+ * which case the workers haven't seen that yet and will still be running,
+ * leading to hangs later when we want to drop the replication origin. If
+ * the subscription was disabled before this transaction, then there
+ * shouldn't be any workers left, so this won't make a difference.
+ *
+ * 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 +935,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
{
LogicalRepWorker *w = (LogicalRepWorker *) lfirst(lc);
- if (slotname)
+ if (slotname || !subenabled)
logicalrep_worker_stop(w->subid, w->relid);
else
logicalrep_worker_stop_at_commit(w->subid, w->relid);
diff --git a/src/test/subscription/t/007_ddl.pl b/src/test/subscription/t/007_ddl.pl
new file mode 100644
index 0000000000..3f36238840
--- /dev/null
+++ b/src/test/subscription/t/007_ddl.pl
@@ -0,0 +1,51 @@
+# Test some logical replication DDL behavior
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 1;
+
+sub wait_for_caught_up
+{
+ my ($node, $appname) = @_;
+
+ $node->poll_query_until('postgres',
+"SELECT pg_current_wal_lsn() <= replay_lsn FROM pg_stat_replication WHERE application_name = '$appname';"
+ ) or die "Timed out while waiting for subscriber to catch up";
+}
+
+my $node_publisher = get_new_node('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+my $node_subscriber = get_new_node('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+my $ddl = "CREATE TABLE test1 (a int, b text);";
+$node_publisher->safe_psql('postgres', $ddl);
+$node_subscriber->safe_psql('postgres', $ddl);
+
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+my $appname = 'replication_test';
+
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION mypub FOR ALL TABLES;");
+$node_subscriber->safe_psql('postgres',
+"CREATE SUBSCRIPTION mysub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION mypub;"
+);
+
+wait_for_caught_up($node_publisher, $appname);
+
+$node_subscriber->safe_psql('postgres', q{
+BEGIN;
+ALTER SUBSCRIPTION mysub DISABLE;
+ALTER SUBSCRIPTION mysub SET (slot_name = NONE);
+DROP SUBSCRIPTION mysub;
+COMMIT;
+});
+
+pass "subscription disable and drop in same transaction did not hang";
+
+$node_subscriber->stop;
+$node_publisher->stop;
--
2.11.0 (Apple Git-81)
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:
/messages/by-id/20170404140717.GA2675809@tornado.leadboat.comI'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
Attachments:
doc_drop_subscription.patchtext/x-patch; charset=US-ASCII; name=doc_drop_subscription.patchDownload
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index f535c00..a7a786e 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -93,6 +93,14 @@ DROP SUBSCRIPTION [ IF EXISTS ] <replaceable class="parameter">name</replaceable
reserve WAL and might eventually cause the disk to fill up. See
also <xref linkend="logical-replication-subscription-slot">.
</para>
+
+ <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>
</refsect1>
<refsect1>
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