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

Dear Peter,

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

Added.

> ======
> 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.

I checked contents and changed to "on|off".

> ======
> 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.

Made the namespace narrower and initialization was removed.

> ~~~
>
> 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).

Modified.

> ~~~
>
> 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?

Changed.

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

Yeah, fixed.

> ======
> 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

They were fixed based on your previous comments.

>
> 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

Modified, but it was included in 0001.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/