Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility.

On Thu, Dec 12, 2024 at 6:04 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> Here are some review comments for the patch v4-0001.
>
> ======
> GENERAL.
>
> 1.
> After reading Vignesh's last review and then the pg_createsubscriber
> documentation I see there can be multiple databases simultaneously
> specified (by writing multiple -d switches) and in that case each one
> gets its own:
> --publication
> --replication-slot
> --subscription
>
> OTOH, this new '--enable-two-phase' switch is just a blanket two_phase
> enablement across all subscriptions. There is no way for the user to
> say if they want it enabled for some subscriptions (on some databases)
> but not for others. I suppose this was intentional and OK (right?),
> but this point needs to be clarified in the docs.
>

Fixed.

> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 2.
> + <para>
> + Enables <link
> linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> + commit for the subscription. The default is <literal>false</literal>.
> + </para>
>
> Following on from the previous review comment. Since this switch is
> applied to all database subscriptions I think the help needs to say
> that. Something like.
>
> "If there are multiple subscriptions specified, this option applies to
> all of them."
>
> ~~~
>

Fixed.

> 3.
> In the "Prerequisites" sections of the docs, it gives requirements for
> various GUC settings on the source server and the target server. So,
> should there be another note here advising to configure the
> 'max_prepared_transactions' GUC when the '--enable-two-phase' is
> specified?
>
> ~~~
>

Fixed.

> 4. "Warnings" section includes the following:
>
> pg_createsubscriber sets up logical replication with two-phase commit
> disabled. This means that any prepared transactions will be replicated
> at the time of COMMIT PREPARED, without advance preparation. Once
> setup is complete, you can manually drop and re-create the
> subscription(s) with the two_phase option enabled.
>
> ~
>
> The above text is from the "Warnings" section, but it seems stale
> information that needs to be updated due to the introduction of this
> new '--enable-two-phase' option.
>

Fixed.

> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> usage:
> 5.
> printf(_(" -t, --recovery-timeout=SECS seconds to wait for
> recovery to end\n"));
> + printf(_(" -T, --enable-two-phase enable two-phase commit
> for the subscription\n"));
>
> Given the previous review comments (#1/#2 etc), I was wondering if it
> might be better to say more like "enable two-phase commit for all
> subscriptions".
>

Fixed.

> ======
> .../t/040_pg_createsubscriber.pl
>
> 6.
> +is($count_prepared_s, qq(1), 'Prepared transaction replicated to subscriber');
>
> Should there also have been an earlier check (way back before the
> PREPARE) just to make sure this count was initially 0?
>

Removed this and added a new test case instead of this.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.