On Tue, Feb 27, 2024 at 4:07 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Tue, Feb 27, 2024 at 06:17:44PM +1100, Peter Smith wrote:
> > +static bool
> > +validate_standby_slots(char **newval)
> > +{
> > + char *rawname;
> > + List *elemlist;
> > + ListCell *lc;
> > + bool ok;
> > +
> > + /* Need a modifiable copy of string */
> > + rawname = pstrdup(*newval);
> > +
> > + /* Verify syntax and parse string into a list of identifiers */
> > + ok = SplitIdentifierString(rawname, ',', &elemlist);
> > +
> > + if (!ok)
> > + {
> > + GUC_check_errdetail("List syntax is invalid.");
> > + }
> > +
> > + /*
> > + * If the replication slots' data have been initialized, verify if the
> > + * specified slots exist and are logical slots.
> > + */
> > + else if (ReplicationSlotCtl)
> > + {
> > + foreach(lc, elemlist)
> >
> > 6a.
> > So, if the ReplicationSlotCtl is NULL, it is possible to return
> > ok=true without ever checking if the slots exist or are of the correct
> > kind. I am wondering what are the ramifications of that. -- e.g.
> > assuming names are OK when maybe they aren't OK at all. AFAICT this
> > works because it relies on getting subsequent WARNINGS when calling
> > FilterStandbySlots(). If that is correct then maybe the comment here
> > can be enhanced to say so.
> >
> > Indeed, if it works like that, now I am wondering do we need this for
> > loop validation at all. e.g. it seems just a matter of timing whether
> > we get ERRORs validating the GUC here, or WARNINGS later in the
> > FilterStandbySlots. Maybe we don't need the double-checking and it is
> > enough to check in FilterStandbySlots?
>
> Good point, I have the feeling that it is enough to check in FilterStandbySlots().
>
I think it is better if we get earlier in a case where the parameter
is changed and performed SIGHUP instead of waiting till we get to
logical decoding. So, there is merit in keeping these checks during
initial validation.
--
With Regards,
Amit Kapila.