Re: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review] - Mailing list pgsql-hackers

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.

We cannot directly call LWLockRelease in catch block as it will lead to
Assert failure.
The reason is errfinish will set InterruptHoldOffCount to 0, and now in
LWLockRelease, 
when it will call RESUME_INTERRUPTS it will check InterruptHoldOffCount
which leads to Assertion failure.

To handle it, we need to call HOLD_INTERRUPTS before it similar to
LWLockReleaseAll().
So I am not sure if we are not sure of scenario, we should add such calls in
Catch block.

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

I have tried it, it will not be straight forward, please see some problems
which we need to handle:
1. few checks like GUC_DISALLOW_IN_FILE will be different for persistent
function   - we can address this by passing ispersistent flag 
2. Cannot call validate_config_option directly, because the below part of
code is   tightly coupled.   a. if (value)      ..      else if()      ..      else 
     Now if the value part is already checked in validate_config_option,
adjusting this loop will be tricky.   b. some of the variable like newextra are required outside function, so
extra parameter      needs to be passed.   c. memory free issues, as newextra will be required in function
set_config_option, however set_config_file      doesn't need it, so it would be better to free that memory inside
validate_config_option 

3. Extra parameter elevel would be required in validate_conf_option. 

4. Function set_config_option is used from many places, so impact can be
higher if we   do too many adjustments. 

Considering all above, I think it might not be advisable to remove common
code of validate_conf_option


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

What exactly you want in this handling?

With Regards,
Amit Kapila.