Re: GUC assign hooks (was Re: wal_buffers = -1 and SIGHUP)

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