On Tue, Feb 27, 2024 at 4:07 PM Bertrand Drouvot <bertranddrouvot...@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.