guc comment changes (was Re: Getting a move on for 8.2 beta) - Mailing list pgsql-hackers

Peter Eisentraut <peter_e@gmx.net> 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