RE: Synchronizing slots from primary to standby

On Thursday, March 7, 2024 10:05 AM Peter Smith <smithpb2250(at)gmail(dot)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