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
>

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

> SET PERSISTENT log_statement_stats = true; restart; SET PERSISTENT
log_statement_stats = false; SET PERSISTENT log_parser_stats = true; ERROR

As we are not loading configuration at end of each command, so this will be
a valid case.

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

> Transaction A: SET PERSISTENT blub = 'bar'; Transaction B: SET PERSISTENT
blub FROM CURRENT;

This behavior is not only for CURRENT, but any other setting will have same
behavior.
It just retains the last set value for any parameter before restart.

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

IMHO, let's try to see if there are any fundamental problems with this
Approach, then it
will make much more sense to change the Approach.

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

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 should add such calls in Catch block.

If you are very sure that such scenario exist where it can reach without
transaction then
we can handle it in above way.

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

Fixed.

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

Changed the line to "faraming the name for".
Here framing means constructing the variable with different inputs.

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

Can you please elaborate a little bit more on this as I am not able to
clearly
Understand.

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

The name passed in via the VariableSetStmt always is lowercase, which will
be same
same as per current postgresql.conf.
Doing it is very simple, but I am not able to understand what is the exact
problem you are
seeing in the current way.

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

I have changed few things in validate_conf_option:
a. comments on top of function
b. memory was not getting freed for one variable.

I have tried to unify, it doesn't seem to 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.

With Regards,
Amit Kapila.