Re: Fix slot synchronization with two_phase decoding enabled

On Tue, Apr 22, 2025 at 3:23 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Apr 11, 2025 at 1:03 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> >
> >
> > Patch "v5_aprch_3-0001" implements the above Approach 3.
> >
> > Thanks Hou-san for implementing approach-2 and providing the patch.
> >
>
> I find Approach 2 better than Approach1. Yet to review Approach3.
> Please find my initial comments:
>

Thanks for the review! I’ve addressed the comments for Approach 2, as
it seems the most suitable. We can revisit the others if needed.

>
>
> Approach #2:
>
> 1)
> + ReorderBufferSkipPrepare(reorder, parsed.twophase_xid);
>
> In xact_decode, why do we have a new call to ReorderBufferSkipPrepare,
> can you please add some comments here?
>

Done.

> 2)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("\"%s\" and \"%s\" are mutually exclusive options when \"%s\"
> is specified",
>
> So like approach 1, here as well we disallow creating subscriptions
> with both failover and two_phase enabled when create_slot and
> copy_data is true? But users can alter the sub later to allow failover
> for two-phase enabled slot provided there are no pending PREPARE txns
> which are not yet consumed by the subscriber. Is my understanding
> correct? Can you please tell all the ways using which the user can
> enable both failover and two_phase together? The patch msg is not that
> clear. It will be good to add such details in patch message.
>

We allow creating subscriptions in this scenario as long as no
prepared transactions exist before the two_phase_at. Similarly,
altering a subscription to set failover = true is permitted, provided
the slot's restart_lsn is greater than two_phase_at.
Amit has suggested the ways at [1] in which users can enable both
failover and two_phase together.
I've updated the commit message to include more details about the
conditions enforced by the fix.

> 3)
>
> + /*
> + * Do not allow users to enable the failover and two_phase options together
> + * when create_slot is specified.
> + *
> + * See comments atop the similar check in DecodingContextFindStartpoint()
> + * for a detailed reason.
> + */
> + if (!opts.create_slot && opts.twophase && opts.failover)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("\"%s\" and \"%s\" are mutually exclusive options when \"%s\"
> is specified",
> +
>
> Is the check '!opts.create_slot' correct? The error msg and comment
> says otherwise.
>

Corrected the check as it should be checking if copy_data is
specified. Thanks for the input!

Please find the attached v6 patch for approach-2 fix on PG17.

[1] https://www.postgresql.org/message-id/CAA4eK1LvMwXxvAzHpK%2BEgjc7vu1NmGxxKcaK_06pE7GKk7JtJQ%40mail.gmail.com

--
Thanks,
Nisha