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

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.

> > 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.
+ */

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

Greetings,

Andres Freund

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