Re: Slow catchup of 2PC (twophase) transactions on replica in LR - Mailing list pgsql-hackers

Hi, Here are some review comments for the patch v7-0003.

======
Commit Message

1.
The patch needs a commit message to describe the purpose and highlight
any limitations and other details.

======
doc/src/sgml/ref/alter_subscription.sgml

2.
+
+     <para>
+      The <literal>two_phase</literal> 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 prepared
+      transactions done by the logical replication worker and aborts them.
+     </para>

Here, the para is referring to "true" and "false" but earlier on this
page it talks about "twophase = off". IMO it is better to use a
consistent terminology like "on|off" everywhere instead of randomly
changing the way it is described each time.

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

3. AlterSubscription

  if (IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT))
  {
+ List *prepared_xacts = NIL;

This 'prepared_xacts' can be declared at a lower scrope because it is
only used if (!opts.twophase).

Furthermore, IIUC you don't need to assign NIL in the declaration
because there is no chance for it to be unassigned anyway.

~~~

4. AlterSubscription

+ /*
+ * The changed two_phase option (true->false) of the
+ * slot can't be rolled back.
+ */
  PreventInTransactionBlock(isTopLevel,
    "ALTER SUBSCRIPTION ... SET (two_phase = off)");

Here is another example of inconsistent mixing of the terminology
where the comment says "true"/"false" but the message says "off".
Let's keep everything consistent. (I prefer on|off).

~~~

5.
+ if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_ENABLED &&
+ (prepared_xacts = GetGidListBySubid(subid)) != NIL)
+ {
+ ListCell *cell;
+
+ /* Abort all listed transactions */
+ foreach(cell, prepared_xacts)
+ FinishPreparedTransaction((char *) lfirst(cell),
+   false);
+
+ list_free(prepared_xacts);
+ }

5A.
IIRC there is a cleaner way to write this loop without needing
ListCell variable -- e.g. foreach_ptr() macro?

~

5B.
Shouldn't this be using list_free_deep() so the pstrdup gid gets freed too?

======
src/test/subscription/t/099_twophase_added.pl

6.
+######
+# Check the case that prepared transactions exist on subscriber node
+######
+

Give some more detailed comments here similar to the review comment of
patch v7-0002 for the other part of this TAP test.

~~~

7. TAP test - comments

Same as for my v7-0002 review comments, I think this test case also
needs a few more one-line comments to describe the sub-steps. e.g.:

# prepare a transaction to insert some rows to the table

# verify the prepared tx is replicated to the subscriber (because
'two_phase = on')

# toggle the two_phase to 'off' *before* the COMMIT PREPARED

# verify the prepared tx got aborted

# do the COMMIT PREPARED (note that now two_phase is 'off')

# verify the inserted rows got replicated ok

~~~

8. TAP test - subscription name

It's better to rename the SUBSCRIPTION in this TAP test so you can
avoid getting log warnings like:

psql:<stdin>:4: WARNING:  subscriptions created by regression test
cases should have names starting with "regress_"
psql:<stdin>:4: NOTICE:  created replication slot "sub" on publisher

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