On Tue, Apr 1, 2025 at 4:28 PM Zhijie Hou (Fujitsu)
<houzj.f...@fujitsu.com> wrote:
>
> Here is the V3 patch set which addressed all the comments.
>
Comment 0n 0001
<literal>NULL</literal> for logical slots where
+ <structfield>two_phase</structfield> is false and physical slots.
+ </para></entry>
change above to:
<literal>NULL</literal> for logical slots where
<structfield>two_phase</structfield> is false and for physical slots.
Comment on 0002
+# Create a subscription with two_phase enabled
+$subscriber1->safe_psql('postgres',
+ "CREATE SUBSCRIPTION regress_mysub2 CONNECTION '$publisher_connstr'
PUBLICATION regress_mypub WITH (slot_name = lsub1_slot, create_slot =
false, enabled = false, two_phase = true);"
+);
+
+# Enable failover for the subscription
+($result, $stdout, $stderr) = $subscriber1->psql('postgres',
+ "ALTER SUBSCRIPTION regress_mysub2 SET (failover = true)");
+ok( $stderr =~ /ERROR: cannot enable failover for a two_phase
enabled subscription/,
+ "Enabling failover is not allowed for a two_phase enabled subscription");
Is there a need for this test to be in .pl file? Can't we add it in .sql file?
Apart from the above, I have made minor modifications to the PG17
patch in the attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 6db3c9347fa..09c5f0ef685 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -649,10 +649,11 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
errhint("Subscriptions with the
password_required option set to false may only be created or modified by the
superuser.")));
/*
- * Do not allow users to enable failover and two_phase option together.
+ * Do not allow users to enable the failover and two_phase options
+ * together.
*
- * See comments atop the similar check in ReplicationSlotCreate() for
- * detailed reasons.
+ * See comments atop the similar check in ReplicationSlotCreate() for a
+ * detailed reason.
*/
if (opts.twophase && opts.failover)
ereport(ERROR,
@@ -1258,11 +1259,11 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
"failover")));
/*
- * Do not allow users to enable
failover and two_phase
- * option together.
+ * Do not allow users to enable the
failover and two_phase
+ * options together.
*
* See comments atop the similar check
in
- * ReplicationSlotCreate() for detailed
reasons.
+ * ReplicationSlotCreate() for a
detailed reason.
*/
if (sub->twophasestate !=
LOGICALREP_TWOPHASE_STATE_DISABLED &&
opts.failover)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index f40028bd941..d251ce95fe6 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -345,14 +345,14 @@ ReplicationSlotCreate(const char *name, bool db_specific,
errmsg("cannot enable failover for a
temporary replication slot"));
/*
- * Do not allow users to enable failover for slots that enable
- * two-phase decoding.
+ * Do not allow users to enable both failover and two_phase for
slots.
*
* This is because the two_phase_at field of a slot, which
tracks the
- * LSN from which two-phase decoding starts, is not
synchronized to
- * standby servers. Without this field, the logical decoding
might
+ * LSN, from which two-phase decoding starts, is not
synchronized to
+ * standby servers. Without two_phase_at, the logical decoding
might
* incorrectly identify prepared transaction as already
replicated to
- * the subscriber, causing them to be skipped.
+ * the subscriber after promotion of standby server, causing
them to be
+ * skipped.
*/
if (two_phase)
ereport(ERROR,
@@ -865,11 +865,10 @@ ReplicationSlotAlter(const char *name, bool failover)
errmsg("cannot enable failover for a temporary
replication slot"));
/*
- * Do not allow users to enable failover for slots that enable two-phase
- * decoding.
+ * Do not allow users to enable failover for a two_phase enabled slot.
*
- * See comments atop the similar check in ReplicationSlotCreate() for
- * detailed reasons.
+ * See comments atop the similar check in ReplicationSlotCreate() for a
+ * detailed reason.
*/
if (failover && MyReplicationSlot->data.two_phase)
ereport(ERROR,
