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. Thanks and regards, Shubham Khanna.
v8-0001-Add-support-for-two-phase-commit-in-pg_createsubs.patch
Description: Binary data
