Re: synchronized_standby_slots behavior inconsistent with quorum-based synchronous replication - Mailing list pgsql-hackers

Hi,

On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com> wrote:
>
> On Friday, March 20, 2026 3:17 PM Ashutosh Sharma <ashu.coek88@gmail.com>
> >
> > On Fri, Mar 20, 2026 at 12:14 PM shveta malik <shveta.malik@gmail.com>
> > wrote:
> > >
> > > On Thu, Mar 19, 2026 at 12:08 PM Hou, Zhijie
> > > <houzj.fnst@fujitsu.com> wrote:
> > > > I didn't find any bugs in the patch, but I have a few comments on the code
> > and
> > > > tests:
> > > >
> > > > 1.
> > > >
> > > > > /*
> > > > >  * Return true if value starts with explicit FIRST syntax:
> > > > >  *
> > > > >  *   FIRST N (...)
> > > > >  *
> > > > >  * This is used to distinguish explicit FIRST from simple list syntax whose
> > > > >  * first slot name may start with "first".
> > > > >  */
> > > > > static bool
> > > > > IsPrioritySyncStandbySlotsSyntax(const char *value)
> > > >
> > > > I think adding a new function to manually parse the list isn't the most
> > elegant
> > > > approach. Instead, it would be cleaner to have a new flag that
> > distinguishes
> > > > when a plain name list is specified, and use that to mark the case
> > > > appropriately like:
> > > >
> > > > /* syncrep_method of SyncRepConfigData */
> > > > #define SYNC_REP_PRIORITY               0
> > > > #define SYNC_REP_QUORUM         1
> > > > +#define SYNC_REP_IMPLICIT              2
> > > >
> > > > standby_config:
> > > > -               standby_list                            { $$ = create_syncrep_config("1", $1,
> > SYNC_REP_PRIORITY); }
> > > > +               standby_list                            { $$ = create_syncrep_config("1", $1,
> > SYNC_REP_IMPLICIT); }
> > > >
> > >
> > > I like the idea. The only thing we have to see is that the changes in
> > > synchronous_standby_names for this look clean (converting IMPLICIT to
> > > PRIORITY and overwriting num_sync to 1). Also, do you think
> > > SYNC_REP_ALL is more meaningful than SYNC_REP_IMPLICIT?
> > >
> >
> > In my view, modifying the shared parser code (the syncrep parser in
> > this case) may not be the best approach, especially when it can be
> > avoided. The current design keeps changes localized to
> > synchronized_standby_slots parsing within the slot handling logic, and
> > preserves existing synchronous replication semantics. The localized
> > approach is also comparatively safer (risk free) and easier to
> > maintain.
> >
> > Additionally, the function that inspects the syncrep_parse output is
> > fairly straightforward, it simply checks for the presence of the FIRST
> > N syntax.
>
> Since we're reusing the same parser for two GUCs that have different
> interpretations of one syntax variant (the plain slot list), making the parser
> more general is a natural approach, especially given that the patch is adding
> new functionality here.
>
> My main concern is the IsPrioritySyncStandbySlotsSyntax() function. It
> introduces additional hard-coded parsing logic that duplicates what's already
> implemented in syncrep_gram.y. I'm also concerned about maintainability,
> particularly since we already discovered a bug in the hard-coded parser code [1]
> and the patch even added a tap-test (part E) to cover that path. All of this
> effort could be avoided by removing this function and leveraging functionality
> provided by the shared parser.
>

The issue that you are referring to here was without this function.

The idea here is to reuse the existing synchronous_standby_names
parser as-is, without changing its grammar or parse behavior.
synchronized_standby_slots differs only in post-parse interpretation
of simple-list syntax, so we add a local helper to disambiguate
explicit priority mode from plain lists before applying
synchronized_standby_slots semantics.

> That said, this is just my opinion. I'm okay with whichever approach
> the community ultimately agrees on.
>

Sure, thanks for sharing your thoughts. We can wait and see what
others have to say on this.

--
With Regards,
Ashutosh Sharma.

> [1] https://www.postgresql.org/message-id/CAFPTHDbrJJtaR4Jf2HNOZQVyBLJF-kq8kk%3DFvmeZ1rfU%2BY3R5g%40mail.gmail.com