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@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com