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

On Monday, April 01, 2013 1:03 PM Greg Smith wrote:
> 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.

I think in that case we can have 3 separate patches
1. Memory growth defect fix
2. Default postgresql.conf to include config directory and SET Persistent
into single file implementation
3. Rearrangement of GUC validation into validate_conf_option function.

As already there is review happened for point 2 and point 1 is an existing
code defect fix, so in my opinion
Patch 1 and 2 should be considered for 9.3.

With Regards,
Amit Kapila.