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

2013-03-13 21:28 keltezéssel, Boszormenyi Zoltan írta:
> 2013-03-13 13:45 keltezéssel, Andres Freund írta:
>> On 2013-03-13 09:09:24 +0100, Boszormenyi Zoltan wrote:
>>> 2013-03-13 07:42 keltezéssel, Craig Ringer írta:
>>>> On 03/12/2013 06:27 AM, Craig Ringer wrote:
>>>>>>> Think also about the case where someone wants to change multiple
>>>>>>> values together and having just some set and not others would be
>>>>>>> inconsistent.
>>>>> Yeah, that's a killer. The reload would need to be scheduled for COMMIT
>>>>> time, it can't be done by `SET PERSISTENT` directly.
>>>> Thinking about this some more, I'm not sure this is a good idea.
>>>>
>>>> Right now, SET takes effect immediately. Always, without exception.
>>>> Delaying SET PERSISTENT's effects until commit would make it
>>>> inconsistent with SET's normal behaviour.
>>>>
>>>> However, *not* delaying it would make it another quirky
>>>> not-transactional not-atomic command. That's OK, but if it's not going
>>>> to follow transactional semantics it should not be allowed to run within
>>>> a transaction, like VACUUM .
>>>>
>>>> Writing the changes immediately but deferring the reload until commit
>>>> seems to be the worst of those two worlds.
>>> I was thinking about it a little. There is a hook that runs at the end
>>> of (sub-)transactions. It can be abused for this purpose to make
>>> SET PERSISTENT transactional. The subtransactions can also stack
>>> these settings, by forgetting settings upon ROLLBACK [ TO SAVEPOINT ]
>>> and overwriting previous settings upon COMMIT and RELEASE SAVEPOINT.
>>> All it needs is a list and maintaining intermediate pointers when entering
>>> into a new level of SAVEPOINT. The functions to register such hooks are
>>> in src/include/access/xact.h:
>>>
>>> extern void RegisterXactCallback(XactCallback callback, void *arg);
>>> extern void UnregisterXactCallback(XactCallback callback, void *arg);
>>> extern void RegisterSubXactCallback(SubXactCallback callback, void *arg);
>>> extern void UnregisterSubXactCallback(SubXactCallback callback, void *arg);
>> (sub)xact commit/abort already calls AtEOXact_GUC(commit, nestlevel). So you
>> wouldn't even need that.
>
> Yes, thanks.
>
>>   It seems we could add another value to enum
>> GucStackState, like GUC_SET_PERSISTENT - and process those only if commit &&
>> nestlevel == 1.
>
> Maybe it's not needed, only enum GucAction needs a new
> GUC_ACTION_PERSISTENT value since that's what has any
> business in push_old_value(). Adding two new members to
> GucStack like these are enough
>     bool has_persistent;
>     config_var_value persistent;
> and SET PERSISTENT can be treated as GUC_SET. push_old_value()
> can merge GUC values set in the same transaction level.

It seems both were needed. The attached patch makes
SET PERSISTENT transactional and uses one setting per file.
It uses the currently existing parsing and validating code
and because of this, the patch is half the size of v12 from Amit.

Best regards,
Zoltán Böszörményi

>
>> Everytime you see one with commit && nestlevel > 1 you put them into them into
>> the stack one level up.
>>
>> This seems like its somewhat in line with the way SET LOCAL is implemented?
>>
>>> On the other hand, it would make a lot of sense to implement it
>>> as one setting per file or extending the  code to allow modifying
>>> settings in bulk. The one setting per file should be easier and
>>> it would also allow extensions to drop their settings automatically
>>> into the automatic config directory. I don't know who mentioned
>>> this idea about extensions but it also came up a while back.
>> I still think one-setting-per-file is the right way to go, yes.
>>
>> Greetings,
>>
>> Andres Freund
>>
>
>


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/