Re: wal_buffers = -1 and SIGHUP

On Thu, Mar 31, 2011 at 8:38 AM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
> This might be nitpicking (or i'm currently missing something), but i
> recognized that setting wal_buffers = -1 always triggers the following on
> reload, even if nothing to wal_buffers had changed:
>
> $ pg_ctl reload
> LOG:  received SIGHUP, reloading configuration files
> LOG:  parameter "wal_buffers" cannot be changed without restarting the
> server
>
> This only happens when you have wal_buffers set to -1.

This is a bug. The root cause is that, on startup, we do this:

if (xbuffers != XLOGbuffers)
{
snprintf(buf, sizeof(buf), "%d", xbuffers);
SetConfigOption("wal_buffers", buf, PGC_POSTMASTER,
PGC_S_OVERRIDE);
}

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Mar 31, 2011 at 8:38 AM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>> This might be nitpicking (or i'm currently missing something), but i
>> recognized that setting wal_buffers = -1 always triggers the following on
>> reload, even if nothing to wal_buffers had changed:
>>
>> $ pg_ctl reload
>> LOG: received SIGHUP, reloading configuration files
>> LOG: parameter "wal_buffers" cannot be changed without restarting the
>> server
>>
>> This only happens when you have wal_buffers set to -1.

> This is a bug. The root cause is that, on startup, we do this:

> if (xbuffers != XLOGbuffers)
> {
> snprintf(buf, sizeof(buf), "%d", xbuffers);
> SetConfigOption("wal_buffers", buf, PGC_POSTMASTER,
> PGC_S_OVERRIDE);
> }

> 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.

regards, tom lane

I wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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 really work 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

On Sun, Apr 3, 2011 at 1:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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.

Yes, I think that's right. A related point is that the API for assign
hooks is not consistent across all data types: string assign hooks can
return a replacement value, whereas everyone else can only succeed or
fail.

> 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).

I think it's a pretty ugly wart, so I'm inclined to say go ahead and
fix it. I'm not sure what alpha is for if it's not cleaning up the
detritus of all the stuff we've committed in the last 9 months. AIUI,
what we're trying to avoid is committing new stuff that may require
additional cleanup, not cleaning up the stuff we already did commit.
Once we get to beta I'll be less enthusiastic about making changes
like this, but I think most of the testing that will get done is still
in front of us.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Apr 3, 2011 at 1:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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.

> Yes, I think that's right. A related point is that the API for assign
> hooks is not consistent across all data types: string assign hooks can
> return a replacement value, whereas everyone else can only succeed or
> fail.

Right. In the original design we only foresaw the need to canonicalize
string values, so that's why it's like that. This change will make it
more consistent.

regards, tom lane

I wrote:
> 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)

After perusing the existing assign_hook functions, I have some further
thoughts about this proposal.

* Many of the existing assign hooks do a nontrivial amount of
computation (such as catalog lookups) to derive internal values from the
presented string; examples include assign_temp_tablespaces and
assign_timezone. A naive implementation of the above design would
require the assign_hook to repeat this computation after the check_hook
already did it, which of course is undesirable.

* Assign hooks that do catalog lookups need special pushups to avoid
having to do such lookups while restoring a previous GUC setting during
transaction abort (since lookups can't safely be performed in an aborted
transaction). Up to now we've used ad-hoc techniques for each such
variable, as seen for instance in assign_session_authorization. The
usual idea is to modify the original string to store additional data,
which then requires a custom show_hook to ensure only the original part
of the string is shown to users. The messiest aspect of this is that
it must be possible to reliably tell a modified string from original
user input.

I think that we can avoid the first problem and clean up the second
problem if we do this:

1. Code guc.c so that the check_hook is only applied to original user
input. When restoring a previous setting during abort (which
necessarily will have been passed through the check_hook at an earlier
time), only call the assign_hook.

2. Guarantee that the string pointer passed to the assign_hook is
exactly what was returned by the check_hook, ie, guc.c is not allowed
to duplicate or copy that string.

Given these rules, a check_hook and assign_hook could cooperate to store
additional data in what guc.c thinks is just a pointer to a string
value, ie, there can be more data after the terminating \0. The
assign_hook could work off just this additional data without ever doing
a catalog lookup. No special show_hook is needed.

Of course this only works for string GUC variables, but I'm finding it
hard to visualize a case where a bool, int, float, or enum GUC could
need a catalog lookup to interpret. We could possibly legislate that
all of these are separately malloc'd to allow the same type of trick to
be applied across the board, but I think that's overkill. We can just
tell people they must use a string GUC if they need hidden data.

This technique would need documentation of course, but at least it
*would* be documented rather than ad-hoc for each variable.

Another variant would be to allow the check_hook to pass back a separate
"void *" value that could be passed on to the assign_hook, containing
any necessary derived data. This is logically a bit cleaner, and would
work for all types of GUC variables; but it would make things messier in
guc.c since there would be an additional value to pass around. I'm not
convinced it's worth that, but could be talked into it if anyone feels
strongly about it.

Another thing I was reminded of while perusing the code is the comment
for GUC_complaint_elevel:

* At some point it'd be nice to replace this with a mechanism that allows
* the custom message to become the DETAIL line of guc.c's generic message.

The reason we invented GUC_complaint_elevel in the first place was to
avoid a change in the signatures of assign hooks. If we are making such
a change, now would be the time to fix it, because we're never gonna fix
it otherwise. I see a few ways we could do it:

1. Add a "char **errdetail" parameter to assign_hooks, which guc.c would
initialize to NULL before call. If the hook stores a non-null pointer
there, guc.c would include that string as errdetail. This is the least
effort from guc.c's viewpoint, but will be relatively painful to use
from the hook's standpoint, because it'd generally have to palloc some
space, or maybe even set up a StringInfo buffer to contain the generated
message.

2. Add a "char *errdetail" parameter to assign_hooks, which points at
a local-variable buffer in the calling routine, of a well-known size
(think GUC_ERRDETAIL_BUFSIZE macro in guc.h). Then hooks do something
like
snprintf(errdetail, GUC_ERRDETAIL_BUFSIZE, _("format"), ...);
to return an error detail string.

3. Create a function in guc.c for hooks to call, along the lines of
void GUC_assign_errdetail(const char *format, ...)
The simplest implementation of this would rely on the assumption that
assign-hook-calling isn't re-entrant, so it could just store the
formatted string in a static variable. That seems a bit ugly, but at
least the ugliness would be hidden in a well-defined place, where it
could be fixed locally if the assumption ever breaks down.

At this point I'm leaning to #3 but wondered if anyone would see it
as either overkill or too ugly. With any of these variants, we could
forget about my previous suggestion of adding an explicit elevel
parameter to assign hook calls, since the hooks would no longer need
to know the elog level to use anyway.

Comments?

regards, tom lane

On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> 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)
>
> After perusing the existing assign_hook functions, I have some further
> thoughts about this proposal.
>
> * Many of the existing assign hooks do a nontrivial amount of
> computation (such as catalog lookups) to derive internal values from the
> presented string; examples include assign_temp_tablespaces and
> assign_timezone.  A naive implementation of the above design would
> require the assign_hook to repeat this computation after the check_hook
> already did it, which of course is undesirable.
>
> * Assign hooks that do catalog lookups need special pushups to avoid
> having to do such lookups while restoring a previous GUC setting during
> transaction abort (since lookups can't safely be performed in an aborted
> transaction).  Up to now we've used ad-hoc techniques for each such
> variable, as seen for instance in assign_session_authorization.  The
> usual idea is to modify the original string to store additional data,
> which then requires a custom show_hook to ensure only the original part
> of the string is shown to users.  The messiest aspect of this is that
> it must be possible to reliably tell a modified string from original
> user input.
>
> I think that we can avoid the first problem and clean up the second
> problem if we do this:
>
> 1. Code guc.c so that the check_hook is only applied to original user
> input.  When restoring a previous setting during abort (which
> necessarily will have been passed through the check_hook at an earlier
> time), only call the assign_hook.
>
> 2. Guarantee that the string pointer passed to the assign_hook is
> exactly what was returned by the check_hook, ie, guc.c is not allowed
> to duplicate or copy that string.
>
> Given these rules, a check_hook and assign_hook could cooperate to store
> additional data in what guc.c thinks is just a pointer to a string
> value, ie, there can be more data after the terminating \0.  The
> assign_hook could work off just this additional data without ever doing
> a catalog lookup.  No special show_hook is needed.

The only thing this proposal has to recommend it is that the current
coding is even worse.

> Of course this only works for string GUC variables, but I'm finding it
> hard to visualize a case where a bool, int, float, or enum GUC could
> need a catalog lookup to interpret.  We could possibly legislate that
> all of these are separately malloc'd to allow the same type of trick to
> be applied across the board, but I think that's overkill.  We can just
> tell people they must use a string GUC if they need hidden data.
>
> This technique would need documentation of course, but at least it
> *would* be documented rather than ad-hoc for each variable.
>
> Another variant would be to allow the check_hook to pass back a separate
> "void *" value that could be passed on to the assign_hook, containing
> any necessary derived data.  This is logically a bit cleaner, and would
> work for all types of GUC variables; but it would make things messier in
> guc.c since there would be an additional value to pass around.  I'm not
> convinced it's worth that, but could be talked into it if anyone feels
> strongly about it.
>
> Another thing I was reminded of while perusing the code is the comment
> for GUC_complaint_elevel:
>
>  * At some point it'd be nice to replace this with a mechanism that allows
>  * the custom message to become the DETAIL line of guc.c's generic message.
>
> The reason we invented GUC_complaint_elevel in the first place was to
> avoid a change in the signatures of assign hooks.  If we are making such
> a change, now would be the time to fix it, because we're never gonna fix
> it otherwise.  I see a few ways we could do it:
>
> 1. Add a "char **errdetail" parameter to assign_hooks, which guc.c would
> initialize to NULL before call.  If the hook stores a non-null pointer
> there, guc.c would include that string as errdetail.  This is the least
> effort from guc.c's viewpoint, but will be relatively painful to use
> from the hook's standpoint, because it'd generally have to palloc some
> space, or maybe even set up a StringInfo buffer to contain the generated
> message.
>
> 2. Add a "char *errdetail" parameter to assign_hooks, which points at
> a local-variable buffer in the calling routine, of a well-known size
> (think GUC_ERRDETAIL_BUFSIZE macro in guc.h).  Then hooks do something
> like
>        snprintf(errdetail, GUC_ERRDETAIL_BUFSIZE, _("format"), ...);
> to return an error detail string.
>
> 3. Create a function in guc.c for hooks to call, along the lines of
>        void GUC_assign_errdetail(const char *format, ...)
> The simplest implementation of this would rely on the assumption that
> assign-hook-calling isn't re-entrant, so it could just store the
> formatted string in a static variable.  That seems a bit ugly, but at
> least the ugliness would be hidden in a well-defined place, where it
> could be fixed locally if the assumption ever breaks down.
>
> At this point I'm leaning to #3 but wondered if anyone would see it
> as either overkill or too ugly.  With any of these variants, we could
> forget about my previous suggestion of adding an explicit elevel
> parameter to assign hook calls, since the hooks would no longer need
> to know the elog level to use anyway.

Definitely +1 for #3.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Given these rules, a check_hook and assign_hook could cooperate to store
>> additional data in what guc.c thinks is just a pointer to a string
>> value, ie, there can be more data after the terminating \0. The
>> assign_hook could work off just this additional data without ever doing
>> a catalog lookup. No special show_hook is needed.

> The only thing this proposal has to recommend it is that the current
> coding is even worse.

Well, if you don't like that, do you like this one?

>> Another variant would be to allow the check_hook to pass back a separate
>> "void *" value that could be passed on to the assign_hook, containing
>> any necessary derived data. This is logically a bit cleaner, and would
>> work for all types of GUC variables; but it would make things messier in
>> guc.c since there would be an additional value to pass around. I'm not
>> convinced it's worth that, but could be talked into it if anyone feels
>> strongly about it.

If not, what do you suggest?

regards, tom lane

On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Apr 4, 2011 at 2:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Given these rules, a check_hook and assign_hook could cooperate to store
>>> additional data in what guc.c thinks is just a pointer to a string
>>> value, ie, there can be more data after the terminating \0.  The
>>> assign_hook could work off just this additional data without ever doing
>>> a catalog lookup.  No special show_hook is needed.
>
>> The only thing this proposal has to recommend it is that the current
>> coding is even worse.
>
> Well, if you don't like that, do you like this one?

To be clear, it's certainly an improvement over what we have now.

>>> Another variant would be to allow the check_hook to pass back a separate
>>> "void *" value that could be passed on to the assign_hook, containing
>>> any necessary derived data.  This is logically a bit cleaner, and would
>>> work for all types of GUC variables; but it would make things messier in
>>> guc.c since there would be an additional value to pass around.  I'm not
>>> convinced it's worth that, but could be talked into it if anyone feels
>>> strongly about it.

I haven't really got the mental energy to think through all of this
right now in detail, but I think that might be better. I think
there's more kludgery here than we're going to fix in one pass, so as
long as we're making improvements, I'm happy. Is there any case for
using a Datum rather than a void * so people can pack a short quantity
in directly without allocating memory, or are we expecting this to
always be (say) a struct pointer?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Another variant would be to allow the check_hook to pass back a separate
>> "void *" value that could be passed on to the assign_hook, containing
>> any necessary derived data. This is logically a bit cleaner, and would
>> work for all types of GUC variables; but it would make things messier in
>> guc.c since there would be an additional value to pass around. I'm not
>> convinced it's worth that, but could be talked into it if anyone feels
>> strongly about it.

> I haven't really got the mental energy to think through all of this
> right now in detail, but I think that might be better. I think
> there's more kludgery here than we're going to fix in one pass, so as
> long as we're making improvements, I'm happy. Is there any case for
> using a Datum rather than a void * so people can pack a short quantity
> in directly without allocating memory, or are we expecting this to
> always be (say) a struct pointer?

Well, I was intending to insist that the void* parameter point to a
single malloc'd block, so that guc.c could release it when the value was
no longer of interest by doing free(). If we don't say that, then we
are going to need a "free" hook for those objects, which is surely way
more notational overhead than is likely to be repaid for the occasional
cases where a single OID or whatever would be sufficient info.

regards, tom lane

On Mon, Apr 4, 2011 at 3:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Apr 4, 2011 at 2:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Another variant would be to allow the check_hook to pass back a separate
>>> "void *" value that could be passed on to the assign_hook, containing
>>> any necessary derived data.  This is logically a bit cleaner, and would
>>> work for all types of GUC variables; but it would make things messier in
>>> guc.c since there would be an additional value to pass around.  I'm not
>>> convinced it's worth that, but could be talked into it if anyone feels
>>> strongly about it.
>
>> I haven't really got the mental energy to think through all of this
>> right now in detail, but I think that might be better.  I think
>> there's more kludgery here than we're going to fix in one pass, so as
>> long as we're making improvements, I'm happy.  Is there any case for
>> using a Datum rather than a void * so people can pack a short quantity
>> in directly without allocating memory, or are we expecting this to
>> always be (say) a struct pointer?
>
> Well, I was intending to insist that the void* parameter point to a
> single malloc'd block, so that guc.c could release it when the value was
> no longer of interest by doing free().  If we don't say that, then we
> are going to need a "free" hook for those objects, which is surely way
> more notational overhead than is likely to be repaid for the occasional
> cases where a single OID or whatever would be sufficient info.

OK. Please comment the crap out of whatever you do, or maybe even add
a README. This stuff is just a bit arcane, and guideposts help a lot.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> OK. Please comment the crap out of whatever you do, or maybe even add
> a README. This stuff is just a bit arcane, and guideposts help a lot.

We already have a README for that ;-). PFA, a patch to
src/backend/utils/misc/README describing the proposed revised API.
If nobody has any objections, I'll get on with making this happen.

regards, tom lane

On Mon, Apr 4, 2011 at 4:57 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> OK.  Please comment the crap out of whatever you do, or maybe even add
>> a README.  This stuff is just a bit arcane, and guideposts help a lot.
>
> We already have a README for that ;-).  PFA, a patch to
> src/backend/utils/misc/README describing the proposed revised API.
> If nobody has any objections, I'll get on with making this happen.

Looks reasonable to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> OK. Please comment the crap out of whatever you do, or maybe even add
>> a README. This stuff is just a bit arcane, and guideposts help a lot.

> We already have a README for that ;-). PFA, a patch to
> src/backend/utils/misc/README describing the proposed revised API.
> If nobody has any objections, I'll get on with making this happen.

Attached is a WIP patch for this. It turned out to be a lot more work
than I thought, because there are a lot more assign hooks than I'd
remembered, and they all needed to be looked at. But I'm pretty happy
with the results: things seem noticeably cleaner, and there are way
fewer strange hacks like having magical things happen when timezone is
set to "UNKNOWN". And it does fix Bernd's original complaint :-)

The patch is not complete yet; I need to refactor some code in timezone
abbreviation support and encoding-conversion support so that the assign
hooks don't have to duplicate work (and risk of failure) from the check
hooks. Also I've not touched contrib or the optional PLs yet.

My original idea of just providing GUC_check_errdetail() to check hooks
proved inadequate: there were several places where useful hints were
being offered, and some where it seemed appropriate to override GUC's
primary message string too. So now there are also GUC_check_errmsg()
and GUC_check_errhint(). A few places were reporting SQLSTATE codes
different from ERRCODE_INVALID_PARAMETER_VALUE; in particular the
routines associated with transaction properties sometimes reported
ERRCODE_ACTIVE_SQL_TRANSACTION instead. As the patch stands it doesn't
maintain that behavior, but just reports ERRCODE_INVALID_PARAMETER_VALUE
in all check-hook failure cases. We could preserve the old errcodes
with another static variable and another function GUC_check_errcode(),
but I'm not sure if it's worth the trouble. Thoughts?

Also, I'm very seriously considering dropping the provision that says
assign hooks can return a bool success flag, but instead having them
return void and just saying they're not supposed to fail. The only ones
that can ever return false at the moment are the ones for timezone
abbrev and encoding, and as stated above that's only for lack of
refactoring of their infrastructure. I think allowing failure just
complicates matters for guc.c without much redeeming social benefit.

Any comments? I'm hoping to push this through to commit today or
tomorrow, so I can get back to fooling with collations.

regards, tom lane