On Fri, 12 Jun 2026 at 15:40, Ashutosh Sharma <[email protected]> wrote: > > Hi Shveta, > > Thanks again for your review comments and suggestions. Please see my > comments inline below: > > On Wed, Jun 10, 2026 at 12:16 PM shveta malik <[email protected]> wrote: > > > > Please find a few comments on June8 version fo patches: > > > > 1) > > patch001: > > > > SYNC_REP_DEFAULT: do we need to give one-line comment for this > > somewhere as unlike PRIORITY and QUORUM, it is not self-explanatory. > > > > Yes, it does makes sense to include a one-line comment. I've added it > in the attached patch. > > > > > patch002: > > 2) > > It is better to avoid mentioning it as 'synchronized standby slots'. > > We can make it as 'synchronized_standby_slots' in below comments: > > > > + /* Parse the synchronized standby slots configuration */ > > > > + * Report problem states for synchronized standby slots that prevented the > > > > Good point. I've made the suggested change in the attached patch > > > 3) > > For these error-messages too, we need to mention GUC name to give > > better clarity. > > > > + GUC_check_errmsg("number of synchronized standby slots (%d) must not > > exceed the number of unique listed slots (%d)", > > + syncrep_parse_result->num_sync, > > + syncrep_parse_result->nmembers); > > > > > > How about: > > GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); > > GUC_check_errmsg("invalid value for parameter \"%s\: synchronization > > requirement (%d) exceeds the number of unique listed slots (%d)", > > "synchronized_standby_slots", > > syncrep_parse_result->num_sync, > > syncrep_parse_result->nmembers); > > > > Updated as suggested in the attached patch. > > > 3) > > + GUC_check_errmsg("number of synchronized standby slots (%d) must be > > greater than zero", > > + syncrep_parse_result->num_sync) > > > > How about: > > GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE); > > GUC_check_errmsg("invalid value for parameter \"%s\: required number > > of synchronized standby slots (%d) must be greater than zero", > > "synchronized_standby_slots", > > syncrep_parse_result->num_sync); > > > > > > > > Updated as suggested in the attached patch. > > > 4) > > +ReportUnavailableSyncStandbySlots(SyncStandbySlotsStateInfo * slot_states > > > > We can get rid of space before slot_states. I think pgindent might > > have put it in my patch. Sorry for the trouble. > > > > 5) > > 003: > > - wait_for_all = (required == > > synchronized_standby_slots_config->nslotnames); > > + wait_for_all = > > + (synchronized_standby_slots_config->syncrep_method == SYNC_REP_DEFAULT); > > > > This change can be moved to 002, right? > > Done. > > PFA patch containing all the above changes. > Hi Ashutosh,
I have reviewed the patches. Here are some comments: 1. Should we update the doc for function "pg_logical_slot_get_changes". It says: ``` If the specified slot is a logical failover slot then the function will not return until all physical slots specified in <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link> have confirmed WAL receipt. ``` This line "return until all physical slots specified in" seems wrong after introduction of "FIRST/ANY" 2. Similarly, should we update the doc for function "pg_replication_slot_advance"? It also says: ``` If the specified slot is a logical failover slot then the function will not return until all physical slots specified in <link linkend="guc-synchronized-standby-slots"><varname>synchronized_standby_slots</varname></link> have confirmed WAL receipt. ``` 3. Comment in syncrep_gram.y states: * syncrep_gram.y - Parser for synchronous_standby_names Since synchronosed_standby_slots is reusing this, should we update this comment? 4. Similarly for syncrep_scanner.l, we have comment: * syncrep_scanner.l * a lexical scanner for synchronous_standby_names Should we update it? 5. enum "SyncStandbySlotsState" and stuct "SyncStandbySlotsStateInfo" should be mentioned in typedefs.list, so that pgindent works properly. Thanks, Shlok Kyal
