Re: guc comment changes (was Re: Getting a move on

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: guc comment changes (was Re: Getting a move on
Date: 2006-09-15 20:46:24
Message-ID: 200609152046.k8FKkOr03672@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


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

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

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)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(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Magnus Hagander 2006-09-15 20:57:31 pg_strcasecmp in fe-connect.c
Previous Message mark 2006-09-15 20:41:20 Re: Optimize ORDER BY ... LIMIT