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

[Peter Smith]

On Thu, Jul 18, 2024 at 9:42 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
...
> I agree and have done that in the attached. I have made some
> additional changes: (a) removed the unrelated change of two_phase in
> protocol.sgml, (b) tried to make the two_phase change before failover
> option wherever it makes sense to keep the code consistent, (c)
> changed/added comments and doc changes at various places.
>
> I'll continue my review and testing of the patch but I thought of
> sharing what I have done till now.
>

Here some minor comments for patch v21

======
You wrote "tried to make the two_phase change before failover option
wherever it makes sense to keep the code consistent". But, still
failover is coded first in lots of places:
- libpqrcv_alter_slot
- ReplicationSlotAlter
- AlterReplicationSlot
etc.

Q. Why not change those ones?

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

IsTwoPhaseTransactionGidForSubid:
nitpick - nicer to rename the temporary gid variable: /gid_generated/gid_tmp/

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

CheckAlterSubOption:
nitpick = function comment period/plural.
nitpick - typo /Samilar/Similar/

======
src/include/replication/slot.h

1.
-extern void ReplicationSlotAlter(const char *name, bool failover);
+extern void ReplicationSlotAlter(const char *name, bool *failover,
+ bool *two_phase);

Use const?

======
99.
Please see attached diffs implementing the nitpicks mentioned above

======
Kind Regards,
Peter Smith.
Fujitsu Australia

diff --git a/src/backend/access/transam/twophase.c 
b/src/backend/access/transam/twophase.c
index 90df32f..da97836 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2712,7 +2712,7 @@ IsTwoPhaseTransactionGidForSubid(Oid subid, char *gid)
        int                     ret;
        Oid                     subid_from_gid;
        TransactionId xid_from_gid;
-       char            gid_generated[GIDSIZE];
+       char            gid_tmp[GIDSIZE];
 
        /* Extract the subid and xid from the given GID */
        ret = sscanf(gid, "pg_gid_%u_%u", &subid_from_gid, &xid_from_gid);
@@ -2729,10 +2729,9 @@ IsTwoPhaseTransactionGidForSubid(Oid subid, char *gid)
         * the given GID and check whether the temporary GID and the given GID
         * match.
         */
-       TwoPhaseTransactionGid(subid, xid_from_gid, gid_generated,
-                                                  sizeof(gid_generated));
+       TwoPhaseTransactionGid(subid, xid_from_gid, gid_tmp, sizeof(gid_tmp));
 
-       return strcmp(gid, gid_generated) == 0;
+       return strcmp(gid, gid_tmp) == 0;
 }
 
 /*
diff --git a/src/backend/commands/subscriptioncmds.c 
b/src/backend/commands/subscriptioncmds.c
index 2e9f329..5f11235 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1071,7 +1071,7 @@ AlterSubscription_refresh(Subscription *sub, bool 
copy_data,
 }
 
 /*
- * Common checks for altering failover and two_phase option
+ * Common checks for altering failover and two_phase options.
  */
 static void
 CheckAlterSubOption(Subscription *sub, const char *option,
@@ -1337,8 +1337,8 @@ AlterSubscription(ParseState *pstate, 
AlterSubscriptionStmt *stmt,
                                if (IsSet(opts.specified_opts, SUBOPT_FAILOVER))
                                {
                                        /*
-                                        * Samilar to two_phase, we need to 
update the failover
-                                        * option for the slot and the 
subscription.
+                                        * Similar to the two_phase case above, 
we need to update
+                                        * the failover option for the slot and 
the subscription.
                                         */
                                        update_failover = true;