On Thursday, March 7, 2024 10:05 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for v107-0001
Thanks for the comments. > > ====== > 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', Changed to 'nslotnames'. > > ~ > > 1b. > (fix typo) > > SUGGESTION for the 2nd field comment > slot_names is a chunk of 'n' X consecutive null-terminated C strings Changed. > > ~ > > 1c. > A more explanatory name for this typedef maybe is > 'StandbySlotNamesConfigData' ? Changed. > > ~~~ > > > 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 .../ Changed. > > ~ > > 2b. > Similar to above -- since this only has name information maybe it is more > correct to call it 'standby_slot_names_config'? > Changed. > ~~~ > > 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/ > Changed. > ~~~ > > 4. check_standby_slot_names > > + /* Need a modifiable copy of string */ rawname = pstrdup(*newval); > > /copy of string/copy of the GUC string/ > Changed. > ~~~ > > 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'? > The memory of extra is maintained by the GUC mechanism. It will be automatically freed when the associated GUC setting is no longer of interest. See src/backend/utils/misc/README for details. > ~~~ > > 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. The standby_slot_config will always be NULL if there is no value in it. While checking, I did find a rare case that if there are only some white space in the standby_slot_names, then slot_num will be 0, and have fixed it so that standby_slot_config will always be NULL if there is no meaning value in guc. > > ~ > > 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) Changed it to SlotExistsInStandbySlotNames. > > ~ > > 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; I personally prefer the current style. > > ~~~ > > 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. Same as above. > > ~~~ > > 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) Same as above, I think we can avoid checking slot_num. Best Regards, Hou zj