Subject: Re: Slow catchup of 2PC (twophase) transactions on replica in LR

From: Peter Smith <[email-masked]>
Date: Wed, 17 Jul 2024 14:28:20 +1000

Hi, here is my review of the v18-0003 patch.

======
sgml/ref/alter_subscription.sgml

nitpick - some minor tweaks to the documentation text. I also added a link
back to the two_phase parameter. Please see the attached diffs file.

======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index e021f87..58db97f 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -256,14 +256,15 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
      </para>
 
      <para>
-      The <literal>two_phase</literal> parameter can only be altered when the
-      subscription is disabled. When altering the parameter from <literal>true</literal>
+      The <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
+      parameter can only be altered when the subscription is disabled.
+      When altering the parameter from <literal>true</literal>
       to <literal>false</literal>, the backend process checks for any incomplete
       prepared transactions done by the logical replication worker (from when
       <literal>two_phase</literal> parameter was still <literal>true</literal>)
-      and, if any are found, this command is failed with an error. In this
-      case, you can resolve prepared transactions on the publisher node or
-      manually roll back them on the subscriber. After the altering from
+      and, if any are found, an error is reported. If this happens, you can
+      resolve prepared transactions on the publisher node or
+      manually roll back them on the subscriber, then try again. After the altering from
       <literal>true</literal> to <literal>false</literal>, the publisher will
       replicate transactions again when they are committed.
      </para>

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 17 Jul 2024 05:13:35 +0000

Dear Hou, Peter,

Thanks for giving comments! PSA new version.
Almost comments were addressed.
What's new:
0001 - IsTwoPhaseTransactionGidForSubid() was updated per comment from Hou-san [1].
          Some nitpicks were accepted.
0002 - An argument in CheckAlterSubOption() was renamed to " slot_needs_update "
          Some nitpicks were accepted.
0003 - Some nitpicks were accepted.

Below part contains the reason why I rejected some comments.

> CommonChecksForFailoverAndTwophase:
> nitpick - added Assert for the generic-looking "option" parameter name

The style looks strange for me, using multiple strcmp() is more straightforward.
Added like that.

> 1c.
> If the error checks can be moved to be done up-front, then all the 'needs_update'
> can be combined. Avoiding multiple checks to 'needs_update' will make this function simpler.

This style was needed to preserve error condition for failover option. Not changed.

[1]: https://www.postgresql.org/message-id/OS3PR01MB571834FBD3E6D3804484038F94A32%40OS3PR01MB5718.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Peter Smith <[email-masked]>
Date: Wed, 17 Jul 2024 17:53:28 +1000

Hi, here are my review comments for v19-0001.

======
doc/src/sgml/protocol.sgml

nitpick - Now there is >1 option. /The following option is supported:/The
following options are supported:/

======
src/backend/access/transam/twophase.c

TwoPhaseTransactionGid:
nitpick - renamed parameter /gid/gid_res/ to emphasize that this is
returned by reference

~~~

1.
IsTwoPhaseTransactionGidForSubid
+ /* Construct the format GID based on the got xid */
+ TwoPhaseTransactionGid(subid, xid, gid_generated, sizeof(gid));

I think the wrong size is being passed here. It should be the buffer size
-- e.g. sizeof(gid_generated).

~

nitpick - renamed a couple of vars for readability
nitpick - expanded some comments.

======
src/backend/commands/subscriptioncmds.c

2. AlterSubscription
+ /*
+ * slot_name and two_phase cannot be altered
+ * simultaneously. The latter part refers to the pre-set
+ * slot name and tries to modify the slot option, so
+ * changing both does not make sense.
+ */

I had given a v18-0002 nitpick suggestion to re-word all of this comment.
But, as I wrote before [1], that fix belongs here in patch 0001 where the
comment was first added.

======
[1]
https://www.postgresql.org/message-id/CAHut%2BPsqMRS3dcijo5jsTSbgV1-9So-YBC7PH7xg0%2BZ8hA7fDQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index cba6661..24c57fb 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2192,7 +2192,7 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
        </varlistentry>
       </variablelist>
 
-      <para>The following option is supported:</para>
+      <para>The following options are supported:</para>
 
       <variablelist>
        <varlistentry>
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index f710030..b426f0b 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2689,7 +2689,7 @@ LookupGXact(const char *gid, XLogRecPtr prepare_end_lsn,
  * Return the GID in the supplied buffer.
  */
 void
-TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid, int szgid)
+TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid_res, int szgid)
 {
 	Assert(subid != InvalidRepOriginId);
 
@@ -2698,7 +2698,7 @@ TwoPhaseTransactionGid(Oid subid, TransactionId xid, char *gid, int szgid)
 				(errcode(ERRCODE_PROTOCOL_VIOLATION),
 				 errmsg_internal("invalid two-phase transaction ID")));
 
-	snprintf(gid, szgid, "pg_gid_%u_%u", subid, xid);
+	snprintf(gid_res, szgid, "pg_gid_%u_%u", subid, xid);
 }
 
 /*
@@ -2710,20 +2710,26 @@ static bool
 IsTwoPhaseTransactionGidForSubid(Oid subid, char *gid)
 {
 	int			ret;
-	Oid			subid_written;
-	TransactionId xid;
+	Oid			subid_from_gid;
+	TransactionId xid_from_gid;
 	char		gid_generated[GIDSIZE];
 
-	ret = sscanf(gid, "pg_gid_%u_%u", &subid_written, &xid);
+	/* Extract the subid and xid from the given GID. */
+	ret = sscanf(gid, "pg_gid_%u_%u", &subid_from_gid, &xid_from_gid);
 
-	/* Return false if the given GID has different format */
-	if (ret != 2 || subid != subid_written)
+	/*
+	 * Check that the given GID has expected format, and at least the subid
+	 * matches.
+	 */
+	if (ret != 2 || subid != subid_from_gid)
 		return false;
 
-	/* Construct the format GID based on the got xid */
-	TwoPhaseTransactionGid(subid, xid, gid_generated, sizeof(gid));
-
-	/* ...And check whether the given GID is exact same as the format GID */
+	/*
+	 * Reconstruct a tmp GID based on the subid and xid extracted from
+	 * the given GID. Check that the tmp GID and the given GID match.
+	 */
+	TwoPhaseTransactionGid(subid_from_gid, xid_from_gid,
+						   gid_generated, sizeof(gid_generated));
 	return strcmp(gid, gid_generated) == 0;
 }

---

From: Peter Smith <[email-masked]>
Date: Thu, 18 Jul 2024 09:24:29 +1000

Hi, here are my review comments for patch v19-0002.

======

CheckAlterSubOption:
nitpick - tweak some comment wording

~

> This style was needed to preserve error condition for failover option. Not changed.

nitpick - Hmm. I think you might be trying to preserve the ordering of
errors when that order is of no consequence. AFAICT which error comes
first here is neither documented nor regression tested. e.g.
reordering them gives no problem for testing, but OTOH reordering them
does simplify the code. Anyway, I have modified the code in my
attached nitpicks diffs to demonstrate this suggestion in case you
change your mind.

~~~

AlterSubscription:
nitpick - let's keep all the variables called 'update_xxx' together.
nitpick - comment typo /needs/need/
nitpick - tweak some comment wording

======
src/test/subscription/t/021_twophase.pl

nitpick - didn't quite understand the "Since we are..." comment. I
reworded it according to what I thought was the intention.

======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f770e9c..a826be9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1079,17 +1079,6 @@ CheckAlterSubOption(Subscription *sub, const char *option,
 		   strcmp(option, "two_phase") == 0);
 
 	/*
-	 * If the slot needs to be updated, the backend must connect to the
-	 * publisher and request the alteration. slot_name must be required at that
-	 * time.
-	 */
-	if (slot_needs_update && !sub->slotname)
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("cannot set %s for a subscription that does not have a slot name",
-						option)));
-
-	/*
 	 * Do not allow changing the option if the subscription is enabled. This
 	 * is because both failover and two_phase options of the slot on the
 	 * publisher cannot be modified if the slot is currently acquired by the
@@ -1101,17 +1090,27 @@ CheckAlterSubOption(Subscription *sub, const char *option,
 				 errmsg("cannot set %s for enabled subscription",
 						option)));
 
-	/*
-	 * The changed option of the slot can't be rolled back, so disallow if we
-	 * are in a transaction block.
-	 */
 	if (slot_needs_update)
 	{
 		StringInfoData cmd;
 
+		/*
+		 * If the slot needs to be updated, the backend must connect to the
+		 * publisher and request the alteration. A slot_name is required at
+		 * that time.
+		 */
+		if (!sub->slotname)
+			ereport(ERROR,
+					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+					 errmsg("cannot set %s for a subscription that does not have a slot name",
+							option)));
+
+		/*
+		 * The changed option of the slot can't be rolled back, so disallow if we
+		 * are in a transaction block.
+		 */
 		initStringInfo(&cmd);
 		appendStringInfo(&cmd, "ALTER SUBSCRIPTION ... SET (%s)", option);
-
 		PreventInTransactionBlock(isTopLevel, cmd.data);
 		pfree(cmd.data);
 	}
@@ -1132,12 +1131,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	HeapTuple	tup;
 	Oid			subid;
 	bool		update_tuple = false;
+	bool		update_failover = false;
+	bool		update_two_phase = false;
 	Subscription *sub;
 	Form_pg_subscription form;
 	bits32		supported_opts;
 	SubOpts		opts = {0};
-	bool		update_failover = false;
-	bool		update_two_phase = false;
 
 	rel = table_open(SubscriptionRelationId, RowExclusiveLock);
 
@@ -1271,7 +1270,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				if (IsSet(opts.specified_opts, SUBOPT_FAILOVER))
 				{
 					/*
-					 * First, mark the needs to alter the replication slot.
+					 * First, mark the need to alter the replication slot.
 					 * Failover option is controlled by both the publisher (as
 					 * a slot option) and the subscriber (as a subscription
 					 * option).
@@ -1295,15 +1294,14 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					 * subscription option). The slot option must be altered
 					 * only when changing "true" to "false".
 					 *
-					 * There is no need to do something remarkable regarding
+					 * There is no need to do anything remarkable for
 					 * the "false" to "true" case; the backend process alters
 					 * subtwophase to LOGICALREP_TWOPHASE_STATE_PENDING once.
 					 * After the subscription is enabled, a new logical
 					 * replication worker requests to change the two_phase
 					 * option of its slot from pending to true when the
-					 * initial data synchronization is done. The code path is
-					 * the same as the case in which two_phase is initially
-					 * set to true.
+					 * initial data synchronization is done. That code path is
+					 * the same as when two_phase is initially set to true.
 					 */
 					update_two_phase = !opts.twophase;
 
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index c8fb3b7..91e2e17 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -428,9 +428,8 @@ is($result, qq(0), 'should be no prepared transactions on subscriber');
 
 ###############################
 # Toggle the two_phase to "true" before the COMMIT PREPARED.
-#
-# Since we are the special path for the case where both two_phase
-# and failover are altered, it is also set to "true".
+# Also, set failover to "true" to test the code path where
+# both two_phase and failover are altered at the same time.
 ###############################
 $node_subscriber->safe_psql('postgres',
     "ALTER SUBSCRIPTION tap_sub_copy DISABLE;");