Re: Fix slot synchronization with two_phase decoding enabled

On Mon, Apr 28, 2025 at 12:04 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Apr 25, 2025 at 5:53 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Please find the attached v8 patch with above comments addressed.
> > This version includes the documentation updates suggested by
> > Sawada-san at [1], and incorporates his feedback from [2] as well.
> >
>
> Thanks for the patches.
>
>
> 1)
> Regarding documents, these are the details added:
>  a) pg_create_logical_replication_slot  says "The parameters twophase
> and failover cannot be enabled together when creating a replication
> slot."
>  b) CREATE_REPLICATION_SLOT says TWO_PHASE: This option is mutually
> exclusive with failover.
>  c) CREATE SUBSCRIPTION doc says: The options failover and twophase
> cannot be enabled together when creating a subscription.
>  d) ALTER SUB says: When two_phase is in the "pending" state, setting
> failover = true is not permitted. Once two_phase is "enabled",
> failover = true can be set only if the slot's restart_lsn has advanced
> beyond its two_phase_at value.
>
> The individual points mentioned are correct. But nowhere we get
> complete picture as in how user can enable failover and twophase
> together  when setting up a slot and subscription. I think we can add
> brief notes in a,b and c to state how user can actually enable these 2
> together.
> Or can add this detail in single page and let other pages refer/point
> to that page. Thoughts?
>

I have updated the docs with a brief explanation of how both options
can be enabled together for points (a), (b), and (c).
Let’s wait for others’ opinions on whether this detail should stay
consolidated on one page, with other pages referring to it.

> 2)
>
> ReplicationSlotCreate():
>
> + if (two_phase && !IsSyncingReplicationSlots())
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot enable both \"%s\" and \"%s\" options during
> replication slot creation",
> +    "failover", "two_phase"),
> + errhint("The \"%s\" option can be enabled after \"%s\" state is ready.",
> + "failover", "two_phase"));
>
> I think  errhint here is not correct. We can not alter failover of
> replication slot without /o doing create-sub first and then performing
> alter-sub. So either we should mention complete details or remove this
> hint, otherwise it is confusing.
>

Removed the hint. I feel this explanation will be too long for the
HINT and also docs have the details.
~~~

Please find the v9 patch, addressing the above and all other comments as well.

--
Thanks,
Nisha