Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility.

On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> <khannashubham1197(at)gmail(dot)com> wrote:
> >
> > On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > > > > > >
> > > 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>.
> > >
> >
> > I have fixed the given comment. The attached patch contains the
> > suggested changes.
> >
>
> Hi Shubham,
>
> I have reviewed the v9 patch. It looks fine to me. I have tested it
> and it is working as expected.
> I have few comments:
>
> 1.
> - pg_log_debug("subscriber(%d): subscription: %s ; connection
> string: %s", i,
> + pg_log_debug("subscriber(%d): subscription: %s ; connection
> string: %s, two_phase: %s", i,
> dbinfo[i].subname ? dbinfo[i].subname : "(auto)",
> - dbinfo[i].subconninfo);
> + dbinfo[i].subconninfo,
> + opt->two_phase ? "true" : "false");
> Here the value of 'opt->two_phase' will be the same for all
> subscriptions. So is it good to log it here? Thoughts?
>

Here, the 'two-phase option' is a global setting, so it makes sense to
log it consistently within the debug output. While it is true that
'opt->two_phase' will have the same value for all subscriptions,
including it in the debug log provides clarity about the replication
setup at the time of execution.
This information can be particularly useful for debugging purposes, as
it gives immediate insight into whether a 'two-phase' commit was
enabled or disabled during the setup. Therefore, logging the
'two-phase' status, even though it's consistent across subscriptions,
adds valuable context to the debug output.

> 2. In documentation pg_createsubscriber.sgml. Under Warning section,
> we have following:
>
> <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.
>
> I think we should update the documentation accordingly.
>

Previously, the warning was necessary because the 'two-phase' option
was not available, and users needed to be informed about the default
behavior regarding 'two-phase' commit. However, with the recent
addition of the 'two-phase' option, users can now directly configure
this behavior during the setup process.
Given this enhancement, the warning message is no longer relevant and
should be removed from the documentation to reflect the latest changes
accurately. Updating the documentation will help ensure that it aligns
with the current functionality and avoids any potential confusion for
users.

The attached patch contains the required changes.

Thanks and regards,
Shubham Khanna.