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

On Tuesday, February 12, 2013 4:55 PM Andres Freund wrote:
> 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?

Forget it, this is not important.
Just wanted to say, there is no confusion about approach between 2 of us.

> > > >> 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.

This can happen if we do any SIGHUP after the command, otherwise it will
have old value only.

> 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.

At this moment, I am not able to think through this, could you explain by
small example.

I am thinking that shall we remove check hook function and do other
validation only, as this will be done at time
of reload, similar to what will get done when user manually edits the
postgresql.conf file.

> 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.

I don't think any such expectation should be there, as with this feature
(SET PERSISTENT),
we will allow user to change the settings in file with command instead of
manually editing the file.

> > > 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 rollback/abort transaction due to error will handle release of
locks.

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

It is there in _copyVariableSetStmt() function.

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

I shall update the patch to address this issue.

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

Sorry, I am not able to find "framing" in quotes.

> 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.

I shall update the patch to address this issue.

> 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 shall check.

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

I had thought of doing this initially, but found it might be little tricky
as duplicate part is not one single chunk.
I shall check, if it can be done in a reasonable way.

Thank you for feedback.

With Regards,
Amit Kapila.