DROP SUBSCRIPTION and ROLLBACK
Hi all,
While testing logical replciation I found that if the transaction
issued DROP SUBSCRIPTION rollbacks then the logical repliation stops
and the subscription can never be removed later. The document says
that the replication worker associated with the subscription will not
stop until after the transaction that issued this command has
committed but it doesn't work.
The cause of this is that DropSubscription stops the apply worker and
drops corresponding replication slot on publisher side without waiting
for commit or rollback. The launcher process launches the apply worker
again but the launched worker will fail to start logical replication
because corresponding replication slot is already removed. And the
orphan subscription can not be removed later.
I think the logical replication should not stop and the corresponding
replication slot and replication origin should not be removed until
the transaction commits.
The solution for this I came up with is that the launcher process
stops the apply worker after DROP SUBSCRIPTION is committed rather
than DropSubscription does. And the apply worker drops replication
slot and replication origin before exits. Attached draft patch fixes
this issue.
Please give me feedback.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
drop_subscription_and_rollback.patchapplication/octet-stream; name=drop_subscription_and_rollback.patchDownload
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 4353e14..f6e8331 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -432,14 +432,10 @@ DropSubscription(DropSubscriptionStmt *stmt)
Oid subid;
Datum datum;
bool isnull;
- char *subname;
char *conninfo;
+ char *subname;
char *slotname;
- char originname[NAMEDATALEN];
- char *err = NULL;
- RepOriginId originid;
- WalReceiverConn *wrconn = NULL;
- StringInfoData cmd;
+ LogicalRepWorker *worker = NULL;
rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
@@ -508,56 +504,19 @@ DropSubscription(DropSubscriptionStmt *stmt)
/* Clean up dependencies */
deleteSharedDependencyRecordsFor(SubscriptionRelationId, subid, 0);
- /* Protect against launcher restarting the worker. */
- LWLockAcquire(LogicalRepLauncherLock, LW_EXCLUSIVE);
-
- /* Kill the apply worker so that the slot becomes accessible. */
- logicalrep_worker_stop(subid);
-
- LWLockRelease(LogicalRepLauncherLock);
+ /* Mark the apply worker need to be stopped */
+ LWLockAcquire(LogicalRepWorkerLock, LW_EXCLUSIVE);
- /* 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);
-
- /* If the user asked to not drop the slot, we are done mow.*/
- if (!stmt->drop_slot)
+ worker = logicalrep_worker_find(subid);
+ if (worker)
{
- heap_close(rel, NoLock);
- return;
+ if (stmt->drop_slot)
+ worker->drop_slot = true;
+ worker->need_to_stop = true;
}
+ on_commit_launcher_wakeup = true;
- /*
- * Otherwise drop the replication slot at the publisher node using
- * the replication connection.
- */
- load_file("libpqwalreceiver", false);
-
- initStringInfo(&cmd);
- appendStringInfo(&cmd, "DROP_REPLICATION_SLOT \"%s\"", slotname);
-
- wrconn = walrcv_connect(conninfo, true, subname, &err);
- if (wrconn == NULL)
- ereport(ERROR,
- (errmsg("could not connect to publisher when attempting to "
- "drop the replication slot \"%s\"", slotname),
- errdetail("The error was: %s", err)));
-
- if (!walrcv_command(wrconn, cmd.data, &err))
- ereport(ERROR,
- (errmsg("could not drop the replication slot \"%s\" on publisher",
- slotname),
- errdetail("The error was: %s", err)));
- else
- ereport(NOTICE,
- (errmsg("dropped replication slot \"%s\" on publisher",
- slotname)));
-
- walrcv_disconnect(wrconn);
-
- pfree(cmd.data);
+ LWLockRelease(LogicalRepWorkerLock);
heap_close(rel, NoLock);
}
diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index d222cff..8e40960 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -73,7 +73,7 @@ static void logicalrep_worker_onexit(int code, Datum arg);
static void logicalrep_worker_detach(void);
bool got_SIGTERM = false;
-static bool on_commit_laucher_wakeup = false;
+bool on_commit_launcher_wakeup = false;
Datum pg_stat_get_subscription(PG_FUNCTION_ARGS);
@@ -274,6 +274,8 @@ logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid)
worker->dbid = dbid;
worker->userid = userid;
worker->subid = subid;
+ worker->need_to_stop = false;
+ worker->drop_slot = false;
LWLockRelease(LogicalRepWorkerLock);
@@ -305,28 +307,10 @@ logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid)
/*
* Stop the logical replication worker and wait until it detaches from the
* slot.
- *
- * The caller must hold LogicalRepLauncherLock to ensure that new workers are
- * not being started during this function call.
*/
void
-logicalrep_worker_stop(Oid subid)
+logicalrep_worker_stop(LogicalRepWorker *worker)
{
- LogicalRepWorker *worker;
-
- Assert(LWLockHeldByMe(LogicalRepLauncherLock));
-
- LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
-
- worker = logicalrep_worker_find(subid);
-
- /* No worker, nothing to do. */
- if (!worker)
- {
- LWLockRelease(LogicalRepWorkerLock);
- return;
- }
-
/*
* If we found worker but it does not have proc set it is starting up,
* wait for it to finish and then kill it.
@@ -335,8 +319,6 @@ logicalrep_worker_stop(Oid subid)
{
int rc;
- LWLockRelease(LogicalRepWorkerLock);
-
CHECK_FOR_INTERRUPTS();
/* Wait for signal. */
@@ -370,7 +352,6 @@ logicalrep_worker_stop(Oid subid)
/* Now terminate the worker ... */
kill(worker->proc->pid, SIGTERM);
- LWLockRelease(LogicalRepWorkerLock);
/* ... and wait for it to die. */
for (;;)
@@ -526,7 +507,7 @@ ApplyLauncherShmemInit(void)
void
AtCommit_ApplyLauncher(void)
{
- if (on_commit_laucher_wakeup)
+ if (on_commit_launcher_wakeup)
ApplyLauncherWakeup();
}
@@ -540,8 +521,8 @@ AtCommit_ApplyLauncher(void)
void
ApplyLauncherWakeupAtCommit(void)
{
- if (!on_commit_laucher_wakeup)
- on_commit_laucher_wakeup = true;
+ if (!on_commit_launcher_wakeup)
+ on_commit_launcher_wakeup = true;
}
void
@@ -579,6 +560,7 @@ ApplyLauncherMain(Datum main_arg)
/* Enter main loop */
while (!got_SIGTERM)
{
+ int i;
int rc;
List *sublist;
ListCell *lc;
@@ -590,6 +572,15 @@ ApplyLauncherMain(Datum main_arg)
now = GetCurrentTimestamp();
+ /* Check if there is worker that need to be stopped */
+ for (i = 0; i < max_logical_replication_workers; i++)
+ {
+ LogicalRepWorker *w = &LogicalRepCtx->workers[i];
+
+ if (w->need_to_stop)
+ logicalrep_worker_stop(w);
+ }
+
/* Limit the start retry to once a wal_retrieve_retry_interval */
if (TimestampDifferenceExceeds(last_start_time, now,
wal_retrieve_retry_interval))
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 9383960..4e666bc 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -1022,7 +1022,7 @@ ApplyLoop(void)
}
}
- if (!in_remote_transaction)
+ if (!in_remote_transaction && !MyLogicalRepWorker->need_to_stop)
{
/*
* If we didn't get any transactions for a while there might be
@@ -1221,7 +1221,7 @@ reread_subscription(void)
/*
* Exit if the subscription was removed.
* This normally should not happen as the worker gets killed
- * during DROP SUBSCRIPTION.
+ * after DROP SUBSCRIPTION committed.
*/
if (!newsub)
{
@@ -1321,6 +1321,7 @@ ApplyWorkerMain(Datum main_arg)
int server_version;
TimeLineID startpointTLI;
WalRcvStreamOptions options;
+ WalReceiverConn *conn = NULL;
/* Attach to slot */
logicalrep_worker_attach(worker_slot);
@@ -1422,8 +1423,55 @@ ApplyWorkerMain(Datum main_arg)
/* Run the main loop. */
ApplyLoop();
+ /* disconnect logical replication */
walrcv_disconnect(wrconn);
+ conn = walrcv_connect(MySubscription->conninfo, true,
+ MySubscription->name, &err);
+ if (conn == NULL)
+ ereport(ERROR,
+ (errmsg("could not connect to publisher when attempting to "
+ "drop the replication slot \"%s\"", MySubscription->slotname),
+ errdetail("The error was: %s", err)));
+
+ /*
+ * Drop the replication slot at the publisher node using
+ * the replication connection.
+ */
+ if (MyLogicalRepWorker->drop_slot)
+ {
+ StringInfoData cmd;
+
+ initStringInfo(&cmd);
+ appendStringInfo(&cmd, "DROP_REPLICATION_SLOT \"%s\"", MySubscription->slotname);
+
+ if (!walrcv_command(conn, cmd.data, &err))
+ ereport(ERROR,
+ (errmsg("could not drop the replication slot \"%s\" on publisher",
+ MySubscription->slotname),
+ errdetail("The error was: %s", err)));
+ else
+ ereport(NOTICE,
+ (errmsg("dropped replication slot \"%s\" on publisher",
+ MySubscription->slotname)));
+ pfree(cmd.data);
+ }
+
+ /* Remove the origin tracking if exists. */
+ if (originid != InvalidRepOriginId)
+ {
+ replorigin_session_reset();
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+
+ StartTransactionCommand();
+ replorigin_drop(originid);
+ CommitTransactionCommand();
+ }
+
+ walrcv_disconnect(conn);
+
/* We should only get here if we received SIGTERM */
proc_exit(0);
}
diff --git a/src/include/replication/worker_internal.h b/src/include/replication/worker_internal.h
index 8cbf268..3db6f55 100644
--- a/src/include/replication/worker_internal.h
+++ b/src/include/replication/worker_internal.h
@@ -40,6 +40,8 @@ typedef struct LogicalRepWorker
TimestampTz last_recv_time;
XLogRecPtr reply_lsn;
TimestampTz reply_time;
+ bool need_to_stop; /* Set by the backend executing DROP SUBSCRIPTION */
+ bool drop_slot; /* Drop replication slot when exits? */
} LogicalRepWorker;
/* libpqreceiver connection */
@@ -51,12 +53,13 @@ extern LogicalRepWorker *MyLogicalRepWorker;
extern bool in_remote_transaction;
extern bool got_SIGTERM;
+extern bool on_commit_launcher_wakeup;
extern void logicalrep_worker_attach(int slot);
extern LogicalRepWorker *logicalrep_worker_find(Oid subid);
extern int logicalrep_worker_count(Oid subid);
extern void logicalrep_worker_launch(Oid dbid, Oid subid, const char *subname, Oid userid);
-extern void logicalrep_worker_stop(Oid subid);
+extern void logicalrep_worker_stop(LogicalRepWorker *worker);
extern void logicalrep_worker_wakeup(Oid subid);
extern void logicalrep_worker_sigterm(SIGNAL_ARGS);
diff --git a/src/test/subscription/t/001_rep_changes.pl b/src/test/subscription/t/001_rep_changes.pl
index b51740b..39dc97d 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -5,6 +5,27 @@ use PostgresNode;
use TestLib;
use Test::More tests => 11;
+sub test_sub
+{
+ my ($self, $expected, $msg, $sql) = @_;
+
+ my $timeout_max = 30;
+ my $timeout = 0;
+ my $result;
+
+ while ($timeout < $timeout_max)
+ {
+ $result = $self->safe_psql('postgres', $sql);
+
+ last if ($result eq $expected);
+
+ $timeout++;
+ sleep 1;
+ }
+
+ is ($result, $expected, $msg);
+}
+
# Initialize publisher node
my $node_publisher = get_new_node('publisher');
$node_publisher->init(allows_streaming => 'logical');
@@ -169,6 +190,13 @@ $result =
$node_subscriber->safe_psql('postgres', "SELECT count(*), min(a), max(a) FROM tab_full");
is($result, qq(11|0|100), 'check replicated insert after alter publication');
+# check if DROP SUBSCRIPTION is transactional
+$node_subscriber->safe_psql('postgres', "
+BEGIN;
+DROP SUBSCRIPTION tap_sub;
+ROLLBACK;
+");
+
# check all the cleanup
$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub");
@@ -176,13 +204,13 @@ $result =
$node_subscriber->safe_psql('postgres', "SELECT count(*) FROM pg_subscription");
is($result, qq(0), 'check subscription was dropped on subscriber');
-$result =
- $node_publisher->safe_psql('postgres', "SELECT count(*) FROM pg_replication_slots");
-is($result, qq(0), 'check replication slot was dropped on publisher');
+test_sub($node_publisher, qq(0),
+ 'check replication slot was dropped on publisher',
+ "SELECT count(*) FROM pg_replication_slots");
-$result =
- $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM pg_replication_origin");
-is($result, qq(0), 'check replication origin was dropped on subscriber');
+test_sub($node_publisher, qq(0),
+ 'check replication origin was dropped on subscriber',
+ "SELECT count(*) FROM pg_replication_origin");
$node_subscriber->stop('fast');
$node_publisher->stop('fast');
On Tue, Feb 7, 2017 at 9:10 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Hi all,
While testing logical replciation I found that if the transaction
issued DROP SUBSCRIPTION rollbacks then the logical repliation stops
and the subscription can never be removed later. The document says
that the replication worker associated with the subscription will not
stop until after the transaction that issued this command has
committed but it doesn't work.
Yeah, this is a bug.
ISTM that CREATE SUBSCRIPTION also has the similar issue. It creates
the replication slot on the publisher side before the transaction has been
committed. Even if the transaction is rollbacked, that replication slot is
not removed. That is, in a transaction block, we should not connect to
the publisher. Instead, the launcher or worker should do.
The cause of this is that DropSubscription stops the apply worker and
drops corresponding replication slot on publisher side without waiting
for commit or rollback. The launcher process launches the apply worker
again but the launched worker will fail to start logical replication
because corresponding replication slot is already removed. And the
orphan subscription can not be removed later.I think the logical replication should not stop and the corresponding
replication slot and replication origin should not be removed until
the transaction commits.
Yes.
The solution for this I came up with is that the launcher process
stops the apply worker after DROP SUBSCRIPTION is committed rather
than DropSubscription does. And the apply worker drops replication
slot and replication origin before exits. Attached draft patch fixes
this issue.Please give me feedback.
The patch failed to apply to HEAD.
+ worker = logicalrep_worker_find(subid);
+ if (worker)
{
- heap_close(rel, NoLock);
- return;
+ if (stmt->drop_slot)
+ worker->drop_slot = true;
+ worker->need_to_stop = true;"drop_slot" and "need_to_stop" seem to be set to true even if the transaction
is rollbacked. This would cause the same problem that you're trying to fix.
I think that we should make the launcher periodically checks pg_subscription
and stop the worker if there is no its corresponding subscription. Then,
if necessary, the worker should remove its replication slot from the publisher.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/02/17 13:10, Masahiko Sawada wrote:
Hi all,
While testing logical replciation I found that if the transaction
issued DROP SUBSCRIPTION rollbacks then the logical repliation stops
and the subscription can never be removed later. The document says
that the replication worker associated with the subscription will not
stop until after the transaction that issued this command has
committed but it doesn't work.
Ah then the docs are wrong and should be fixed. Maybe we should not
allow DROP SUBSCRIPTION inside transaction similarly to CREATE INDEX
CONCURRENTLY.
The cause of this is that DropSubscription stops the apply worker and
drops corresponding replication slot on publisher side without waiting
for commit or rollback. The launcher process launches the apply worker
again but the launched worker will fail to start logical replication
because corresponding replication slot is already removed. And the
orphan subscription can not be removed later.I think the logical replication should not stop and the corresponding
replication slot and replication origin should not be removed until
the transaction commits.The solution for this I came up with is that the launcher process
stops the apply worker after DROP SUBSCRIPTION is committed rather
than DropSubscription does. And the apply worker drops replication
slot and replication origin before exits. Attached draft patch fixes
this issue.
I don't think we can allow the slot drop to be postponed. There is too
many failure scenarios where we would leave the remote slot in the
database and that's not acceptable IMHO.
For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/02/17 16:26, Petr Jelinek wrote:
On 07/02/17 13:10, Masahiko Sawada wrote:
I think the logical replication should not stop and the corresponding
replication slot and replication origin should not be removed until
the transaction commits.The solution for this I came up with is that the launcher process
stops the apply worker after DROP SUBSCRIPTION is committed rather
than DropSubscription does. And the apply worker drops replication
slot and replication origin before exits. Attached draft patch fixes
this issue.I don't think we can allow the slot drop to be postponed. There is too
many failure scenarios where we would leave the remote slot in the
database and that's not acceptable IMHO.For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?
Not to mention that slot creation/drop is not transactional by itself so
even if there was some way to tie remote transaction to local
transaction (like say 2pc), it would still not work with ROLLBACK.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
On 07/02/17 13:10, Masahiko Sawada wrote:
Hi all,
While testing logical replciation I found that if the transaction
issued DROP SUBSCRIPTION rollbacks then the logical repliation stops
and the subscription can never be removed later. The document says
that the replication worker associated with the subscription will not
stop until after the transaction that issued this command has
committed but it doesn't work.Ah then the docs are wrong and should be fixed. Maybe we should not
allow DROP SUBSCRIPTION inside transaction similarly to CREATE INDEX
CONCURRENTLY.The cause of this is that DropSubscription stops the apply worker and
drops corresponding replication slot on publisher side without waiting
for commit or rollback. The launcher process launches the apply worker
again but the launched worker will fail to start logical replication
because corresponding replication slot is already removed. And the
orphan subscription can not be removed later.I think the logical replication should not stop and the corresponding
replication slot and replication origin should not be removed until
the transaction commits.The solution for this I came up with is that the launcher process
stops the apply worker after DROP SUBSCRIPTION is committed rather
than DropSubscription does. And the apply worker drops replication
slot and replication origin before exits. Attached draft patch fixes
this issue.I don't think we can allow the slot drop to be postponed. There is too
many failure scenarios where we would leave the remote slot in the
database and that's not acceptable IMHO.For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?
Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.
Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.
Regards,
--
Fujii Masao
--
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, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.
It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.
Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.
For consistency that may be important.
--
Michael
--
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, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.
Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.
Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
drop_subscription_and_rollback_v2.patchapplication/octet-stream; name=drop_subscription_and_rollback_v2.patchDownload
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 9f2fb93..17235ad 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -38,8 +38,8 @@ DROP SUBSCRIPTION [ IF EXISTS ] <replaceable class="parameter">name</replaceable
</para>
<para>
- The replication worker associated with the subscription will not stop until
- after the transaction that issued this command has committed.
+ <command>DROP SUBSCRIPTION</command> cannot be executed inside a
+ transaction block.
</para>
</refsect1>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index ab21e64..c966acc 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -18,6 +18,7 @@
#include "access/heapam.h"
#include "access/htup_details.h"
+#include "access/xact.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
@@ -38,6 +39,7 @@
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/snapmgr.h"
#include "utils/syscache.h"
/*
@@ -424,7 +426,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
* Drop a subscription
*/
void
-DropSubscription(DropSubscriptionStmt *stmt)
+DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -441,6 +443,12 @@ DropSubscription(DropSubscriptionStmt *stmt)
WalReceiverConn *wrconn = NULL;
StringInfoData cmd;
+ /*
+ * We cannot run DROP SUBSCRIPTION inside a user transaction
+ * block.
+ */
+ PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");
+
rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
tup = SearchSysCache2(SUBSCRIPTIONNAME, MyDatabaseId,
@@ -508,6 +516,20 @@ DropSubscription(DropSubscriptionStmt *stmt)
/* Clean up dependencies */
deleteSharedDependencyRecordsFor(SubscriptionRelationId, subid, 0);
+ /* Save the origin */
+ snprintf(originname, sizeof(originname), "pg_%u", subid);
+ originid = replorigin_by_name(originname, true);
+
+ heap_close(rel, NoLock);
+
+ /*
+ * Commit transcation and start new transaction just after
+ * drop subscription.
+ */
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ StartTransactionCommand();
+
/* Protect against launcher restarting the worker. */
LWLockAcquire(LogicalRepLauncherLock, LW_EXCLUSIVE);
@@ -516,18 +538,13 @@ DropSubscription(DropSubscriptionStmt *stmt)
LWLockRelease(LogicalRepLauncherLock);
- /* Remove the origin tracking if exists. */
- snprintf(originname, sizeof(originname), "pg_%u", subid);
- originid = replorigin_by_name(originname, true);
+ /* Drop the origin trancking if exists */
if (originid != InvalidRepOriginId)
replorigin_drop(originid);
/* If the user asked to not drop the slot, we are done mow.*/
if (!stmt->drop_slot)
- {
- heap_close(rel, NoLock);
return;
- }
/*
* Otherwise drop the replication slot at the publisher node using
@@ -558,8 +575,6 @@ DropSubscription(DropSubscriptionStmt *stmt)
walrcv_disconnect(wrconn);
pfree(cmd.data);
-
- heap_close(rel, NoLock);
}
/*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 5d3be38..26def04 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1616,7 +1616,7 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_DropSubscriptionStmt:
- DropSubscription((DropSubscriptionStmt *) parsetree);
+ DropSubscription((DropSubscriptionStmt *) parsetree, isTopLevel);
/* no commands stashed for DROP */
commandCollected = true;
break;
diff --git a/src/include/commands/subscriptioncmds.h b/src/include/commands/subscriptioncmds.h
index 127696c..e3f83db 100644
--- a/src/include/commands/subscriptioncmds.h
+++ b/src/include/commands/subscriptioncmds.h
@@ -20,7 +20,7 @@
extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt);
extern ObjectAddress AlterSubscription(AlterSubscriptionStmt *stmt);
-extern void DropSubscription(DropSubscriptionStmt *stmt);
+extern void DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel);
extern ObjectAddress AlterSubscriptionOwner(const char *name, Oid newOwnerId);
extern void AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId);
On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.
Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?
Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.
I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.
Thought?
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 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.
We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.
IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.
Note that we do give users options to not create and not drop slots if
they wish so we should really treat slot related failures as command errors.
I don't want subscription setup to be 20 step where you have to deal
with various things on multiple servers when it can be just plain simple
one command on each side in many cases.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?
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 Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.
On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?
Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.
Thank you for the explanation. I understood and agree.
I think we should disallow to call ALTER SUBSCRIPTION inside a user's
transaction block as well.
Attached patch changes these three DDLs so that they cannot be called
inside a user's transaction block.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
disallow_sub_ddls_in_transaction_block.patchapplication/octet-stream; name=disallow_sub_ddls_in_transaction_block.patchDownload
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 032ecbb..2a4eb58 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -48,6 +48,11 @@ ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> DISABLE
To alter the owner, you must also be a direct or indirect member of the
new owning role. The new owner has to be a superuser
</para>
+
+ <para>
+ <command>ALTER SUBSCRIPTION</command> cannot be executed inside a
+ transaciton block.
+ </para>
</refsect1>
<refsect1>
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 59e5ad0..6480690 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -47,8 +47,9 @@ CREATE SUBSCRIPTION <replaceable class="PARAMETER">subscription_name</replaceabl
</para>
<para>
- A logical replication worker will be started to replicate data for the new
- subscription at the commit of the transaction where this command is run.
+ <command>CREATE SUBSCRIPTION</command> cannot be executed inside a
+ transaction block. A logical replication worker will be started to replicate
+ data for the new subscription at the end of this command.
</para>
<para>
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 9f2fb93..17235ad 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -38,8 +38,8 @@ DROP SUBSCRIPTION [ IF EXISTS ] <replaceable class="parameter">name</replaceable
</para>
<para>
- The replication worker associated with the subscription will not stop until
- after the transaction that issued this command has committed.
+ <command>DROP SUBSCRIPTION</command> cannot be executed inside a
+ transaction block.
</para>
</refsect1>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index ab21e64..6903a15 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -18,6 +18,7 @@
#include "access/heapam.h"
#include "access/htup_details.h"
+#include "access/xact.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
@@ -204,7 +205,7 @@ publicationListToArray(List *publist)
* Create new subscription.
*/
ObjectAddress
-CreateSubscription(CreateSubscriptionStmt *stmt)
+CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -221,6 +222,12 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
bool create_slot;
List *publications;
+ /*
+ * We cannot run CREATE SUBSCRIPTION inside a user transaction
+ * block.
+ */
+ PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION");
+
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -335,7 +342,7 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
* Alter the existing subscription.
*/
ObjectAddress
-AlterSubscription(AlterSubscriptionStmt *stmt)
+AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -350,6 +357,12 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
char *slot_name;
List *publications;
+ /*
+ * We cannot run CREATE SUBSCRIPTION inside a user transaction
+ * block.
+ */
+ PreventTransactionChain(isTopLevel, "ALTER SUBSCRIPTION");
+
rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
/* Fetch the existing tuple. */
@@ -424,7 +437,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
* Drop a subscription
*/
void
-DropSubscription(DropSubscriptionStmt *stmt)
+DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -441,6 +454,12 @@ DropSubscription(DropSubscriptionStmt *stmt)
WalReceiverConn *wrconn = NULL;
StringInfoData cmd;
+ /*
+ * We cannot run DROP SUBSCRIPTION inside a user transaction
+ * block.
+ */
+ PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");
+
rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
tup = SearchSysCache2(SUBSCRIPTIONNAME, MyDatabaseId,
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 5d3be38..a9856ad 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1608,15 +1608,17 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_CreateSubscriptionStmt:
- address = CreateSubscription((CreateSubscriptionStmt *) parsetree);
+ address = CreateSubscription((CreateSubscriptionStmt *) parsetree,
+ isTopLevel);
break;
case T_AlterSubscriptionStmt:
- address = AlterSubscription((AlterSubscriptionStmt *) parsetree);
+ address = AlterSubscription((AlterSubscriptionStmt *) parsetree,
+ isTopLevel);
break;
case T_DropSubscriptionStmt:
- DropSubscription((DropSubscriptionStmt *) parsetree);
+ DropSubscription((DropSubscriptionStmt *) parsetree, isTopLevel);
/* no commands stashed for DROP */
commandCollected = true;
break;
diff --git a/src/include/commands/subscriptioncmds.h b/src/include/commands/subscriptioncmds.h
index 127696c..4d8470f 100644
--- a/src/include/commands/subscriptioncmds.h
+++ b/src/include/commands/subscriptioncmds.h
@@ -18,9 +18,11 @@
#include "catalog/objectaddress.h"
#include "nodes/parsenodes.h"
-extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt);
-extern ObjectAddress AlterSubscription(AlterSubscriptionStmt *stmt);
-extern void DropSubscription(DropSubscriptionStmt *stmt);
+extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt,
+ bool isTopLevel);
+extern ObjectAddress AlterSubscription(AlterSubscriptionStmt *stmt,
+ bool isTopLevel);
+extern void DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel);
extern ObjectAddress AlterSubscriptionOwner(const char *name, Oid newOwnerId);
extern void AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId);
On 15/02/17 06:43, Masahiko Sawada wrote:
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.Thank you for the explanation. I understood and agree.
I think we should disallow to call ALTER SUBSCRIPTION inside a user's
transaction block as well.
Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
On 15/02/17 06:43, Masahiko Sawada wrote:
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.Thank you for the explanation. I understood and agree.
I think we should disallow to call ALTER SUBSCRIPTION inside a user's
transaction block as well.Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever.
Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached
fixed version patch.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
disallow_sub_ddls_in_transaction_block_v2.patchapplication/octet-stream; name=disallow_sub_ddls_in_transaction_block_v2.patchDownload
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 59e5ad0..6480690 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -47,8 +47,9 @@ CREATE SUBSCRIPTION <replaceable class="PARAMETER">subscription_name</replaceabl
</para>
<para>
- A logical replication worker will be started to replicate data for the new
- subscription at the commit of the transaction where this command is run.
+ <command>CREATE SUBSCRIPTION</command> cannot be executed inside a
+ transaction block. A logical replication worker will be started to replicate
+ data for the new subscription at the end of this command.
</para>
<para>
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 9f2fb93..17235ad 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -38,8 +38,8 @@ DROP SUBSCRIPTION [ IF EXISTS ] <replaceable class="parameter">name</replaceable
</para>
<para>
- The replication worker associated with the subscription will not stop until
- after the transaction that issued this command has committed.
+ <command>DROP SUBSCRIPTION</command> cannot be executed inside a
+ transaction block.
</para>
</refsect1>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index ab21e64..1779719 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -18,6 +18,7 @@
#include "access/heapam.h"
#include "access/htup_details.h"
+#include "access/xact.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
@@ -204,7 +205,7 @@ publicationListToArray(List *publist)
* Create new subscription.
*/
ObjectAddress
-CreateSubscription(CreateSubscriptionStmt *stmt)
+CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -221,6 +222,12 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
bool create_slot;
List *publications;
+ /*
+ * We cannot run CREATE SUBSCRIPTION inside a user transaction
+ * block.
+ */
+ PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION");
+
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -424,7 +431,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
* Drop a subscription
*/
void
-DropSubscription(DropSubscriptionStmt *stmt)
+DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -441,6 +448,12 @@ DropSubscription(DropSubscriptionStmt *stmt)
WalReceiverConn *wrconn = NULL;
StringInfoData cmd;
+ /*
+ * We cannot run DROP SUBSCRIPTION inside a user transaction
+ * block.
+ */
+ PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");
+
rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
tup = SearchSysCache2(SUBSCRIPTIONNAME, MyDatabaseId,
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 3bc0ae5..20b5273 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1609,7 +1609,8 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_CreateSubscriptionStmt:
- address = CreateSubscription((CreateSubscriptionStmt *) parsetree);
+ address = CreateSubscription((CreateSubscriptionStmt *) parsetree,
+ isTopLevel);
break;
case T_AlterSubscriptionStmt:
@@ -1617,7 +1618,7 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_DropSubscriptionStmt:
- DropSubscription((DropSubscriptionStmt *) parsetree);
+ DropSubscription((DropSubscriptionStmt *) parsetree, isTopLevel);
/* no commands stashed for DROP */
commandCollected = true;
break;
diff --git a/src/include/commands/subscriptioncmds.h b/src/include/commands/subscriptioncmds.h
index 127696c..1765879 100644
--- a/src/include/commands/subscriptioncmds.h
+++ b/src/include/commands/subscriptioncmds.h
@@ -18,9 +18,10 @@
#include "catalog/objectaddress.h"
#include "nodes/parsenodes.h"
-extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt);
+extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt,
+ bool isTopLevel);
extern ObjectAddress AlterSubscription(AlterSubscriptionStmt *stmt);
-extern void DropSubscription(DropSubscriptionStmt *stmt);
+extern void DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel);
extern ObjectAddress AlterSubscriptionOwner(const char *name, Oid newOwnerId);
extern void AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId);
On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 15/02/17 06:43, Masahiko Sawada wrote:
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.Thank you for the explanation. I understood and agree.
I think we should disallow to call ALTER SUBSCRIPTION inside a user's
transaction block as well.Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever.
Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached
fixed version patch.
We should disallow CREATE/DROP SUBSCRIPTION inside a user transaction
block only when CREATE/DROP SLOT option is set?
+ /*
+ * We cannot run CREATE SUBSCRIPTION inside a user transaction
+ * block.
+ */
+ PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION");I think that more comments about why the command is disallowed inside
a user transaction block are necessary. For example,
----------------------
Disallow CREATE SUBSCRIPTION [CREATE SLOT] inside a user transaction block.
When CREATE SLOT is set, this command creates the replication slot on
the remote server. This operation is not transactional. So, if the transaction
is rollbacked, the created replication slot unexpectedly remains while
there is no corresponding entry in pg_subscription. To reduce the possibility
of this problem, we allow CREATE SLOT option only outside a user transaction
block.
XXX Note that this restriction cannot completely prevent "orphan" replication
slots. The transaction of CREATE SUBSCRIPTION can still fail after creating
the replication slot on the remote server, though it's basically less likely
to happen.
----------------------
+ * We cannot run DROP SUBSCRIPTION inside a user transaction
+ * block.
+ */
+ PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");Same as above.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 15/02/17 06:43, Masahiko Sawada wrote:
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.Thank you for the explanation. I understood and agree.
I think we should disallow to call ALTER SUBSCRIPTION inside a user's
transaction block as well.Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever.
Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached
fixed version patch.We should disallow CREATE/DROP SUBSCRIPTION inside a user transaction
block only when CREATE/DROP SLOT option is set?+ /* + * We cannot run CREATE SUBSCRIPTION inside a user transaction + * block. + */ + PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION");I think that more comments about why the command is disallowed inside
a user transaction block are necessary. For example,
I agree with you.
----------------------
Disallow CREATE SUBSCRIPTION [CREATE SLOT] inside a user transaction block.When CREATE SLOT is set, this command creates the replication slot on
the remote server. This operation is not transactional. So, if the transaction
is rollbacked, the created replication slot unexpectedly remains while
there is no corresponding entry in pg_subscription. To reduce the possibility
of this problem, we allow CREATE SLOT option only outside a user transaction
block.XXX Note that this restriction cannot completely prevent "orphan" replication
slots. The transaction of CREATE SUBSCRIPTION can still fail after creating
the replication slot on the remote server, though it's basically less likely
to happen.
----------------------+ * We cannot run DROP SUBSCRIPTION inside a user transaction + * block. + */ + PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");Same as above.
While writing regression test for this issue, I found an another bug
of DROP SUBSCRIPTION; DROP SUBSCRIPTION with DROP SLOT option fails to
parse because DROP is a keyword, not IDENT. Attached 000 patch fixes
it, and 001 patches fixes the original issue on this thread.
Please review these.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
001_disallow_sub_ddls_in_transaction_block_v3.patchapplication/octet-stream; name=001_disallow_sub_ddls_in_transaction_block_v3.patchDownload
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 250806f..406b518 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -47,8 +47,10 @@ CREATE SUBSCRIPTION <replaceable class="PARAMETER">subscription_name</replaceabl
</para>
<para>
- A logical replication worker will be started to replicate data for the new
- subscription at the commit of the transaction where this command is run.
+ <command>CREATE SUBSCRIPTION</command> cannot be executed inside a
+ transaction block when <literal>CREATE SLOT</literal> is specified.
+ A logical replication worker will be started to replicate data for
+ the new subscription at the end of this command.
</para>
<para>
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 9f2fb93..4228f1a 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -38,8 +38,8 @@ DROP SUBSCRIPTION [ IF EXISTS ] <replaceable class="parameter">name</replaceable
</para>
<para>
- The replication worker associated with the subscription will not stop until
- after the transaction that issued this command has committed.
+ <command>DROP SUBSCRIPTION</command> cannot be executed inside a
+ transaction block when <literal>DROP SLOT</literal> is specified.
</para>
</refsect1>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index ab21e64..26b38af 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -18,6 +18,7 @@
#include "access/heapam.h"
#include "access/htup_details.h"
+#include "access/xact.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
@@ -204,7 +205,7 @@ publicationListToArray(List *publist)
* Create new subscription.
*/
ObjectAddress
-CreateSubscription(CreateSubscriptionStmt *stmt)
+CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -221,6 +222,24 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
bool create_slot;
List *publications;
+ /*
+ * Parse and check options.
+ * Connection and publication should not be specified here.
+ */
+ parse_subscription_options(stmt->options, NULL, NULL,
+ &enabled_given, &enabled,
+ &create_slot, &slotname);
+
+ /*
+ * Since creating replication slot is not transactional, it
+ * leaves created replication slot even when the transaction
+ * rollbacks, while there is no corresponding entry in
+ * pg_subscription. So we cannot run CREATE SUBSCRIPTION inside
+ * a user transaction block if creating replication slot.
+ */
+ if (create_slot)
+ PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION CREATE SLOT");
+
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -239,13 +258,6 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
stmt->subname)));
}
- /*
- * Parse and check options.
- * Connection and publication should not be specified here.
- */
- parse_subscription_options(stmt->options, NULL, NULL,
- &enabled_given, &enabled,
- &create_slot, &slotname);
if (slotname == NULL)
slotname = stmt->subname;
@@ -424,7 +436,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
* Drop a subscription
*/
void
-DropSubscription(DropSubscriptionStmt *stmt)
+DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -441,6 +453,15 @@ DropSubscription(DropSubscriptionStmt *stmt)
WalReceiverConn *wrconn = NULL;
StringInfoData cmd;
+ /*
+ * Since dropping replication slot is not transactional, it drops
+ * the replication slot even when the transaction rollbacks.
+ * So we cannot run DROP SUBSCRIPTION inside a user transaction
+ * block if dropping replication slot.
+ */
+ if (stmt->drop_slot)
+ PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION DROP SLOT");
+
rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
tup = SearchSysCache2(SUBSCRIPTIONNAME, MyDatabaseId,
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 3bc0ae5..20b5273 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1609,7 +1609,8 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_CreateSubscriptionStmt:
- address = CreateSubscription((CreateSubscriptionStmt *) parsetree);
+ address = CreateSubscription((CreateSubscriptionStmt *) parsetree,
+ isTopLevel);
break;
case T_AlterSubscriptionStmt:
@@ -1617,7 +1618,7 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_DropSubscriptionStmt:
- DropSubscription((DropSubscriptionStmt *) parsetree);
+ DropSubscription((DropSubscriptionStmt *) parsetree, isTopLevel);
/* no commands stashed for DROP */
commandCollected = true;
break;
diff --git a/src/include/commands/subscriptioncmds.h b/src/include/commands/subscriptioncmds.h
index 127696c..1765879 100644
--- a/src/include/commands/subscriptioncmds.h
+++ b/src/include/commands/subscriptioncmds.h
@@ -18,9 +18,10 @@
#include "catalog/objectaddress.h"
#include "nodes/parsenodes.h"
-extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt);
+extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt,
+ bool isTopLevel);
extern ObjectAddress AlterSubscription(AlterSubscriptionStmt *stmt);
-extern void DropSubscription(DropSubscriptionStmt *stmt);
+extern void DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel);
extern ObjectAddress AlterSubscriptionOwner(const char *name, Oid newOwnerId);
extern void AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId);
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 68c17d5..b08374d 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -12,6 +12,11 @@ CREATE SUBSCRIPTION testsub CONNECTION 'foo';
CREATE SUBSCRIPTION testsub PUBLICATION foo;
set client_min_messages to error;
+-- fail - cannot CREATE SUBSCRIPTION CREATE SLOT inside transaction block
+BEGIN;
+CREATE SUBSCRIPTION testsub CONNECTION 'testconn' PUBLICATION testpub WITH (CREATE SLOT);
+COMMIT;
+
CREATE SUBSCRIPTION testsub CONNECTION 'testconn' PUBLICATION testpub;
CREATE SUBSCRIPTION testsub CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (DISABLED, NOCREATE SLOT);
reset client_min_messages;
@@ -38,7 +43,14 @@ ALTER SUBSCRIPTION testsub DISABLE;
COMMIT;
+-- fail - cannot DROP SUBSCRIPTION DROP SLOT inside transaction block
+BEGIN;
+DROP SUBSCRIPTION testsub DROP SLOT;
+COMMIT;
+
+BEGIN;
DROP SUBSCRIPTION testsub NODROP SLOT;
+COMMIT;
RESET SESSION AUTHORIZATION;
DROP ROLE regress_subscription_user;
000_fix_drop_sub_drop_slot.patchapplication/octet-stream; name=000_fix_drop_sub_drop_slot.patchDownload
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 07cc81e..af47a01 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -644,7 +644,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
MAPPING MATCH MATERIALIZED MAXVALUE METHOD MINUTE_P MINVALUE MODE MONTH_P MOVE
- NAME_P NAMES NATIONAL NATURAL NCHAR NEW NEXT NO NONE
+ NAME_P NAMES NATIONAL NATURAL NCHAR NEW NEXT NO NODROP NONE
NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
NULLS_P NUMERIC
@@ -9190,19 +9190,9 @@ DropSubscriptionStmt: DROP SUBSCRIPTION name opt_drop_slot
;
opt_drop_slot:
- IDENT SLOT
- {
- if (strcmp($1, "drop") == 0)
- $$ = TRUE;
- else if (strcmp($1, "nodrop") == 0)
- $$ = FALSE;
- else
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("unrecognized option \"%s\"", $1),
- parser_errposition(@1)));
- }
- | /*EMPTY*/ { $$ = TRUE; }
+ DROP SLOT { $$ = TRUE; }
+ | NODROP SLOT { $$ = FALSE; }
+ | /* EMPTY */ { $$ = TRUE; }
;
/*****************************************************************************
@@ -14441,6 +14431,7 @@ unreserved_keyword:
| NEW
| NEXT
| NO
+ | NODROP
| NOTHING
| NOTIFY
| NOWAIT
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index 985d650..f7bbe71 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -256,6 +256,7 @@ PG_KEYWORD("nchar", NCHAR, COL_NAME_KEYWORD)
PG_KEYWORD("new", NEW, UNRESERVED_KEYWORD)
PG_KEYWORD("next", NEXT, UNRESERVED_KEYWORD)
PG_KEYWORD("no", NO, UNRESERVED_KEYWORD)
+PG_KEYWORD("nodrop", NODROP, UNRESERVED_KEYWORD)
PG_KEYWORD("none", NONE, COL_NAME_KEYWORD)
PG_KEYWORD("not", NOT, RESERVED_KEYWORD)
PG_KEYWORD("nothing", NOTHING, UNRESERVED_KEYWORD)
On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 15/02/17 06:43, Masahiko Sawada wrote:
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.Thank you for the explanation. I understood and agree.
I think we should disallow to call ALTER SUBSCRIPTION inside a user's
transaction block as well.Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever.
Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached
fixed version patch.We should disallow CREATE/DROP SUBSCRIPTION inside a user transaction
block only when CREATE/DROP SLOT option is set?+ /* + * We cannot run CREATE SUBSCRIPTION inside a user transaction + * block. + */ + PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION");I think that more comments about why the command is disallowed inside
a user transaction block are necessary. For example,I agree with you.
----------------------
Disallow CREATE SUBSCRIPTION [CREATE SLOT] inside a user transaction block.When CREATE SLOT is set, this command creates the replication slot on
the remote server. This operation is not transactional. So, if the transaction
is rollbacked, the created replication slot unexpectedly remains while
there is no corresponding entry in pg_subscription. To reduce the possibility
of this problem, we allow CREATE SLOT option only outside a user transaction
block.XXX Note that this restriction cannot completely prevent "orphan" replication
slots. The transaction of CREATE SUBSCRIPTION can still fail after creating
the replication slot on the remote server, though it's basically less likely
to happen.
----------------------+ * We cannot run DROP SUBSCRIPTION inside a user transaction + * block. + */ + PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");Same as above.
While writing regression test for this issue, I found an another bug
of DROP SUBSCRIPTION; DROP SUBSCRIPTION with DROP SLOT option fails to
parse because DROP is a keyword, not IDENT.
Good catch!
Attached 000 patch fixes it
Or we should change the syntax of DROP SUBSCRIPTION as follows, and
handle the options in the same way as the options like "CREATE SLOT" in
CREATE/ALTER SUBSCRIPTION? In CREATE/ALTER commands, the options
are specified with WITH clause. But only DROP command doesn't accept
WITH clause. This looks inconsistent.
----------------------
DROP SUBSCRIPTION [ IF EXISTS ] name [ WITH (option [, ... ]) ]
where option can be:
| DROP SLOT | NODROP SLOT
----------------------
Regards,
--
Fujii Masao
--
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, Feb 22, 2017 at 1:52 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 15/02/17 06:43, Masahiko Sawada wrote:
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.Thank you for the explanation. I understood and agree.
I think we should disallow to call ALTER SUBSCRIPTION inside a user's
transaction block as well.Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever.
Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached
fixed version patch.We should disallow CREATE/DROP SUBSCRIPTION inside a user transaction
block only when CREATE/DROP SLOT option is set?+ /* + * We cannot run CREATE SUBSCRIPTION inside a user transaction + * block. + */ + PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION");I think that more comments about why the command is disallowed inside
a user transaction block are necessary. For example,I agree with you.
----------------------
Disallow CREATE SUBSCRIPTION [CREATE SLOT] inside a user transaction block.When CREATE SLOT is set, this command creates the replication slot on
the remote server. This operation is not transactional. So, if the transaction
is rollbacked, the created replication slot unexpectedly remains while
there is no corresponding entry in pg_subscription. To reduce the possibility
of this problem, we allow CREATE SLOT option only outside a user transaction
block.XXX Note that this restriction cannot completely prevent "orphan" replication
slots. The transaction of CREATE SUBSCRIPTION can still fail after creating
the replication slot on the remote server, though it's basically less likely
to happen.
----------------------+ * We cannot run DROP SUBSCRIPTION inside a user transaction + * block. + */ + PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");Same as above.
While writing regression test for this issue, I found an another bug
of DROP SUBSCRIPTION; DROP SUBSCRIPTION with DROP SLOT option fails to
parse because DROP is a keyword, not IDENT.Good catch!
Attached 000 patch fixes it
Or we should change the syntax of DROP SUBSCRIPTION as follows, and
handle the options in the same way as the options like "CREATE SLOT" in
CREATE/ALTER SUBSCRIPTION? In CREATE/ALTER commands, the options
are specified with WITH clause. But only DROP command doesn't accept
WITH clause. This looks inconsistent.
+1 for adding WITH clause to DROP SUBSCRIPTION option. That way we can
check conflicting or redundant options easier. Will update patches.
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, Feb 22, 2017 at 2:17 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Wed, Feb 22, 2017 at 1:52 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Tue, Feb 21, 2017 at 7:52 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Feb 21, 2017 at 3:42 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Feb 16, 2017 at 11:40 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Thu, Feb 16, 2017 at 7:52 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 15/02/17 06:43, Masahiko Sawada wrote:
On Tue, Feb 14, 2017 at 1:13 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Mon, Feb 13, 2017 at 4:05 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Sat, Feb 11, 2017 at 8:18 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 10/02/17 19:55, Masahiko Sawada wrote:
On Thu, Feb 9, 2017 at 12:44 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:On 08/02/17 07:40, Masahiko Sawada wrote:
On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:For example what happens if apply crashes during the DROP
SUBSCRIPTION/COMMIT and is not started because the delete from catalog
is now visible so the subscription is no longer there?Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
make it emit an error if it's executed within user's transaction block.It seems to me that this is exactly Petr's point: using
PreventTransactionChain() to prevent things to happen.Agreed. It's better to prevent to be executed inside user transaction
block. And I understood there is too many failure scenarios we need to
handle.Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
after removing the entry from pg_subscription, then connect to the publisher
and remove the replication slot.For consistency that may be important.
Agreed.
Attached patch, please give me feedback.
This looks good (and similar to what initial patch had btw). Works fine
for me as well.Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?Hmm, after thought I suspect current discussing approach. For
example, please image the case where CRAETE SUBSCRIPTION creates
subscription successfully but fails to create replication slot for
whatever reason, and then DROP SUBSCRIPTION drops the subscription but
dropping replication slot is failed. In such case, CREAET SUBSCRIPTION
and DROP SUBSCRIPTION return ERROR but the subscription is created and
dropped successfully. I think that this behaviour confuse the user.I think we should just prevent calling DROP SUBSCRIPTION in user's
transaction block. Or I guess that it could be better to separate the
starting/stopping logical replication from subscription management.We need to stop the replication worker(s) in order to be able to drop
the slot. There is no such issue with startup of the worker as that one
is launched by launcher after the transaction has committed.IMO best option is to just don't allow DROP/CREATE SUBSCRIPTION inside a
transaction block and don't do any commits inside of those (so that
there are no rollbacks, which solves your initial issue I believe). That
way failure to create/drop slot will result in subscription not being
created/dropped which is what we want.On second thought, +1.
I basically agree this option, but why do we need to change CREATE
SUBSCRIPTION as well?Because the window between the creation of replication slot and the transaction
commit of CREATE SUBSCRIPTION should be short. Otherwise, if any error happens
during that window, the replication slot unexpectedly remains while there is no
corresponding subscription. Of course, even If we prevent CREATE SUBSCRIPTION
from being executed within user's transaction block, there is still such
window. But we can reduce the possibility of that problem.Thank you for the explanation. I understood and agree.
I think we should disallow to call ALTER SUBSCRIPTION inside a user's
transaction block as well.Why? ALTER SUBSCRIPTION does not create/drop anything on remote server ever.
Hmm you're right. ALTER SUBSCRIPTION can support transaction. Attached
fixed version patch.We should disallow CREATE/DROP SUBSCRIPTION inside a user transaction
block only when CREATE/DROP SLOT option is set?+ /* + * We cannot run CREATE SUBSCRIPTION inside a user transaction + * block. + */ + PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION");I think that more comments about why the command is disallowed inside
a user transaction block are necessary. For example,I agree with you.
----------------------
Disallow CREATE SUBSCRIPTION [CREATE SLOT] inside a user transaction block.When CREATE SLOT is set, this command creates the replication slot on
the remote server. This operation is not transactional. So, if the transaction
is rollbacked, the created replication slot unexpectedly remains while
there is no corresponding entry in pg_subscription. To reduce the possibility
of this problem, we allow CREATE SLOT option only outside a user transaction
block.XXX Note that this restriction cannot completely prevent "orphan" replication
slots. The transaction of CREATE SUBSCRIPTION can still fail after creating
the replication slot on the remote server, though it's basically less likely
to happen.
----------------------+ * We cannot run DROP SUBSCRIPTION inside a user transaction + * block. + */ + PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION");Same as above.
While writing regression test for this issue, I found an another bug
of DROP SUBSCRIPTION; DROP SUBSCRIPTION with DROP SLOT option fails to
parse because DROP is a keyword, not IDENT.Good catch!
Attached 000 patch fixes it
Or we should change the syntax of DROP SUBSCRIPTION as follows, and
handle the options in the same way as the options like "CREATE SLOT" in
CREATE/ALTER SUBSCRIPTION? In CREATE/ALTER commands, the options
are specified with WITH clause. But only DROP command doesn't accept
WITH clause. This looks inconsistent.+1 for adding WITH clause to DROP SUBSCRIPTION option. That way we can
check conflicting or redundant options easier. Will update patches.
Attached updated version patches. Please review these.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
001_disallow_sub_ddls_in_transaction_block_v4.patchapplication/octet-stream; name=001_disallow_sub_ddls_in_transaction_block_v4.patchDownload
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 250806f..406b518 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -47,8 +47,10 @@ CREATE SUBSCRIPTION <replaceable class="PARAMETER">subscription_name</replaceabl
</para>
<para>
- A logical replication worker will be started to replicate data for the new
- subscription at the commit of the transaction where this command is run.
+ <command>CREATE SUBSCRIPTION</command> cannot be executed inside a
+ transaction block when <literal>CREATE SLOT</literal> is specified.
+ A logical replication worker will be started to replicate data for
+ the new subscription at the end of this command.
</para>
<para>
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index c89a016..0828257 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -42,8 +42,8 @@ PostgreSQL documentation
</para>
<para>
- The replication worker associated with the subscription will not stop until
- after the transaction that issued this command has committed.
+ <command>DROP SUBSCRIPTION</command> cannot be executed inside a
+ transaction block when <literal>DROP SLOT</literal> is specified.
</para>
</refsect1>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index c5c8581..bd45acf 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -18,6 +18,7 @@
#include "access/heapam.h"
#include "access/htup_details.h"
+#include "access/xact.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
@@ -227,7 +228,7 @@ publicationListToArray(List *publist)
* Create new subscription.
*/
ObjectAddress
-CreateSubscription(CreateSubscriptionStmt *stmt)
+CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -244,6 +245,24 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
bool create_slot;
List *publications;
+ /*
+ * Parse and check options.
+ * Connection and publication should not be specified here.
+ */
+ parse_subscription_options(stmt->options, NULL, NULL,
+ &enabled_given, &enabled,
+ &create_slot, NULL, &slotname);
+
+ /*
+ * Since creating replication slot is not transactional, it
+ * leaves created replication slot even when the transaction
+ * rollbacks, while there is no corresponding entry in
+ * pg_subscription. So we cannot run CREATE SUBSCRIPTION inside
+ * a user transaction block if creating replication slot.
+ */
+ if (create_slot)
+ PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION CREATE SLOT");
+
if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -262,13 +281,6 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
stmt->subname)));
}
- /*
- * Parse and check options.
- * Connection and publication should not be specified here.
- */
- parse_subscription_options(stmt->options, NULL, NULL,
- &enabled_given, &enabled,
- &create_slot, NULL, &slotname);
if (slotname == NULL)
slotname = stmt->subname;
@@ -447,7 +459,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
* Drop a subscription
*/
void
-DropSubscription(DropSubscriptionStmt *stmt)
+DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
{
Relation rel;
ObjectAddress myself;
@@ -473,6 +485,23 @@ DropSubscription(DropSubscriptionStmt *stmt)
NULL, NULL, NULL, &drop_slot,
NULL);
+ /*
+ * Parse and check options. Other than drop slot option
+ * should not be specified here.
+ */
+ parse_subscription_options(stmt->options, NULL, NULL,
+ NULL, NULL, NULL, &drop_slot,
+ NULL);
+
+ /*
+ * Since dropping replication slot is not transactional, it drops
+ * the replication slot even when the transaction rollbacks.
+ * So we cannot run DROP SUBSCRIPTION inside a user transaction
+ * block if dropping replication slot.
+ */
+ if (drop_slot)
+ PreventTransactionChain(isTopLevel, "DROP SUBSCRIPTION DROP SLOT");
+
rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
tup = SearchSysCache2(SUBSCRIPTIONNAME, MyDatabaseId,
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 3bc0ae5..20b5273 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1609,7 +1609,8 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_CreateSubscriptionStmt:
- address = CreateSubscription((CreateSubscriptionStmt *) parsetree);
+ address = CreateSubscription((CreateSubscriptionStmt *) parsetree,
+ isTopLevel);
break;
case T_AlterSubscriptionStmt:
@@ -1617,7 +1618,7 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_DropSubscriptionStmt:
- DropSubscription((DropSubscriptionStmt *) parsetree);
+ DropSubscription((DropSubscriptionStmt *) parsetree, isTopLevel);
/* no commands stashed for DROP */
commandCollected = true;
break;
diff --git a/src/include/commands/subscriptioncmds.h b/src/include/commands/subscriptioncmds.h
index 127696c..1765879 100644
--- a/src/include/commands/subscriptioncmds.h
+++ b/src/include/commands/subscriptioncmds.h
@@ -18,9 +18,10 @@
#include "catalog/objectaddress.h"
#include "nodes/parsenodes.h"
-extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt);
+extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt,
+ bool isTopLevel);
extern ObjectAddress AlterSubscription(AlterSubscriptionStmt *stmt);
-extern void DropSubscription(DropSubscriptionStmt *stmt);
+extern void DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel);
extern ObjectAddress AlterSubscriptionOwner(const char *name, Oid newOwnerId);
extern void AlterSubscriptionOwner_oid(Oid subid, Oid newOwnerId);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 340180e..9c9dba0 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -14,6 +14,11 @@ ERROR: syntax error at or near "PUBLICATION"
LINE 1: CREATE SUBSCRIPTION testsub PUBLICATION foo;
^
set client_min_messages to error;
+-- fail - cannot CREATE SUBSCRIPTION CREATE SLOT inside transaction block
+BEGIN;
+CREATE SUBSCRIPTION testsub CONNECTION 'testconn' PUBLICATION testpub WITH (CREATE SLOT);
+ERROR: CREATE SUBSCRIPTION CREATE SLOT cannot run inside a transaction block
+COMMIT;
CREATE SUBSCRIPTION testsub CONNECTION 'testconn' PUBLICATION testpub;
ERROR: invalid connection string syntax: missing "=" after "testconn" in connection info string
@@ -61,6 +66,13 @@ ALTER SUBSCRIPTION testsub DISABLE;
(1 row)
COMMIT;
+-- fail - connot DROP SUBSCRIPTION DROP SLOT inside transaction block
+BEGIN;
+DROP SUBSCRIPTION testsub WITH (DROP SLOT);
+ERROR: DROP SUBSCRIPTION DROP SLOT cannot run inside a transaction block
+COMMIT;
+BEGIN;
DROP SUBSCRIPTION testsub WITH (NODROP SLOT);
+COMMIT;
RESET SESSION AUTHORIZATION;
DROP ROLE regress_subscription_user;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index e325b0a..5775398 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -12,6 +12,11 @@ CREATE SUBSCRIPTION testsub CONNECTION 'foo';
CREATE SUBSCRIPTION testsub PUBLICATION foo;
set client_min_messages to error;
+-- fail - cannot CREATE SUBSCRIPTION CREATE SLOT inside transaction block
+BEGIN;
+CREATE SUBSCRIPTION testsub CONNECTION 'testconn' PUBLICATION testpub WITH (CREATE SLOT);
+COMMIT;
+
CREATE SUBSCRIPTION testsub CONNECTION 'testconn' PUBLICATION testpub;
CREATE SUBSCRIPTION testsub CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (DISABLED, NOCREATE SLOT);
reset client_min_messages;
@@ -38,7 +43,14 @@ ALTER SUBSCRIPTION testsub DISABLE;
COMMIT;
+-- fail - connot DROP SUBSCRIPTION DROP SLOT inside transaction block
+BEGIN;
+DROP SUBSCRIPTION testsub WITH (DROP SLOT);
+COMMIT;
+
+BEGIN;
DROP SUBSCRIPTION testsub WITH (NODROP SLOT);
+COMMIT;
RESET SESSION AUTHORIZATION;
DROP ROLE regress_subscription_user;
000_change_drop_sub_option_syntax.patchapplication/octet-stream; name=000_change_drop_sub_option_syntax.patchDownload
diff --git a/doc/src/sgml/ref/drop_subscription.sgml b/doc/src/sgml/ref/drop_subscription.sgml
index 9f2fb93..c89a016 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -21,7 +21,11 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-DROP SUBSCRIPTION [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [ <replaceable class="parameter">DROP SLOT</replaceable> | <replaceable class="parameter">NODROP SLOT</replaceable> ]
+ DROP SUBSCRIPTION [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [ WITH ( <replaceable class="PARAMETER">option</replaceable> [, ...] ) ]
+
+<phrase> where <replaceable class="PARAMETER">option</replaceable> can be:</phrase>
+
+ | DROP SLOT | NODROP SLOT
</synopsis>
</refsynopsisdiv>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 0e08138..c5c8581 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -41,7 +41,8 @@
#include "utils/syscache.h"
/*
- * Common option parsing function for CREATE and ALTER SUBSCRIPTION commands.
+ * Common option parsing function for CREATE, DROP and ALTER SUBSCRIPTION
+ * commands.
*
* Since not all options can be specified in both commands, this function
* will report an error on options if the target output pointer is NULL to
@@ -50,10 +51,12 @@
static void
parse_subscription_options(List *options, char **conninfo,
List **publications, bool *enabled_given,
- bool *enabled, bool *create_slot, char **slot_name)
+ bool *enabled, bool *create_slot, bool *drop_slot,
+ char **slot_name)
{
ListCell *lc;
bool create_slot_given = false;
+ bool drop_slot_given = false;
if (conninfo)
*conninfo = NULL;
@@ -66,6 +69,8 @@ parse_subscription_options(List *options, char **conninfo,
}
if (create_slot)
*create_slot = true;
+ if (drop_slot)
+ *drop_slot = true;
if (slot_name)
*slot_name = NULL;
@@ -132,6 +137,24 @@ parse_subscription_options(List *options, char **conninfo,
create_slot_given = true;
*create_slot = !defGetBoolean(defel);
}
+ else if (strcmp(defel->defname, "drop slot") == 0 && drop_slot)
+ {
+ if (drop_slot_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options")));
+ drop_slot_given = true;
+ *drop_slot = defGetBoolean(defel);
+ }
+ else if (strcmp(defel->defname, "nodrop slot") == 0 && drop_slot)
+ {
+ if (drop_slot_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options")));
+ drop_slot_given = true;
+ *drop_slot = !defGetBoolean(defel);
+ }
else if (strcmp(defel->defname, "slot name") == 0 && slot_name)
{
if (*slot_name)
@@ -245,7 +268,7 @@ CreateSubscription(CreateSubscriptionStmt *stmt)
*/
parse_subscription_options(stmt->options, NULL, NULL,
&enabled_given, &enabled,
- &create_slot, &slotname);
+ &create_slot, NULL, &slotname);
if (slotname == NULL)
slotname = stmt->subname;
@@ -372,7 +395,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
/* Parse options. */
parse_subscription_options(stmt->options, &conninfo, &publications,
&enabled_given, &enabled,
- NULL, &slot_name);
+ NULL, NULL, &slot_name);
/* Form a new tuple. */
memset(values, 0, sizeof(values));
@@ -440,6 +463,15 @@ DropSubscription(DropSubscriptionStmt *stmt)
RepOriginId originid;
WalReceiverConn *wrconn = NULL;
StringInfoData cmd;
+ bool drop_slot;
+
+ /*
+ * Parse and check options. Other than drop slot option
+ * should not be specified here.
+ */
+ parse_subscription_options(stmt->options, NULL, NULL,
+ NULL, NULL, NULL, &drop_slot,
+ NULL);
rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
@@ -523,7 +555,7 @@ DropSubscription(DropSubscriptionStmt *stmt)
replorigin_drop(originid);
/* If the user asked to not drop the slot, we are done mow.*/
- if (!stmt->drop_slot)
+ if (!drop_slot)
{
heap_close(rel, NoLock);
return;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 05d8538..b5396b1 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4345,7 +4345,7 @@ _copyDropSubscriptionStmt(const DropSubscriptionStmt *from)
DropSubscriptionStmt *newnode = makeNode(DropSubscriptionStmt);
COPY_STRING_FIELD(subname);
- COPY_SCALAR_FIELD(drop_slot);
+ COPY_NODE_FIELD(options);
COPY_SCALAR_FIELD(missing_ok);
return newnode;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index d595cd7..f99fab1 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2188,7 +2188,7 @@ _equalDropSubscriptionStmt(const DropSubscriptionStmt *a,
const DropSubscriptionStmt *b)
{
COMPARE_STRING_FIELD(subname);
- COMPARE_SCALAR_FIELD(drop_slot);
+ COMPARE_NODE_FIELD(options);
COMPARE_SCALAR_FIELD(missing_ok);
return true;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 6c6d21b..7804eee 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -413,7 +413,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <fun_param_mode> arg_class
%type <typnam> func_return func_type
-%type <boolean> opt_trusted opt_restart_seqs opt_drop_slot
+%type <boolean> opt_trusted opt_restart_seqs
%type <ival> OptTemp
%type <ival> OptNoLog
%type <oncommit> OnCommitOption
@@ -9165,44 +9165,28 @@ AlterSubscriptionStmt:
/*****************************************************************************
*
- * DROP SUBSCRIPTION [ IF EXISTS ] name
+ * DROP SUBSCRIPTION [ IF EXISTS ] name [ WITH (options) ]
*
*****************************************************************************/
-DropSubscriptionStmt: DROP SUBSCRIPTION name opt_drop_slot
+DropSubscriptionStmt: DROP SUBSCRIPTION name opt_definition
{
DropSubscriptionStmt *n = makeNode(DropSubscriptionStmt);
n->subname = $3;
- n->drop_slot = $4;
+ n->options = $4;
n->missing_ok = false;
$$ = (Node *) n;
}
- | DROP SUBSCRIPTION IF_P EXISTS name opt_drop_slot
+ | DROP SUBSCRIPTION IF_P EXISTS name opt_definition
{
DropSubscriptionStmt *n = makeNode(DropSubscriptionStmt);
n->subname = $5;
- n->drop_slot = $6;
+ n->options = $6;
n->missing_ok = true;
$$ = (Node *) n;
}
;
-opt_drop_slot:
- IDENT SLOT
- {
- if (strcmp($1, "drop") == 0)
- $$ = TRUE;
- else if (strcmp($1, "nodrop") == 0)
- $$ = FALSE;
- else
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("unrecognized option \"%s\"", $1),
- parser_errposition(@1)));
- }
- | /*EMPTY*/ { $$ = TRUE; }
- ;
-
/*****************************************************************************
*
* QUERY: Define Rewrite Rule
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 5afc3eb..8ef61fa 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3296,7 +3296,7 @@ typedef struct DropSubscriptionStmt
{
NodeTag type;
char *subname; /* Name of of the subscription */
- bool drop_slot; /* Should we drop the slot on remote side? */
+ List *options; /* List of DefElem nodes */
bool missing_ok; /* Skip error if missing? */
} DropSubscriptionStmt;
diff --git a/src/test/regress/expected/object_address.out b/src/test/regress/expected/object_address.out
index ec5ada9..de0c86d 100644
--- a/src/test/regress/expected/object_address.out
+++ b/src/test/regress/expected/object_address.out
@@ -464,7 +464,7 @@ SELECT (pg_identify_object(addr1.classid, addr1.objid, addr1.subobjid)).*,
SET client_min_messages TO 'warning';
DROP FOREIGN DATA WRAPPER addr_fdw CASCADE;
DROP PUBLICATION addr_pub;
-DROP SUBSCRIPTION addr_sub NODROP SLOT;
+DROP SUBSCRIPTION addr_sub WITH (NODROP SLOT);
DROP SCHEMA addr_nsp CASCADE;
DROP OWNED BY regress_addr_user;
DROP USER regress_addr_user;
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 2ccec98..340180e 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -61,6 +61,6 @@ ALTER SUBSCRIPTION testsub DISABLE;
(1 row)
COMMIT;
-DROP SUBSCRIPTION testsub NODROP SLOT;
+DROP SUBSCRIPTION testsub WITH (NODROP SLOT);
RESET SESSION AUTHORIZATION;
DROP ROLE regress_subscription_user;
diff --git a/src/test/regress/sql/object_address.sql b/src/test/regress/sql/object_address.sql
index e658ea3..06cf337 100644
--- a/src/test/regress/sql/object_address.sql
+++ b/src/test/regress/sql/object_address.sql
@@ -197,7 +197,7 @@ SET client_min_messages TO 'warning';
DROP FOREIGN DATA WRAPPER addr_fdw CASCADE;
DROP PUBLICATION addr_pub;
-DROP SUBSCRIPTION addr_sub NODROP SLOT;
+DROP SUBSCRIPTION addr_sub WITH (NODROP SLOT);
DROP SCHEMA addr_nsp CASCADE;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 68c17d5..e325b0a 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -38,7 +38,7 @@ ALTER SUBSCRIPTION testsub DISABLE;
COMMIT;
-DROP SUBSCRIPTION testsub NODROP SLOT;
+DROP SUBSCRIPTION testsub WITH (NODROP SLOT);
RESET SESSION AUTHORIZATION;
DROP ROLE regress_subscription_user;
On 23/02/17 08:24, Masahiko Sawada wrote:
Attached updated version patches. Please review these.
This version looks good to me, I'd only change the
+ PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION CREATE SLOT");
to "CREATE SUBSCRIPTION ... CREATE SLOT" as that's afaik how we do it
for other commands (and same with DROP).
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 3/3/17 13:58, Petr Jelinek wrote:
On 23/02/17 08:24, Masahiko Sawada wrote:
Attached updated version patches. Please review these.
This version looks good to me, I'd only change the
+ PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION CREATE SLOT");
to "CREATE SUBSCRIPTION ... CREATE SLOT" as that's afaik how we do it
for other commands (and same with DROP).
I have committed fixes for these issues.
I didn't like the syntax change in DROP SUBSCRIPTION, so I have just
fixed the parsing of the existing syntax. We can discuss syntax changes
separately. The second patch I have committed after some editing. I
think it was generated on top of the existing data copy patch, so it was
a bit of a mess.
--
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, Mar 4, 2017 at 1:46 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 3/3/17 13:58, Petr Jelinek wrote:
On 23/02/17 08:24, Masahiko Sawada wrote:
Attached updated version patches. Please review these.
This version looks good to me, I'd only change the
+ PreventTransactionChain(isTopLevel, "CREATE SUBSCRIPTION CREATE SLOT");
to "CREATE SUBSCRIPTION ... CREATE SLOT" as that's afaik how we do it
for other commands (and same with DROP).I have committed fixes for these issues.
Thanks!
I didn't like the syntax change in DROP SUBSCRIPTION, so I have just
fixed the parsing of the existing syntax. We can discuss syntax changes
separately.
Understood.
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