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

On 2013-02-12 14:57:51 +0530, Amit Kapila wrote:
> On Tuesday, February 12, 2013 11:24 AM Boszormenyi Zoltan wrote:
> > 2013-02-12 04:54 keltezéssel, Amit Kapila írta:
> > > On Tuesday, February 12, 2013 12:54 AM Andres Freund wrote:
> > > Zoltan has reviewed this patch very thoroughly

Sorry, the patch as referenced in the commitfest app is *far* from being
thoroughly reviewed. Its not even remotely ready for commit. I
absolutely don't want to say *anything* against Zoltans reviews or
such. I have looked over the patch before as well, but I found several
severe issues in a 5min look.

> > > I have never seen a comment
> > > from him that the current
> > > Patch (one-file-all-settings) approach is not as good as having
> > > one-file-per-setting approach.
> > > Zoltan, please correct me, If I am wrong.
> >
> > I cannot recall arguing for the one-file-per-GUC way.
> > But I haven't re-read my mails in this thread, either.

Zoltan, mis(read|remembered) something, sorry.

> Thanks for confirmation. I also don't think if ever have argument over this
> approach.

No idea what youre trying to say here though?

> > >> Check
> > >> http://www.postgresql.org/message-
> > >> id/20130126162728(dot)GA5482(at)awork2(dot)anarazel(dot)de
> > >> and related messages for some of the problems. Most of which are
> > >> unhandled in the current patch, i.e. currently you *will* loose
> > changes
> > >> made in concurrent sessions.
> >
> > This mail lists this order for the single file approach:
> >
> > > 1) exlusive lock
> > > 2) reload config file (to update in memory structures)
> > > 3) check new variable
> > > 4) write config file (needs to be done atomically)
> > > 5) optionally reload config file
> > > 6) reload lock
> >
> > The patch does it this way:
> > 1. check new variable. No problem with that, validation for proper GUC
> > name,
> > type of the value, etc. can be done outside the lock.
> > 2. grab lock
> > 3. parse the config file
> > 4. write the new config file
> > 5. release lock

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.
2) You need to apply the current config file for exactly the same
reason before checking the new value. Also
validate_conf_option/find_option doesn't work appropriately without
an up2date config file. E.g. CURRENT does absurd things without but I
think there are other cases as well.

I am not saying its impossible to do the one-file approach correctly, I
just think its harder while not being more useful.

> > Reloading the config file is intentionally not done, it's even
> > documented. You can do SELECT pg_reload_conf() after SET PERSISTENT
> > if you need it.

Not being done and it being documented as not doing so doesn't make it
correct :P
I think a SET having no immediate results is confusing. Especially if I
am right and we do need to apply previous config changes before doing
the next SET. But I don't have *that* strong feelings about it.

> > Specifically, LWLockAcquire() is called first, then ParseConfigFp()
> > in a PG_TRY() block, so reading the original postgresql.auto.conf
> > is serialized. No chance to lose changes done in parallel.

Not a fundamental issue, but I just noticed LWLockRelease isn't called
in the PG_CATCH branch in set_config_file. There's also an ereport()
which needs to be moved into the PG_TRY to avoid exiting with a held
lock.

I think you also forgot to adjust copyfuncs.c et al for your
VariableSetStmt change (addition of is_persistent).

You should also check for GUC_DISALLOW_IN_FILE in validate_conf_option,
its not the same as PGC_INTERNAL.

What do you mean by "framing" a variable? Surrounding it by ""?

It might be worth adding a check that setting and especially resetting a
user-defined variable works correctly. I.e. persistently setting foo.bar
= 'blub' and then resetting it again.

You cannot use the name passed in via the VariableSetStmt in
set_config_file, you should use the one in 'record' as returned by
find_option (via validate_conf_option) as it handles the correct
name. Otherwise you will get into problems with stuff like TimeZone and
similar.

I think validate_conf_option duplicates far too much code. You need to
unify parts of set_config_option with validate_conf_option.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services