Re: Fix slot synchronization with two_phase decoding enabled

On Thu, Jun 12, 2025 at 4:00 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Wed, Jun 11, 2025 at 8:16 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > On Fri, Jun 6, 2025 at 5:07 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> > >
> > >
> > > Attached v18 patch.
> > > - patch-001: modified error messages as suggested above.
> > > - patch-002: improved pg_dump docs as per Shveta's off-list suggestions.
> > >
> > > [1] https://www.postgresql.org/message-id/CAA4eK1%2BB067G8mUJzKUEjc5KSkYq6z0utTaHey-qeRt%2BnZTNJg%40mail.gmail.com
> > >
> > > --
> > > Thanks,
> > > Nisha
> >
> > Hi Nisha,
> >
> > Some comments.
> >
> > Patch 0001
> > 1. Commit message
> >
> > from "two_phase enables slots." to "two_phase enabled slots"
> >
> > 2.
> > +
> > + <para>
> > + When a subscription's
> > + <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
> > + is in the pending state, setting <literal>failover</literal> to
> > true is not
> > + permitted. Once
> >
> > The term "subscription's two_phase" sounds a bit vague here.
> > Referring https://www.postgresql.org/docs/17/sql-createsubscription.html
> > and https://www.postgresql.org/docs/current/catalog-pg-subscription.html
> > two_phase is referred to both as the "internal state" as well as the
> > "mode" of the subscription.
> >
> > Probably rephrase it to - "When a subscription's internal two_phase
> > mode is in the pending state, setting failover to true is not
> > permitted. See column subtwophasestate of pg_subscription to know the
> > actual internal state of the two-phase mode" (with appropriate tags
> > and reference links)
> >
>
> There seems a potential muddling of terms two_phase VS two-phase.
>
> AFAIK "two_phase" (with the underscore) is the name of the
> subscription option; it might also be the name of an internal
> structure member or catalog column name. Furthermore, when you are
> referring to the option name I think it ought to be double-quoted for
> clarity.
>
> But, unless you are referring specifically to the option/field/column
> then IMO it should say two-phase (with the dash).
> e.g. The "two_phase" option enables two-phase mode.
>

I agree with your suggestion to use two-phase when referring to the
internal state of the subscription.
I've updated the commit message and the respective doc (with reference
link) accordingly in v19 patches.

--
Thanks,
Nisha