On Fri, 13 Dec 2024 at 15:33, Shubham Khanna
<khannashubham1...@gmail.com> wrote:
>
> On Fri, Dec 13, 2024 at 2:39 PM vignesh C <vignes...@gmail.com> wrote:
> >
> > On Fri, 13 Dec 2024 at 13:01, Shubham Khanna
> > <khannashubham1...@gmail.com> wrote:
> > >
> > > On Fri, Dec 13, 2024 at 12:20 PM Peter Smith <smithpb2...@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 */
> >
>
> I have fixed the given comments. The attached patch contains the
> suggested changes.
The documentation requires a minor update: instead of specifying
subscriptions, the user will specify multiple databases, and the
subscription will be created on the specified databases. Documentation
should be updated accordingly:
+ <para>
+ Enables <link
linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
+ commit for the subscription. If there are multiple subscriptions
+ specified, this option applies to all of them.
+ The default is <literal>false</literal>.
Regards,
Vignesh
