On Tue, Jul 29, 2014 at 3:33 AM, Josh Berkus <j...@agliodbs.com> wrote: > On 07/28/2014 11:03 AM, Fujii Masao wrote: >> On Sat, Jul 26, 2014 at 1:07 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: >>> On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao <masao.fu...@gmail.com> wrote: >>>> On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila <amit.kapil...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers