Re: Synchronizing slots from primary to standby

On Thu, Mar 7, 2024 at 7:35 AM Peter Smith  wrote:
>
> 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',
>

We can probably just add a comment above slot_num and that should be
sufficient but I am fine with 'nslotnames' as well, in anycase let's
add a comment for the same.

>
> 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)
>

+1. How about naming it as SlotExistsinStandbySlotNames(char
*slot_name) and pass the slot_name from MyReplicationSlot? Otherwise,
we need an Assert for MyReplicationSlot in this function.

Also, can we add a comment like below before the loop:
+ /*
+ * XXX: We are not expecting this list to be long so a linear search
+ * shouldn't hurt but if that turns out not to be true then we can cache
+ * this information for each WalSender as well.
+ */

-- 
With Regards,
Amit Kapila.