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. -- With Regards, Ashutosh Sharma. > [1] > https://www.postgresql.org/message-id/CAFPTHDbrJJtaR4Jf2HNOZQVyBLJF-kq8kk%3DFvmeZ1rfU%2BY3R5g%40mail.gmail.com
