Re: Fix slot synchronization with two_phase decoding enabled

On Fri, Apr 11, 2025 at 1:03 PM Nisha Moond <nisha.moond412@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:


Approach#1:

1)
When slot is created with failover enabled and later we try to create
a subscription using that pre-created slot with two-phase enabled, it
does not error out. But it silently retains two_phase of the existing
slot as false. Please check if an error is possible in this case too.


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?

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.

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.

thanks
Shveta