Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility. - Mailing list pgsql-hackers

On Fri, 13 Dec 2024 at 13:01, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Shubham.
> >
> > Here are my review comments for v6-0001.
> >
> > ======
> > 1.
> > +# Verify that the subtwophase is enabled ('e') in the pg_subscription catalog
> > +$node_s->poll_query_until('postgres',
> > + "SELECT count(1) = 2 FROM pg_subscription WHERE subtwophasestate = 'e';")
> > +  or die "Timed out while waiting for subscriber to enable twophase";
> > +
> >
> > This form of the SQL is probably OK but it's a bit tricky; Probably it
> > should have been explained in the comment about where that count "2"
> > has come from.
> >
> > ~~
> >
> > I think it was OK as before (v5) to be querying until nothing was NOT
> > 'e'. In other words, until everything was enabled 'e'.
> > SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate NOT IN ('e');
> >
> > ~~
> >
> > OTOH, to save execution time we really would be satisfied with both
> > 'p' and 'e' states here. (we don't strictly need to wait for the
> > transition from 'p' to 'e' to occur).
> >
> > So, SQL like the one below might be the best:
> >
> > # Verify that all subtwophase states are pending or enabled,
> > # e.g. there are no subscriptions where subtwophase is disabled ('d').
> > SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d')
> >
>
> I have fixed the given comment. Since prepared transactions are not
> being used anymore, I have removed it from the test file.
> The attached patch contains the suggested changes.

Few comments:
1) Since we are providing --enable-two-phase option now and user can
create subscriptions with two-phase option this warning can be removed
now:
    <para>
-    <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 <option>--enable-two-phase</option> 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
     the <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
-    option enabled.

2) Since we are not going to wait till the subscriptions are enabled,
we can use safe_psql instead of poll_query_until, something like:
is($node_s->safe_psql('postgres',
"SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate = 'd'"),
't', 'subscriptions are created with the two-phase option enabled');

instead of:
+$node_s->poll_query_until('postgres',
+       "SELECT count(1) = 0 FROM pg_subscription WHERE
subtwophasestate IN ('d');"
+);

3) This can be removed from the commit message:
Documentation has been updated to reflect the new option, and test cases have
been added to validate various scenarios, including proper validation of the
'--enable-two-phase' option and its combinations with other options.

4) Should" two-phase option" be "enable-two-phase option" here:
        const char *sub_username;       /* subscriber username */
+       bool            two_phase;              /* two-phase option */
        SimpleStringList database_names;        /* list of database names */

Regards,
Vignesh