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?

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.

Thanks and Regards,
Shlok Kyal


Reply via email to