Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

At this point SET PERSISTENT is looking complete enough that some parts
of it might be committed. There's been a lot of useful progress in
nailing down the early/obvious problems and what fundamental approach
makes sense. Accepting the whole thing still seems a bit too invasive
to chew on this late in 9.3 though. Unfortunately it's not until
everything goes in that a really useful change happens.

What I'll mainly talk about here is how to break this into smaller
pieces to consider as potential future commits, hopeful ones that might
all go in early for 9.4. There's no identified committer for this
feature yet. I think which committer (or committers, plural, given the
number of pieces) is willing to take on the GUC changes, and how
aggressively they want to consider the pieces of this, is going to
determine the timeline for when the moving parts of this idea are adopted.

As for the code that seems high risk to me, there's a whole class of
subtle problems possible in the memory management in particular. The
first stress test I did (just to try and break the feature) shook out
what looks so far like a memory allocation concern in the existing
codebase, before this new feature is even considered. That didn't feel
like a good sign to me.

Stepping back, I see four potential commits worth of changes here now,
and I would consider them in this order:

1) Fix for pg_reload_conf memory context growth problem uncovered during
testing. This is a small patch and minor bug fix that could even go
into 9.3. The problem is not so severe it seems worth the disruption of
backporting to earlier versions, and it wouldn't really hurt much to
kick the problem forward to 9.4.

2) Change the default postgresql.conf to include a config directory.
This has been one of my soapbox positions for a while now, and I think
this feature is working well enough now to have proven the form of
re-arrangement does something useful. This is a small change to the
PostgreSQL code that will ripple out to impact packaging. It would be
possible to do this part and face the main disruption for 9.3 even if
the exact SET PERSISTENT implementation is pushed forward. If there was
not much else going on with packaging right now, I might even advocate
that myself. Unfortunately, right now "not much else going on" is the
exact opposite of the situation all the packagers are in. It's just a
bad time to talk about it.

3) Rearrangement of GUC validation into validate_conf_option function.
This is ~400 lines of code that could be changed as a zero-impact
refactoring, one that just makes the SET PERSISTENT option easier to
implement.

4) Implementation of SET PERSISTENT into a single file,
config/persistent.auto.conf. As written, a reasonable amount of error
checking is done to reduce the odds of someone breaking its
implementation without realizing it. The responsibility of activating
the new configuration using pg_reload_conf is pushed toward the user,
documented but without an explicit warning.

All of these changes have different risk vs. reward trade-offs in them.
I'd guess the validation_config_option change (3) is the riskiest of
this bunch, because it could easily break the standard GUC path in a
major way even for people who don't use the feature. It's not that much
code, but it is going to take a good bit of committer level review to
accept due to its risk.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com