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

OK, patch sent to the 8.3 hold queue for later work, open item removed.

---------------------------------------------------------------------------

Tom Lane wrote:
> 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
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
> 
>                http://www.postgresql.org/docs/faq

--  Bruce Momjian   bruce@momjian.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +