On Thu, Dec 12, 2024 at 6:04 AM Peter Smith <smithpb2...@gmail.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.
v5-0001-Add-support-for-two-phase-commit-in-pg_createsubs.patch
Description: Binary data
