On Thu, Jan 16, 2025 at 3:15 PM Shlok Kyal <shlok.kyal....@gmail.com> wrote:
>
> On Wed, 15 Jan 2025 at 12:03, Shubham Khanna
> <khannashubham1...@gmail.com> wrote:
> >
> > On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal <shlok.kyal....@gmail.com> wrote:
> > >
> > > On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> > > <khannashubham1...@gmail.com> wrote:
> > > >
> > > > On Fri, Dec 27, 2024 at 11:30 AM vignesh C <vignes...@gmail.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
>
Following our previous discussions, I have incorporated the necessary
changes to ensure that when the '--enable-two-phase' option is used,
the logical replication slots are also created with the 'two_phase =
true' setting. This addresses the current behavior where 'two_phase'
is set to false by default.
The v11 version patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BZqc9qVEaJDW03Cux_S_CMHWNM056qqJi5%2Bz9vSYgtew%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
