On Thu, Mar 19, 2026 at 12:08 PM Hou, Zhijie/侯 志杰 <[email protected]> wrote: > > On Wednesday, March 18, 2026 6:38 PM Ashutosh Sharma <[email protected]> > wrote: > > PFA patch that addresses above comments. > > Thanks for the patch! Overall, I think this is a valuable feature as I've > heard > requests from many customers for a way to avoid blocking logical replication > when only some instances are down when using synchronized_standby_slots. > > 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? thanks Shveta
