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

On Mon, Jan 20, 2025 at 1:34 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> 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)
>

Fixed the given comment. The attached patch contains the required changes.

Thanks and regards,
Shubham Khanna.