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

On Wed, 15 Jan 2025 at 12:03, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> 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.
>

Hi Shubham,

Thanks for providing the updated patch.

I have few comments:

1. When we are using '--enable-two-phase' option, I think we should
also set the 'two_phase = true' while creating logical replication
slots as we are using the same slot for the subscriber.
Currently by default it is being set to 'false':

appendPQExpBuffer(str,
"SELECT lsn FROM
pg_catalog.pg_create_logical_replication_slot(%s, 'pgoutput', false,
false, false)",
slot_name_esc);

2. Regarding the documentation and warning message regarding the two
phase option, I agreed with the suggestions given by Peter in [1].

[1]: https://www.postgresql.org/message-id/CAHut%2BPviJDKW0ftZSg1cX4XK5EJmhGW-Uh3wGrRE4ktFrnxn9w%40mail.gmail.com

Thanks and Regards,
Shlok Kyal