OK, patch sent to the 8.3 hold queue for later work, open item removed. ---------------------------------------------------------------------------
Tom Lane wrote: > Peter Eisentraut <[EMAIL PROTECTED]> writes: > > That does not mean that the patch is bad, and I certainly support the > > feature change. But I can't efficiently review the patch. If someone > > else wants to do it, go ahead. > > I've finally taken a close look at this patch, and I don't like it any > more than Peter does. The refactoring might or might not be good at its > core, but as presented it is horrid. As just one example, it replaces one > reasonably well-commented function with three misnamed, poorly commented > functions. In place of > > /* > ! * Sets option `name' to given value. The value should be a string > ! * which is going to be parsed and converted to the appropriate data > ! * type. The context and source parameters indicate in which context this > ! * function is being called so it can apply the access restrictions > ! * properly. > ! * > ! * If value is NULL, set the option to its default value. If the > ! * parameter changeVal is false then don't really set the option but do all > ! * the checks to see if it would work. > ! * > ! * If there is an error (non-existing option, invalid value) then an > ! * ereport(ERROR) is thrown *unless* this is called in a context where we > ! * don't want to ereport (currently, startup or SIGHUP config file reread). > ! * In that case we write a suitable error message via ereport(DEBUG) and > ! * return false. This is working around the deficiencies in the ereport > ! * mechanism, so don't blame me. In all other cases, the function > ! * returns true, including cases where the input is valid but we chose > ! * not to apply it because of context or source-priority considerations. > ! * > ! * See also SetConfigOption for an external interface. > */ > ! bool > ! set_config_option(const char *name, const char *value, > ! GucContext context, GucSource source, > ! bool isLocal, bool changeVal) > > we find > > /* > ! * Try to parse value. Determine what is type and call related > ! * parsing function or if newval is equal to NULL, reset value > ! * to default or bootval. If the value parsed okay return true, > ! * else false. > */ > ! static bool > ! parse_value(int elevel, const struct config_generic *record, > ! const char *value, GucSource *source, bool changeVal, > ! union config_var_value *retval) > > which doesn't tell you quite what the parameters do, but more > fundamentally is misnamed because one would expect "parse_value" > returning bool to merely check whether the value is syntactically > correct. Well, it doesn't do that: it applies the value too. > Another broken-out routine is > > ! /* > ! * Check if the option can be set at this time. See guc.h for the precise > ! * rules. > ! */ > ! static bool > ! checkContext(int elevel, struct config_generic *record, GucContext context) > > which is again a misleading description because it doesn't bother to > explain that control may not come back if the option is rejected > (depending on elevel). One might also think, given that description, > that the caller is supposed to emit an error message on false result. > Lastly we have > > + /* > + * Verify if option exists and value is valid. > + * It is primary used for validation of items in configuration file. > + */ > + bool > + verify_config_option(const char *name, const char *value, > + GucContext context, GucSource source, > + bool *isNewEqual, bool *isContextOK) > > which again is far south of my ideas for adequate documentation of a > function with a fairly complicated API. And guess what, this one has > side effects too, which it surely should not (and that leads directly > to a bug: GUC_IN_CONFFILE could remain set in a variable after a > parsing failure). > > It's possible that a refactoring along these lines could be an > improvement if it were well coded and well documented, but this patch > is not it. > > The comment-reversion part of the patch is not any better. It's poorly > factored (what the heck is guc-file.l doing patching up the source > settings after calling set_config_option?), which is surprising > considering the whole point of the refactoring was to support this. > And the handling of reversion to a PGC_S_ENV_VAR setting is just a kluge > involving duplicated code (what was that about refactoring again?). > > In short, whether or not it has any remaining undetected bugs, this > patch is a severe disimprovement from the standpoint of source code > quality, and I recommend rejecting it. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 3: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faq -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---------------------------(end of broadcast)--------------------------- TIP 6: explain analyze is your friend
