On 2013-02-12 10:45:28 -0500, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2013-02-12 20:19:43 +0530, Amit Kapila wrote:
> >> On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
> >>> 1) You need to grab the lock before the value is checked since some
> >>> variables are interdependent (e.g. log_statement_stats, wal_level,
> >>> archive_mode) and thus the check needs to be made after preventing
> >>> concurrent changes.
>
> >> This can happen if we do any SIGHUP after the command, otherwise it will
> >> have old value only.
>
> > Yes, and thats a problem imo.
>
> That sounds to me like an entirely unreasonable requirement to put on
> the patch. There is no way to positively guarantee that a value with
> interdependencies will load successfully into other sessions, so why
> try to enforce it at all? I note also that trying to make the value
> active in the current session doesn't necessarily result in a meaningful
> configuration --- what if there's an active session-level SET, for
> instance? You can't just override that.
Fair point. The reason I think its reasonable to put more effort into
validation is that I am pretty sure that users will expect that its hard
to get into a state where the server doesn't restart anymore. And
getting 80% of that is better than 50%.
And it seems required to reload the configuration anyway to make FROM
CURRENT work properly. Obviously we can also just document that it
throws away previously set values.
There's also the point that I find it suprising behaviour that SET
PERSISTENT doesn't change the active value unless a manual
pg_reload_conf() is issued.
If we decide that those are not important enough warts to care, yes,
there is no reason for any complication here.
> (I've said this before, but this discussion smells of overdesign every
> time I look into the thread. I can't help thinking this is a big patch
> with a small patch struggling to get out.)
To be honest, I don't really think thats fair to the patch. I don't see
much that can be made smaller as long as the "one file for all
persistent settings" dogma is upheld which more people seem to vote for
(exluding me).
There's the duplication around validate_conf_option and some comment
cleanup, but otherwise there really isn't that much code that looks
superflous for a simple version?
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
