On Fri, Mar 20, 2026 at 2:08 PM Ashutosh Sharma <[email protected]> wrote: > > Hi, > > On Fri, Mar 20, 2026 at 1:21 PM Hou, Zhijie/侯 志杰 <[email protected]> > wrote: > > > > On Friday, March 20, 2026 3:17 PM Ashutosh Sharma <[email protected]> > > > > > > On Fri, Mar 20, 2026 at 12:14 PM shveta malik <[email protected]> > > > wrote: > > > > > > > > On Thu, Mar 19, 2026 at 12:08 PM Hou, Zhijie > > > > <[email protected]> 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. >
FWIW, I’m also slightly hesitant to touch the synchronous_standby_names code for this, and I had concern about whether the changes in that layer will be clean/acceptable. But let’s see what others have to say. thanks Shveta
