On Thursday, March 7, 2024 12:46 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> 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.Added. > > > > > 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. Changed as suggested. > > 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. > + */ Added. Attach the V108 patch set which addressed above and Peter's comments. I also removed the check for "*" in guc check hook. Best Regards, Hou zj
v108-0001-Allow-logical-walsenders-to-wait-for-the-physic.patch
Description: v108-0001-Allow-logical-walsenders-to-wait-for-the-physic.patch
