On Thu, Mar 7, 2024 at 7:35 AM Peter Smith <smithpb2...@gmail.com> 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.