On Fri, Dec 13, 2024 at 4:42 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Hi Shubham,
>
> Here are my review comments for the patch v5-0001.
>
> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 1.
> - must accept local connections.
> + must accept local connections. If you are planning to use the
> + --enable-two-phase switch then you will also need to set the
> + <xref linkend="guc-max-prepared-transactions"/> appropriately.
>
> Should use sgml <option> markup for "--enable-two-phase".
>
> ~~~
>
> 2.
> - <application>pg_createsubscriber</application> sets up logical
> - replication with two-phase commit disabled. This means that any
> - prepared transactions will be replicated at the time
> - of <command>COMMIT PREPARED</command>, without advance preparation.
> - Once setup is complete, you can manually drop and re-create the
> - subscription(s) with
> + If --enable-two-phase switch is not specified, the
> + <application>pg_createsubscriber</application> sets up logical
> replication
> + with two-phase commit disabled. This means that any prepared
> transactions
> + will be replicated at the time of <command>COMMIT PREPARED</command>,
> + without advance preparation. Once setup is complete, you can manually
> drop
> + and re-create the subscription(s) with
>
> Should use sgml <option> markup for "--enable-two-phase".
>
> ======
> .../t/040_pg_createsubscriber.pl
>
> 3.
> +# Verify that the subtwophase is 'p' in the pg_subscription catalog
> +my $poll_query_until = $node_s->safe_psql('postgres',
> + "SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT
> IN ('e');"
> +);
> +
> +is($poll_query_until, qq(t),
> + 'Timed out while waiting for subscriber to enable twophase');
>
> 3a.
> Hmm. Does this code match the comment? The comment says verify
> subtwophase is 'p' (aka "pending") but AFAICT the code is actually
> waiting until every subtwophase is 'e' (aka "enabled").
>
> ~
>
> 3b.
> Also, if you are going to name char-codes (like 'p') in comments, it
> might be helpful to include the equivalent words, saving readers from
> having to search the documentation to find the meaning.
>I have fixed the given comments. The attached patch contains the suggested changes. Thanks and regards, Shubham Khanna.
v6-0001-Add-support-for-two-phase-commit-in-pg_createsubs.patch
Description: Binary data
