Re: Avoid resetting subscription two_phase state to pending when already enabled

On Thu, Oct 29, 2026 at 8:44 PM Zhijie Hou <houzhijie22(at)gmail(dot)com> wrote:
>
> On Thu, Oct 29, 2026 at 06:41 PM Bertrand Drouvot wrote:
> >
> > On Thu, Oct 29, 2026 at 03:52:46PM +0530, vignesh C wrote:
> >> There is the following documentation mentioning that slots must be
> >> created with the same parameters of the subscription at [1].
> >> When altering the slot_name, the failover and two_phase property
> >> values of the named slot may differ from the counterpart failover and
> >> two_phase parameters specified in the subscription. When creating the
> >> slot, ensure the slot properties failover and two_phase match their
> >> counterpart parameters of the subscription. Otherwise, the slot on the
> >> publisher may behave differently from what these subscription options
> >> say: for example, the slot on the publisher could either be synced to
> >> the standbys even when the subscription's failover option is disabled
> >> or could be disabled for sync even when the subscription's failover
> >> option is enabled.
> >>
> >> I'm not sure if this should be handled here as it is documented already.
> >
> > I agree about the documentation, but the patch introduces a regression: without
> > the patch, two_phase is enabled on the replacement slot. With the patch, it remains
> > disabled.
>
> Like Vignesh, I think the documentation already requires users to create
> the replacement slot with matching properties when altering slot_name,
> so we don't promise that setting two_phase = true will update the
> replacement slot.
>
> > It might be worth fixing, as preserving 'e' only when slot_name is unchanged
> > should be simple enough.
>
> I don't think handling that here would be simple. Resetting ENABLED to
> PENDING when slot_name is changed would re-introduce the same bugs that
> this patch aims to fix: prepared transactions from the previous slot may
> still exist on the subscriber, and if the replacement slot already has
> two_phase enabled on the publisher, prepared transactions could again be
> streamed concurrently with table synchronization. Avoiding that would
> require checking prepared transactions and the remote slot's status
> during the change, which would complicate the fix.
>

I agree that we don't need to preserve the old behavior as it will
open up other bugs and we already document that such slot's properties
should match subscription's options. But I feel we should add that
change in behavior for new slot's in the commit message. Other than
that the latest patch in this thread looks good to me.

--
With Regards,
Amit Kapila.