Re: Adding a '--two-phase' option to 'pg_createsubscriber' utility. - Mailing list pgsql-hackers

On Fri, 17 Jan 2025 at 09:52, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Fri, Jan 17, 2025 at 5:43 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Shubham,
> >
> > Some review comments for patch v11-0001.
> >
> > ======
> > src/sgml/ref/pg_createsubscriber.sgml
> >
> > 1.
> > -    must accept local connections.
> > +    must accept local connections. If you are planning to use the
> > +    <option>--enable-two-phase</option> then you will also need to set the
> > +    <xref linkend="guc-max-prepared-transactions"/> appropriately.
> >     </para>
> >
> >
> > The missing word "switch"?
> >
> > /If you are planning to use the <option>--enable-two-phase</option>/If
> > you are planning to use the <option>--enable-two-phase</option>
> > switch/
> >
> >
> > 2.
> >     <para>
> > +    Unless the '--enable-two-phase' switch is specified,
> >      <application>pg_createsubscriber</application> sets up logical
> >
> > Remove the single quotes, and use the proper SGML markup same as other
> > places where --enable-two-phase is mentioned.
> >
> > <option>--enable-two-phase</option>
> >
> > ======
> >
> >
> > 3.
> > + pg_log_warning_hint("You can use '--enable-two_phase' switch to
> > enable two_phase.");
> >
> > 3a.
> > Typo. The real switch name does not have underscores.
> >
> > /--enable-two_phase/--enable-two-phase/
> >
> > ~
> >
> > 3b.
> > I checked other PG source code, but I couldn't find any examples where
> > the switch is given in single quotes quite like this.
> >
> > Maybe choose from one of the below instead:
> >
> > SUGGESTION #1
> > Use the \"--enable-two-phase\" switch to enable two_phase.
> >
> > SUGGESTION #2
> > Use the --enable-two-phase switch to enable two_phase.
> >
>
> Fixed the given comments. The attached patch contains the required changes.
>

Hi Shubham,

I have a comment for the v12 patch.
I think we can pass just the two_phase option instead of the whole
structure here. As we are just using 'opt->two_phase'.

+check_publisher(const struct LogicalRepInfo *dbinfo,
+               const struct CreateSubscriberOptions *opt)

Thanks and Regards,
Shlok Kyal