Here are some review comments for v107-0001 ====== src/backend/replication/slot.c
1. +/* + * Struct for the configuration of standby_slot_names. + * + * Note: this must be a flat representation that can be held in a single chunk + * of guc_malloc'd memory, so that it can be stored as the "extra" data for the + * standby_slot_names GUC. + */ +typedef struct +{ + int slot_num; + + /* slot_names contains nmembers consecutive nul-terminated C strings */ + char slot_names[FLEXIBLE_ARRAY_MEMBER]; +} StandbySlotConfigData; + 1a. To avoid any ambiguity this 1st field is somehow a slot ID number, I felt a better name would be 'nslotnames' or even just 'n' or 'count', ~ 1b. (fix typo) SUGGESTION for the 2nd field comment slot_names is a chunk of 'n' X consecutive null-terminated C strings ~ 1c. A more explanatory name for this typedef maybe is 'StandbySlotNamesConfigData' ? ~~~ 2. +/* This is parsed and cached configuration for standby_slot_names */ +static StandbySlotConfigData *standby_slot_config; 2a. /This is parsed and cached configuration for .../This is the parsed and cached configuration for .../ ~ 2b. Similar to above -- since this only has name information maybe it is more correct to call it 'standby_slot_names_config'? ~~~ 3. +/* + * A helper function to validate slots specified in GUC standby_slot_names. + * + * The rawname will be parsed, and the parsed result will be saved into + * *elemlist. + */ +static bool +validate_standby_slots(char *rawname, List **elemlist) /and the parsed result/and the result/ ~~~ 4. check_standby_slot_names + /* Need a modifiable copy of string */ + rawname = pstrdup(*newval); /copy of string/copy of the GUC string/ ~~~ 5. +assign_standby_slot_names(const char *newval, void *extra) +{ + /* + * The standby slots may have changed, so we must recompute the oldest + * LSN. + */ + ss_oldest_flush_lsn = InvalidXLogRecPtr; + + standby_slot_config = (StandbySlotConfigData *) extra; +} To avoid leaking don't we need to somewhere take care to free any memory used by a previous value (if any) of this 'standby_slot_config'? ~~~ 6. AcquiredStandbySlot +/* + * Return true if the currently acquired slot is specified in + * standby_slot_names GUC; otherwise, return false. + */ +bool +AcquiredStandbySlot(void) +{ + const char *name; + + /* Return false if there is no value in standby_slot_names */ + if (standby_slot_config == NULL) + return false; + + name = standby_slot_config->slot_names; + for (int i = 0; i < standby_slot_config->slot_num; i++) + { + if (strcmp(name, NameStr(MyReplicationSlot->data.name)) == 0) + return true; + + name += strlen(name) + 1; + } + + return false; +} 6a. Just checking "(standby_slot_config == NULL)" doesn't seem enough to me, because IIUIC it is possible when 'standby_slot_names' has no value then maybe standby_slot_config is not NULL but standby_slot_config->slot_num is 0. ~ 6b. IMO this function would be tidier written such that the MyReplicationSlot->data.name is passed as a parameter. Then you can name the function more naturally like: IsSlotInStandbySlotNames(const char *slot_name) ~ 6c. IMO the body of the function will be tidier if written so there are only 2 returns instead of 3 like SUGGESTION: if (...) { for (...) { ... return true; } } return false; ~~~ 7. + /* + * Don't need to wait for the standbys to catch up if there is no value in + * standby_slot_names. + */ + if (standby_slot_config == NULL) + return true; (similar to a previous review comment) This check doesn't seem enough because IIUIC it is possible when 'standby_slot_names' has no value then maybe standby_slot_config is not NULL but standby_slot_config->slot_num is 0. ~~~ 8. WaitForStandbyConfirmation + /* + * Don't need to wait for the standby to catch up if the current acquired + * slot is not a logical failover slot, or there is no value in + * standby_slot_names. + */ + if (!MyReplicationSlot->data.failover || !standby_slot_config) + return; (similar to a previous review comment) IIUIC it is possible that when 'standby_slot_names' has no value, then standby_slot_config is not NULL but standby_slot_config->slot_num is 0. So shouldn't that be checked too? Perhaps it is convenient to encapsulate this check using some macro: #define StandbySlotNamesHasNoValue() (standby_slot_config = NULL || standby_slot_config->slot_num == 0) ---------- Kind Regards, Peter Smith. Fujitsu Australia