Re: Fix slot synchronization with two_phase decoding enabled

On Thu, Jun 5, 2025 at 3:26 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jun 5, 2025 at 2:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, Jun 3, 2025 at 11:05 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
> > >
> > >
> > > Attached v17 patches. Added a top-up patch 0002 implementing the idea
> > > suggested by Amit above.
> >
> > I have started reviewing this, although I haven't done a complete
> > review yet, but I have a question on the fix we are trying to do, IIUC
> > we are disallowing to use 'two phase' and 'failover' options together
> > at the create slot time and now users has to create slot with one of
> > the option and later enable other option right (if user want to use
> > both options)? But don't you think it will affect usability? because
> > if a user wants to use both the options together then after creating
> > the slot they need to track when is the right time to enable the other
> > option?  Not sure if anyone else has this concern or it's just me?
> >
> Some additional comments while quickly glancing at the patch
>

Thank you for the review.

> 1.
> + if (two_phase && !IsSyncingReplicationSlots() && !IsBinaryUpgrade)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot enable both \"%s\" and \"%s\" options during
> replication slot creation",
> +    "failover", "two_phase"));
>
> I think we should also give hints to retry later when a certain
> constraint is met?
>

Since it’s not possible to create a slot with both options enabled,
this command will always fail.

As there are no specific constraints to fulfill that would allow
creating a slot with both options enabled, the possible hints are:
 - Suggesting to use ALTER_REPLICATION_SLOT command to alter failover
later. But it may not be appropriate here from the user's perspective.
 - Recommending to use ALTER SUBSCRIPTION to enable failover later.
But not all slots are created for subscriptions, so this may not apply
in every case.

Please let me know if you have any suggestions for a suitable hint we
could provide here.

> Also this is hardcoded options "failover" and "two_phase" so why do we
> need to use %s for contruncting this error message?
>

I’ve followed the error message guidelines to keep the messages
translator-friendly. The use of %s helps isolate keywords like GUCs or
sub-options so they remain untouched during translation.

That said, it’s not a strict rule, usage often depends on developer or
committer judgment. After revisiting the docs and re-evaluating this
patch related files, I’ve adjusted the messages with the following in
mind:
 - do not use "%s" when terms "failover" and "two-phase" can be used
as concepts and not subscription options.
 - use "%s" when referring explicitly to options (or nearby code is so).
 - wrap SQL commands in double quotes for clarity.
 - since slot.c doesn’t use "%s" in similar contexts, I’ve updated the
message style there for consistency.

Please let me know if you feel any of the messages could still be
improved further.

> 2.
> +    "failover", "two_phase"),
> + errhint("Use ALTER SUBSCRIPTION ... SET (failover = true) to enable
> \"%s\" after two_phase state is ready",
> + "failover"));
>
> Here we are using a mix of hardcoded string and formatted string, like
> for (failover = true) we hardcoded the "failover" whereas to enable
> \"%s\", we have
> used %s.  Better to just directly use failover as we are not depending
> on any variable.  Please look at other places as well, I see a few
> more places whereas
> we have used like this.
>

Modified this message following the reasoning outlined above.

> 3. + if (slot->data.failover)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot enable two-phase decoding for failover enabled slot \"%s\"",
> + NameStr(slot->data.name))));
>
> So for a failover slot we can never enable two_phase, whereas for
> two_phase enabled slot we can enable failover? This seems confusing,
> no?
>

This restriction is not introduced by this patch.
As Amit pointed out [1], this patch is specific to PG17, where
altering the two_phase option for a subscription is not permitted.
This capability has been added in PG18 onwards.
~~~

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