Re: Fix slot synchronization with two_phase decoding enabled

On Thu, Jun 5, 2025 at 2:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Jun 3, 2025 at 11:05 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> >
> > Attached v17 patches. Added a top-up patch 0002 implementing the idea
> > suggested by Amit above.
>
> I have started reviewing this, although I haven't done a complete
> review yet, but I have a question on the fix we are trying to do, IIUC
> we are disallowing to use 'two phase' and 'failover' options together
> at the create slot time and now users has to create slot with one of
> the option and later enable other option right (if user want to use
> both options)? But don't you think it will affect usability? because
> if a user wants to use both the options together then after creating
> the slot they need to track when is the right time to enable the other
> option?  Not sure if anyone else has this concern or it's just me?
>
Some additional comments while quickly glancing at the patch

1.
+ if (two_phase && !IsSyncingReplicationSlots() && !IsBinaryUpgrade)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot enable both \"%s\" and \"%s\" options during
replication slot creation",
+    "failover", "two_phase"));

I think we should also give hints to retry later when a certain
constraint is met?

Also this is hardcoded options "failover" and "two_phase" so why do we
need to use %s for contruncting this error message?

2.
+    "failover", "two_phase"),
+ errhint("Use ALTER SUBSCRIPTION ... SET (failover = true) to enable
\"%s\" after two_phase state is ready",
+ "failover"));

Here we are using a mix of hardcoded string and formatted string, like
for (failover = true) we hardcoded the "failover" whereas to enable
\"%s\", we have
used %s.  Better to just directly use failover as we are not depending
on any variable.  Please look at other places as well, I see a few
more places whereas
we have used like this.

3. + if (slot->data.failover)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot enable two-phase decoding for failover enabled slot \"%s\"",
+ NameStr(slot->data.name))));

So for a failover slot we can never enable two_phase, whereas for
two_phase enabled slot we can enable failover? This seems confusing,
no?



-- 
Regards,
Dilip Kumar
Google