On Tue, Apr 22, 2025 at 3:23 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Fri, Apr 11, 2025 at 1:03 PM Nisha Moond <nisha.moond...@gmail.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
v6-0001-PG17-Approach-2-Fix-slot-synchronization-for-two_.patch
Description: Binary data
