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

On Tuesday, February 12, 2013 9:02 PM Andres Freund wrote:
> On 2013-02-12 20:19:43 +0530, Amit Kapila wrote:
> > 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:
> > > > > 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.
>
> Yes, and thats a problem imo.
>
> SET PERSISTENT log_statement_stats = true;
> restart;
> SET PERSISTENT log_statement_stats = false;
> SET PERSISTENT log_parser_stats = true;
> ERROR...
>
> thats why I think the config file needs to be processed.
>
> > > 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.
>
> The most trivial one I can think of is:
>
> Transaction A: SET PERSISTENT blub = 'bar';
> Transaction B: SET PERSISTENT blub FROM CURRENT;
>
> > 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.
>
> Why? The user isn't editing the file by hand for a reason.
>
> > > 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.
>
> I don't see why that follows. The users *do* want something different,
> otherwise they would hand-edit 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.
>
> Yes, in a proper transaction abort this is done but for a utility
> command it might be possible to get there without a StartTransaction
> being done. I don't immediately see how, but I wouldn't want to rely on
> it, especially as doing it is simple.

True, doing it is simple but without reason can raise questions again like
why is it released when it
will be done as part of abort.
I will think more on it to see if there can be any way it can reach without
StartTransaction.

> > > 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.
>
> Oh, sorry, skipped over it somehow.
>
> > > What do you mean by "framing" a variable? Surrounding it by ""?
> >
> > Sorry, I am not able to find "framing" in quotes.
>
> The quotes were just there to quote the word ;). I was referring to the
> following comment:
>
> + /*
> + * The "auto.conf.d" directory should follow the postgresql.conf
> file
> + * directory not the data_directory. The same is validated while
> parsing
> + * the postgresql.conf configuration file. So while framing the
> + * postgresql.auto.conf and it's temp file we need to follow the
> + * postgresql.conf file directory.
> + */

Shall we change it as one of below:
a. "framing the path for postgresql.auto.conf ... "
b. "framing the name for postgresql.auto.conf ... "
c. "generating the path for postgresql.auto.conf ..."
d. any other suggestion?

> > > 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.
>
> I think calling validate_conf_option() from set_config_option and
> removing the now redundant validation from there should do the trick.

Okay.

I have intentionally not replied some parts of message as I want to wait if
there can be conclusion
for the points you have mentioned in another mail.

With Regards,
Amit Kapila.