GUC assign hooks (was Re: wal_buffers = -1 and SIGHUP) - Mailing list pgsql-hackers

I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I had intended to commit Greg's patch with a show hook and an assign
>> hook and the calculated value stored separately from the GUC variable,
>> which I believe would avoid this is a problem, but Tom thought this
>> way was better.  Unfortunately, my knowledge of the GUC machinery is
>> insufficient to think of a way to avoid it, other than the one Tom
>> already rejected.

> Mph ... I think this shows the error of my thinking :-(.  We at least
> need an assign hook here.  Will fix, since it was my oversight to begin
> with.

After thinking about this for awhile, I think the fundamental problem
is in the is_newvalue_equal() function, which as its comment states is
pretty cheesy:

/** Attempt (badly) to detect if a proposed new GUC setting is the same* as the current value.** XXX this does not
reallywork because it doesn't account for the* effects of canonicalization of string values by assign_hooks.*/
 

If you hold your head at a funny angle you can see replacement of "-1"
with a suitable default value as a form of canonicalization, so the
behavior Bernd complained of is exactly what the comment is talking
about.  We've not had complaints previously because is_newvalue_equal()
is only used for PGC_POSTMASTER variables, and few of those have assign
hooks that do any canonicalization.  But it is possible to trigger the
problem with unix_socket_directory, for example: try setting it to
something like '/tmp/bogus/..', and you'll see that pg_ctl reload
triggers a log message:

LOG:  parameter "unix_socket_directory" cannot be changed without restarting the server

Robert had suggested fixing this by kluging up wal_buffers' assign and
show hooks, but IIRC he never actually got that to work; I doubt it's
possible to make it work in the EXEC_BACKEND case without some
additional hacks to get the state to be properly replicated into child
processes.  Even if we could make it work, wal_buffers is not likely to
be the last variable that we'll want to allow auto-tuning of.  So
I think we ought to address the underlying problem instead of trying
to work around it in the variable-specific code for wal_buffers.

IMO the real problem is essentially that GUC assign hooks have two
functions, checking and canonicalization of the value-to-be-stored
versus executing secondary actions when an assignment is made; and
there's no way to get at just the first one.  So we cannot canonicalize
the value first and then see if it's equal to the current setting.
I think the only real fix is to change the API for assign hooks.  This
is a bit annoying but it's not like we haven't ever done that before.
I'm thinking about splitting assign hooks into two functions, along the
lines of
bool check_hook (datatype *new_value, GucSource source)bool assign_hook (datatype new_value, GucSource source)

check_hook would validate the new value, and possibly change it (hence
the pass-by-reference parameter).  assign_hook would only be responsible
for executing secondary actions needed when an assignment is done.
The "doit" flag can go away since we'd not call the assign_hook at all
unless we actually want the assignment to occur.  I think most of the
existing uses might only need one or the other of these hooks, but
haven't gone through them yet.  It might be appropriate to change the
signatures some more while we're at it, in particular pass the desired
elog message level explicitly instead of making hooks infer it from the
source parameter.

It would probably take less than a day to flesh out this idea and make
it happen, but it does seem like a rather large change for late alpha.
If people don't want to do this now, I suggest that we just live with
the problem for 9.1.  It's purely cosmetic, and easy enough to work
around (just don't uncomment the default value).
        regards, tom lane