Re: Fix slot synchronization with two_phase decoding enabled

On Wed, May 28, 2025 at 6:10 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Sun, May 25, 2025 at 11:34 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> >
> > to
> > On Fri, May 23, 2025 at 10:06 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > >
> > > Here are review comments for v14 patch:
> > >
> >
> > Thank you for the review.
> >
> > > I think we need to include a basic test case where we simply create a
> > > subscription with two_phase=true and then enable the failover via
> > > ALTER SUBSCRIPTION after making sure that slot's restart_lsn passed
> > > two_phase_at. Probably if we use this case in 003_logical_slots.pl, we
> > > can avoid creating regress_sub2?
> > >
> >
> > A test has been added in 040_standby_failover_slots_sync.pl to enable
> > failover via ALTER SUBSCRIPTION.
>
> Yes but the slot is originally created via SQL API, which seems
> uncommon usage in practice. I thought it would be good to have the
> basic steps in the tests to enable both fields.
>

Apologies for the earlier misunderstanding. I've updated
040_standby_failover_slots_sync.pl to modify the slot creation via the
CREATE SUBSCRIPTION command as suggested.

> > Regarding regress_sub2 in 003_logical_slots.pl :
> > If we use ALTER SUBSCRIPTION to set failover=true on regress_sub, it
> > leads to pg_upgrade failure, as it attempts to create a slot on the
> > new_node(upgraded version) with both two_phase and failover enabled,
> > which is an unsupported combination.
>
> I think that the pg_upgrade test should cover the case where it
> restores logical slots with both fields enabled in the new cluster.
> When I actually tried this case, I realized that pg_upgrade doesn't
> handle this case; it tried to create the logical slot via SQL API but
> it failed as we don't allow it to create a logical slot with enabling
> both two_phase and failover. How can we handle this case?
>

On further analysis, it seems feasible and safe to allow creation of
slot and subscription with both two_phase and failover enabled during
upgrade (pg_upgrade).
As before starting the upgrade, pg_upgrade ensures that -
a) All slot changes have been fully consumed.
b) No prepared transactions exist.
Additionally, it is also documented[1] - "Obviously, no one should be
accessing the clusters during the upgrade."

Though, if allowed, the slot may be created with a restart_lsn <
two_phase_at. IIUC, the guarantees above make it impossible for a
prepared transaction to exist before two_phase_at, thus preventing the
bug case.
~~~~

Attached is the v16 patch, which includes the following changes:
- Enabled creation of slots and subscriptions with both two_phase and
failover options when in binary upgrade mode.
- Updated 003_logical_slots.pl to configure regress_sub's failover
option using ALTER SUBSCRIPTION, removing the need for the additional
regress_sub2 subscription.

[1] https://www.postgresql.org/docs/current/pgupgrade.html

Thanks & Regards,
Nisha Moond