Re: postgresql.auto.conf and reload

On Tue, Jul 29, 2014 at 3:33 AM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 07/28/2014 11:03 AM, Fujii Masao wrote:
>> On Sat, Jul 26, 2014 at 1:07 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>> On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>>>> wrote:
>>>>> Okay. As mentioned upthread, I have fixed by ensuring that for duplicate
>>>>> config params, retain only which comes later during parsing.
>>>>> I think it might have been bit simpler to fix, if we try to fix after
>>>>> parsing
>>>>> is complete, but I think for that we might need to replicate the logic
>>>>> at multiple places.
>>>>
>>>> ISTM that the patch works fine. Only concern is that the logic needs
>>>> O(n^2) comparison, which may cause performance problem. But
>>>> "n" in O(n^2) is the number of uncommented parameters and I don't
>>>> think it's so big, ISTM I can live with the logic...
>>>
>>> Thanks for reviewing the patch. I also think that having O(n^2)
>>> comparisons should not be a problem in this logic as it will be processed
>>> only during load/parse of config file which we don't do in performance
>>> sensitive path.
>>
>> Yep.
>>
>> There is other side effect on this patch. With the patch, when reloading
>> the configurartion file, the server cannot warm an invalid setting value
>> if it's not the last setting of the parameter. This may cause problematic
>> situation as follows.
>>
>> 1. ALTER SYSTEM SET work_mem TO '1024kB';
>> 2. Reload the configuration file ---> success
>> 3. Then, a user accidentally adds "work_mem = '2048KB'" into postgresql.conf
>> The setting value '2048KB' is invalid, and the unit should be
>> 'kB' instead of 'KB'.
>> 4. Reload the configuration file ---> success
>> The invalid setting is ignored because the setting of work_mem in
>> postgresql.auto.conf is preferred. So a user cannot notice that
>> postgresql.conf
>> has an invalid setting.
>> 5. Failover on shared-disk HA configuration happens, then PostgreSQL fails to
>> start up because of such an invalid setting. When PostgreSQL
>> starts up, the last
>> setting is preferred. But all the settings are checked.
>>
>> Can we live with this issue?
>
> I'd think so, yes. That's pretty extreme corner-case.

Yes, that's corner-case. But I'd like to adopt the safer approach if we can
implement that easily.

The patch chooses the last settings for every parameters and ignores the
former settings. But I don't think that every parameters need to be processed
this way. That is, we can change the patch so that only PGC_POSTMASTER
parameters are processed that way. The invalid settings in the parameters
except PGC_POSTMASTER can be checked by pg_ctl reload as they are now.
Also this approach can reduce the number of comparison to choose the
last setting, i.e., "n" in O(n^2) is the number of uncommented *PGC_POSTMASTER*
parameters (not every parameters). Thought?

Regards,

--
Fujii Masao