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

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.

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

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

Best Regards,
Hou zj