Re: Fix slot synchronization with two_phase decoding enabled

On Fri, Apr 25, 2025 at 5:53 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)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?

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.

3)
CreateSubscription:

+ if (opts.twophase && opts.failover)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot enable both \"%s\" and \"%s\" options at CREATE SUBSCRIPTION",
+ "failover", "two_phase"),
+ errhint("The \"%s\" option can be enabled after \"%s\" state is ready.",
+ "failover", "two_phase"));

Shall we mention "using Alter Sub" in the errhint?

4)

AlterSubscription:

+ if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_PENDING &&
+ opts.failover)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot enable failover for a subscription with a pending
two_phase state"));
+state"));

Shall we mention errhint here too?

thanks
Shveta