Thread: postgresql.auto.conf and reload

I've just run into this:

$ psql -p 5433   (that port is configured in postgresql.conf)
# alter system set port = 5494;

... restart the server

$ psql -p 5494
# select pg_reload_conf();

2014-06-25 14:22:07 CEST [11297-4] LOG:  received SIGHUP, reloading configuration files
2014-06-25 14:22:07 CEST [11297-5] LOG:  parameter "port" cannot be changed without restarting the server
2014-06-25 14:22:07 CEST [11297-6] LOG:  configuration file "/etc/postgresql/9.4/main/postgresql.conf" contains errors;
unaffectedchanges were applied
 

I think reloading shouldn't be nagging me about parameters in
postgresql.conf that are going to be overwritten in
postgresql.auto.conf. Reloading again will issue the warnings again...

It's true that my config has two contradicting settings in there now,
but if that's not an issue at server start, it shouldn't be one for
reloading either.

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

Hi,

On Wed, 2014-06-25 at 18:42 +0530, Amit Kapila wrote:
> This will happen without Alter System as well, if you change
> the value of port in postgresql.conf and try to load conf file with
> SIGHUP. You cannot reload PGC_POSTMASTER parameters without server
> restart.

Ok, but Christoph already started the server (successfully) with port
5494, so the pg_reload_conf() should ignore it in the next attempt.

Regards,
--
Devrim GÜNDÜZ
Principal Systems Engineer @ EnterpriseDB: http://www.enterprisedb.com
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

Re: Amit Kapila 2014-06-25 <CAA4eK1Log98jvFOV9wzTqpCdEWJa+5JR54TTpkiZ3XBnGJydLA@mail.gmail.com>
> On Wed, Jun 25, 2014 at 6:11 PM, Christoph Berg <cb@df7cb.de> wrote:
> >
> > I've just run into this:
> >
> > $ psql -p 5433   (that port is configured in postgresql.conf)
> > # alter system set port = 5494;
> >
> > ... restart the server
> >
> > $ psql -p 5494
> > # select pg_reload_conf();
> >
> > 2014-06-25 14:22:07 CEST [11297-4] LOG:  received SIGHUP, reloading
> configuration files
> > 2014-06-25 14:22:07 CEST [11297-5] LOG:  parameter "port" cannot be
> changed without restarting the server
> > 2014-06-25 14:22:07 CEST [11297-6] LOG:  configuration file
> "/etc/postgresql/9.4/main/postgresql.conf" contains errors; unaffected
> changes were applied
> 
> This will happen without Alter System as well, if you change
> the value of port in postgresql.conf and try to load conf file with SIGHUP.
> You cannot reload PGC_POSTMASTER parameters without server restart.

That's not the issue. I did restart the server, which didn't log any
problems, yet reload keeps complaining indefinitely. This makes ALTER
SYSTEM not-so-nice-to-use to override parameters already set in
postgresql.conf.

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

Re: Amit Kapila 2014-06-25 <CAA4eK1+F9ZtoGvVw-WYj2+vT0K8_JXTziqHp8iVB7wdo1w1Rkw@mail.gmail.com>
> The main reason behind such such a behaviour after restart is
> that there are duplicate entries, one in postgresql.conf and
> another in postgresql.conf.  It always first read postgresql.conf
> and then .auto file and applies the changed parameters one by one,
> so when it reads a different value than current setting, it can lead
> to such messages.  During processing of config files it doesn't try
> to eliminate duplicate entries.
> 
> You can observe same behaviour incase you have another conf
> file (special.conf, containing conflicting settings) and include that
> in postgresql.conf.

Or even two statements for the same guc in postgresql.conf itself.

> One way could be that while processing if we could eliminate
> duplicate entries, then it will not lead to such messages, but I think
> that is existing mechanism and not introduced by Alter System,
> so changing just for Alter System might impact some existing users.

Yes, it's the same mechanism as before, but now we have a tool that
was specifically meant to be used to set and override postgresql.conf
parameters, so duplicate entries in postgresql.conf and
postgresql.auto.conf are no longer user error (or weirdness), but
expected. The system should deal with it.

> I think maintaining values both in postgresql.conf and by Alter System
> is not advisable.

Possibly, but then the system should be warning about all options, not
just the restart-only ones. And it should warn at startup, not at
reload time.

> I am not sure if this addresses your concern completely, but I thinking
> changing some existing mechanism (maintaining duplicate entries during
> processing of config files) at this point might be risky.

Not sure how intrusive a fix would be - collect all settings during
config parse, and only warn once everything has been seen, instead of
emitting warnings line-by-line.

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

Re: Amit Kapila 2014-06-26 <CAA4eK1+mUTjc=GXJK3bYtSwV2BmBni=PHEvBqLQkhdUV9cWd1A@mail.gmail.com>
> On Wed, Jun 25, 2014 at 7:52 PM, Christoph Berg <cb@df7cb.de> wrote:
> > Re: Amit Kapila 2014-06-25 <
> CAA4eK1+F9ZtoGvVw-WYj2+vT0K8_JXTziqHp8iVB7wdo1w1Rkw@mail.gmail.com>
> >
> > > I think maintaining values both in postgresql.conf and by Alter System
> > > is not advisable.
> >
> > Possibly, but then the system should be warning about all options, not
> > just the restart-only ones. And it should warn at startup, not at
> > reload time.
> 
> How about adding a note in Alter System so that users are aware of
> such behaviour and can ensure that they don't have duplicate entries?

If the behavior isn't going to change, that issue need to be
documented, sure.

> Clearly such warnings indicate that there are conflicting settings, so
> user can take appropriate action to avoid it.

I don't think conflicting settings in postgresql.auto.conf are a user
error, or misconfiguration. They are just normal use of ALTER SYSTEM.
Of course the user might want to eventually consolidate their config,
but we shouldn't treat the situation as bogus.

Frankly what bugs me most about this is that the warnings occur only
at reload time, not at startup. If the server thinks something is
wrong with my config, it should tell me rightaway.

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

Amit Kapila <amit.kapila16@gmail.com> writes:
> Please find the patch attached to address the above concern.  I have
> updated docs, so that users can be aware of such behaviour.

I'm in the camp that says that we need to do something about this, not
just claim that it's operating as designed.  AFAICS, the *entire* argument
for having ALTER SYSTEM at all is ease of use; but this behavior is not
what I would call facilitating ease of use.  In particular, if you are
conveniently able to edit postgresql.conf, what the heck do you need
ALTER SYSTEM for?

One possible answer is to modify guc-file.l so that only the last value
supplied for any one GUC gets processed.  The naive way of doing that
would be O(N^2) in the number of uncommented settings, which might or
might not be a problem; if it is, we could no doubt devise a smarter
data structure.
        regards, tom lane

Re: Amit Kapila 2014-07-06 <CAA4eK1K=2WCD5ur8c-34NOw+XKg57Q4k0SajwSQXcwciD-=+0w@mail.gmail.com>
> Another could be that during initdb all the uncommented settings be
> written to postgresql.auto.conf file rather than to postgresql.conf.
> I think we can do this by changing code in initdb.c->setup_config().
> This will ensure that unless user is changing settings in a mixed way
> (some by Alter System and some manually by editing postgresql.conf),
> there will no such problem.

That will make editing postgresql.conf totally useless, because users
will always need to check if their new value isn't overridden by
something in auto.conf. This will affect *all* users, as everyone
needs to bump shared_buffers, which is set by initdb. We'd get 10^X
complaints from admins who got confused where their config actually
is.

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

On Sat, Jul 5, 2014 at 10:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> Please find the patch attached to address the above concern.  I have
>> updated docs, so that users can be aware of such behaviour.
>
> I'm in the camp that says that we need to do something about this, not
> just claim that it's operating as designed.  AFAICS, the *entire* argument
> for having ALTER SYSTEM at all is ease of use; but this behavior is not
> what I would call facilitating ease of use.  In particular, if you are
> conveniently able to edit postgresql.conf, what the heck do you need
> ALTER SYSTEM for?
>
> One possible answer is to modify guc-file.l so that only the last value
> supplied for any one GUC gets processed.  The naive way of doing that
> would be O(N^2) in the number of uncommented settings, which might or
> might not be a problem; if it is, we could no doubt devise a smarter
> data structure.

I've been really annoyed by the current behavior even on older
releases because I have a script, which I use frequently, that does
this:

initdb
cat >> $PGDATA/postgresql.conf <<EOM
shared_buffers=8GB
another setting
another setting
another setting
EOM

Now, sometimes while experimenting, I will change a setting in
postgresql.conf and reload the config.  At which point it complains
that it can't change shared_buffers at runtime.  It does this because
the line from initdb is in the file, and so is the one I added.  But
I'm *not* trying to change shared_buffers.  Sure, the first value in
the config file doesn't match the current value, but the *last* line
in the config file is the one that's supposed to control, so why is it
complaining about the earlier line?

I haven't looked at the code in this area too carefully, but it seems
to me like the flow ought to be:

1. Read all of the config files and determine what the final value
present in each config file is.
2. *Then*, in a second pass, enforce requirements like "can't be
changed except at server start".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 07/08/2014 10:07 AM, Robert Haas wrote:
> I haven't looked at the code in this area too carefully, but it seems
> to me like the flow ought to be:
> 
> 1. Read all of the config files and determine what the final value
> present in each config file is.
> 2. *Then*, in a second pass, enforce requirements like "can't be
> changed except at server start".

+1

This would also make conf.d much more useful; I wouldn't have to worry
as much about overlapping config settings.

Sounds like a 9.5 feature, though.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 07/06/2014 01:27 AM, Christoph Berg wrote:
> Another could be that during initdb all the uncommented settings be
>> written to postgresql.auto.conf file rather than to postgresql.conf.
>> I think we can do this by changing code in initdb.c->setup_config().
>> This will ensure that unless user is changing settings in a mixed way
>> (some by Alter System and some manually by editing postgresql.conf),
>> there will no such problem.

There is no reasonable way for us to prevent issues for users who are
manually changing both pg.conf and pg.auto.conf.  Users should stick to
one or the other method of management (or, thirdly, using conf.d); if
they mix methods, we can't prevent confusion at restart time and we
shouldn't try.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 09/07/14 05:13, Josh Berkus wrote:
> On 07/06/2014 01:27 AM, Christoph Berg wrote:
>> Another could be that during initdb all the uncommented settings be
>>> written to postgresql.auto.conf file rather than to postgresql.conf.
>>> I think we can do this by changing code in initdb.c->setup_config().
>>> This will ensure that unless user is changing settings in a mixed way
>>> (some by Alter System and some manually by editing postgresql.conf),
>>> there will no such problem.
>
> There is no reasonable way for us to prevent issues for users who are
> manually changing both pg.conf and pg.auto.conf.  Users should stick to
> one or the other method of management (or, thirdly, using conf.d); if
> they mix methods, we can't prevent confusion at restart time and we
> shouldn't try.
>


Yes, but even well behaved users will see this type of error, because 
initdb uncomments certain values (ones that are dead certs for being 
changed via ALTER SYSTEM subsequently like shared_buffers), and then - 
bang! your next reload gets that "your postgresql.conf contains errors" 
message.

Regards

Mark

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Wed, Jul 9, 2014 at 6:40 AM, Mark Kirkwood <mark.kirkwood@catalyst.net.nz>
> wrote:
>> Yes, but even well behaved users will see this type of error, because
>> initdb uncomments certain values (ones that are dead certs for being
>> changed via ALTER SYSTEM subsequently like shared_buffers), and then -
>> bang! your next reload gets that "your postgresql.conf contains errors"
>> message.

> That is the reason, why I have suggested up-thread that uncommented
> values should go to postgresql.auto.conf, that will avoid any such
> observations for a well-behaved user.

Uh, what?  That sounds like you are proposing that postgresql.conf itself
is a dead letter.  Which is not going to fly.  We had that conversation
already.

The right way to fix this is just to avoid processing entries that get
overridden later in the configuration file scan.  That won't cause anyone
to get upset about how their old habits no longer work.
        regards, tom lane

Re: Tom Lane 2014-07-08 <28635.1404844154@sss.pgh.pa.us>
> > Sounds like a 9.5 feature, though.
> 
> No, ALTER SYSTEM is there now and it needs to work right in its first
> release.  I will go fix this if nobody else does.

I'd like to throw in again that imho this should include ALTER SYSTEM
RESET (ALL) in 9.4. It wouldn't make much sense to delay this to 9.5,
when it's simple functionality that can easily be tested and hence
shouldn't break anything in 9.4.

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

On 07/08/2014 06:10 PM, Mark Kirkwood wrote:
> On 09/07/14 05:13, Josh Berkus wrote:
>> On 07/06/2014 01:27 AM, Christoph Berg wrote:
>>> Another could be that during initdb all the uncommented settings be
>>>> written to postgresql.auto.conf file rather than to postgresql.conf.
>>>> I think we can do this by changing code in initdb.c->setup_config().
>>>> This will ensure that unless user is changing settings in a mixed way
>>>> (some by Alter System and some manually by editing postgresql.conf),
>>>> there will no such problem.
>>
>> There is no reasonable way for us to prevent issues for users who are
>> manually changing both pg.conf and pg.auto.conf.  Users should stick to
>> one or the other method of management (or, thirdly, using conf.d); if
>> they mix methods, we can't prevent confusion at restart time and we
>> shouldn't try.
>>
> 
> 
> Yes, but even well behaved users will see this type of error, because
> initdb uncomments certain values (ones that are dead certs for being
> changed via ALTER SYSTEM subsequently like shared_buffers), and then -
> bang! your next reload gets that "your postgresql.conf contains errors"
> message.

Actually, my response was based on misreading Berg's suggestion; I
thought he was suggesting that we would try to disentangle manual
changes to both, whereas he was suggesting the opposite.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 07/08/2014 08:18 PM, Amit Kapila wrote:
>> Yes, but even well behaved users will see this type of error, because
> initdb uncomments certain values (ones that are dead certs for being
> changed via ALTER SYSTEM subsequently like shared_buffers), and then -
> bang! your next reload gets that "your postgresql.conf contains errors"
> message.
> 
> That is the reason, why I have suggested up-thread that uncommented
> values should go to postgresql.auto.conf, that will avoid any such
> observations for a well-behaved user

Not an effective solution for three reasons:

1) Some users will copy over their older pg.conf's to 9.4, which will
already have shared_buffers uncommented;

2) Some distros ship their own pg.conf;

3) Doesn't solve the issue of overlapping files in conf.d, which is the
same problem.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh@agliodbs.com> writes:
> On 07/08/2014 10:07 AM, Robert Haas wrote:
>> I haven't looked at the code in this area too carefully, but it seems
>> to me like the flow ought to be:
>> 
>> 1. Read all of the config files and determine what the final value
>> present in each config file is.

AFAICS we only care about the final value, not the final-value-in-each-
config-file.

>> 2. *Then*, in a second pass, enforce requirements like "can't be
>> changed except at server start".

> +1

> This would also make conf.d much more useful; I wouldn't have to worry
> as much about overlapping config settings.

> Sounds like a 9.5 feature, though.

No, ALTER SYSTEM is there now and it needs to work right in its first
release.  I will go fix this if nobody else does.
        regards, tom lane

Tom Lane wrote:
> Josh Berkus <josh@agliodbs.com> writes:

> >> 2. *Then*, in a second pass, enforce requirements like "can't be
> >> changed except at server start".
> 
> > This would also make conf.d much more useful; I wouldn't have to worry
> > as much about overlapping config settings.
> 
> > Sounds like a 9.5 feature, though.
> 
> No, ALTER SYSTEM is there now and it needs to work right in its first
> release.  I will go fix this if nobody else does.

Just checking -- you didn't get around to dealing with this, right?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> No, ALTER SYSTEM is there now and it needs to work right in its first
>> release.  I will go fix this if nobody else does.

> Just checking -- you didn't get around to dealing with this, right?

Not yet... do you want it?
        regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> No, ALTER SYSTEM is there now and it needs to work right in its first
> >> release.  I will go fix this if nobody else does.
> 
> > Just checking -- you didn't get around to dealing with this, right?
> 
> Not yet... do you want it?

I might give it a try, but not just yet.  I'll let you know if I attempt
anything.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Jul 9, 2014 at 10:14 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Amit Kapila <amit.kapila16@gmail.com> writes:
>> > On Wed, Jul 9, 2014 at 6:40 AM, Mark Kirkwood
>> > <mark.kirkwood@catalyst.net.nz>
>> > wrote:
>> >> Yes, but even well behaved users will see this type of error, because
>> >> initdb uncomments certain values (ones that are dead certs for being
>> >> changed via ALTER SYSTEM subsequently like shared_buffers), and then -
>> >> bang! your next reload gets that "your postgresql.conf contains errors"
>> >> message.
>>
>> > That is the reason, why I have suggested up-thread that uncommented
>> > values should go to postgresql.auto.conf, that will avoid any such
>> > observations for a well-behaved user.
>>
>> Uh, what?  That sounds like you are proposing that postgresql.conf itself
>> is a dead letter.  Which is not going to fly.  We had that conversation
>> already.
>
> It might sound like that but honestly my intention was to just ease the use
> for users who just want to rely on Alter System.
>
>
>> The right way to fix this is just to avoid processing entries that get
>> overridden later in the configuration file scan.  That won't cause anyone
>> to get upset about how their old habits no longer work.
>
> 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...

Regards,

-- 
Fujii Masao

On Sat, Jul 26, 2014 at 1:07 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila <amit.kapila16@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,
andthe 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.confis 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?

Regards,

-- 
Fujii Masao

On 07/28/2014 11:03 AM, Fujii Masao wrote:
> On Sat, Jul 26, 2014 at 1:07 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila <amit.kapila16@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.

Also, it's my perspective that users who change conf by concurrently
editing pg.conf *and* doing ALTER SYSTEM SET are hopeless anyway.
There's simply no way we can protect them from themselves.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Tue, Jul 29, 2014 at 3:33 AM, Josh Berkus <josh@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.kapila16@gmail.com> wrote:
>>> On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila <amit.kapila16@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

Fujii Masao <masao.fujii@gmail.com> writes:
> 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?

I don't find that to be a particularly good idea.  In the first place,
it introduces extra complication and a surprising difference in the
behavior of different GUCs.  In the second place, I thought part of the
point of this patch was to suppress log messages complaining about
invalid values that then weren't actually used for anything.  That issue
exists just as much for non-POSTMASTER variables.  (IOW, "value cannot
be changed now" is not the only log message we're trying to suppress.)
        regards, tom lane

On Mon, Aug 4, 2014 at 11:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Fujii Masao <masao.fujii@gmail.com> writes:
>> 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?
>
> I don't find that to be a particularly good idea.  In the first place,
> it introduces extra complication and a surprising difference in the
> behavior of different GUCs.  In the second place, I thought part of the
> point of this patch was to suppress log messages complaining about
> invalid values that then weren't actually used for anything.  That issue
> exists just as much for non-POSTMASTER variables.  (IOW, "value cannot
> be changed now" is not the only log message we're trying to suppress.)

Yep, sounds reasonable. This makes me think that we can suppress
such invalid message of the parameters which are actually not used
at not only conf file reload but also *postmaster startup*. That's another
story, though. Anyway, barring any objection, I will commit Amit's patch.

Regards,

-- 
Fujii Masao

On Tue, Aug 5, 2014 at 12:49 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Mon, Aug 4, 2014 at 11:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Fujii Masao <masao.fujii@gmail.com> writes:
>>> 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?
>>
>> I don't find that to be a particularly good idea.  In the first place,
>> it introduces extra complication and a surprising difference in the
>> behavior of different GUCs.  In the second place, I thought part of the
>> point of this patch was to suppress log messages complaining about
>> invalid values that then weren't actually used for anything.  That issue
>> exists just as much for non-POSTMASTER variables.  (IOW, "value cannot
>> be changed now" is not the only log message we're trying to suppress.)
>
> Yep, sounds reasonable. This makes me think that we can suppress
> such invalid message of the parameters which are actually not used
> at not only conf file reload but also *postmaster startup*. That's another
> story, though. Anyway, barring any objection, I will commit Amit's patch.

Applied the slightly-modified version!

Regards,

-- 
Fujii Masao

On Tue, Jul 29, 2014 at 9:26 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Jul 28, 2014 at 11:33 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> 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.
>
> The reason is that during startup DataDir is not set by the time server
> processes config file due to which we process .auto.conf file in second
> pass.  I think ideally it should ignore the invalid setting in such a case
> as it does during reload, however currently there is no good way to
> process .auto.conf  incase DataDir is not set, so we handle it separately
> in second pass.

What about picking up only data_directory at the first pass?

Regards,

-- 
Fujii Masao

On Thu, Aug 7, 2014 at 12:36 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Aug 6, 2014 at 1:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Tue, Jul 29, 2014 at 9:26 PM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>> >
>> > The reason is that during startup DataDir is not set by the time server
>> > processes config file due to which we process .auto.conf file in second
>> > pass.  I think ideally it should ignore the invalid setting in such a
>> > case
>> > as it does during reload, however currently there is no good way to
>> > process .auto.conf  incase DataDir is not set, so we handle it
>> > separately
>> > in second pass.
>>
>> What about picking up only data_directory at the first pass?
>
> I think that will workout and for that I think we might need to add
> few checks during ProcessConfigFile.  Do you want to address it
> during parsing of config file or during setting of values?

I prefer "during paring". The attached patch does that. In this patch,
the first call of ProcessConfigFile() picks up only data_directory. One
concern of this patch is that the logic is a bit ugly. Do you have any
other better idea?

Regards,

--
Fujii Masao

On Thu, Aug 7, 2014 at 12:28 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Aug 6, 2014 at 11:39 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> On Tue, Aug 5, 2014 at 12:49 PM, Fujii Masao <masao.fujii@gmail.com>
>> wrote:
>> > On Mon, Aug 4, 2014 at 11:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >> Fujii Masao <masao.fujii@gmail.com> writes:
>> >>> 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?
>> >>
>> >> I don't find that to be a particularly good idea.  In the first place,
>> >> it introduces extra complication and a surprising difference in the
>> >> behavior of different GUCs.  In the second place, I thought part of the
>> >> point of this patch was to suppress log messages complaining about
>> >> invalid values that then weren't actually used for anything.  That
>> >> issue
>> >> exists just as much for non-POSTMASTER variables.  (IOW, "value cannot
>> >> be changed now" is not the only log message we're trying to suppress.)
>> >
>> > Yep, sounds reasonable. This makes me think that we can suppress
>> > such invalid message of the parameters which are actually not used
>> > at not only conf file reload but also *postmaster startup*. That's
>> > another
>> > story, though. Anyway, barring any objection, I will commit Amit's
>> > patch.
>>
>> Applied the slightly-modified version!
>
> Thanks.  There is a commitfest entry [1] for this patch, do you
> want some thing more to be addressed or shall we mark that as
> committed.
>
> [1]
> https://commitfest.postgresql.org/action/patch_view?id=1500

Yeah, let's mark this as committed because your patch has been committed and
the originally-reported problem has been fixed. We are now discussing another
patch, but I will add that as new CF entry.

Regards,

-- 
Fujii Masao

On Fri, Aug 8, 2014 at 1:19 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Aug 7, 2014 at 12:36 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Thu, Aug 7, 2014 at 12:36 PM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>> > On Wed, Aug 6, 2014 at 1:32 PM, Fujii Masao <masao.fujii@gmail.com>
>> > wrote:
>
>> >> What about picking up only data_directory at the first pass?
>> >
>> > I think that will workout and for that I think we might need to add
>> > few checks during ProcessConfigFile.  Do you want to address it
>> > during parsing of config file or during setting of values?
>>
>> I prefer "during paring". The attached patch does that. In this patch,
>> the first call of ProcessConfigFile() picks up only data_directory. One
>> concern of this patch is that the logic is a bit ugly.
>
> I think handling 'data_directory' in ParseConfigFp() looks bit out of
> place as this API is used to parse other config files as well like
> recovery.conf.  I agree that for all other paths DataDir might be
> already set due to which this new path will never be executed, still
> from code maintenance point of view it would have been better if we
> can find a way to handle it in a place where we are processing
> the server's main config file (postgresql.conf).

Yep, right. ParseConfigFp() is not good place to pick up data_directory.
What about the attached patch which makes ProcessConfigFile() instead of
ParseConfigFp() pick up data_directory just after the configuration file is
parsed?

Regards,

--
Fujii Masao

On Sun, Aug 10, 2014 at 3:54 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Aug 8, 2014 at 11:41 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> Yep, right. ParseConfigFp() is not good place to pick up data_directory.
>> What about the attached patch which makes ProcessConfigFile() instead of
>> ParseConfigFp() pick up data_directory just after the configuration file
>> is
>> parsed?
>
> I think this is better way to handle it. Few comments about patch:

Thanks for the review! Attached is the updated version of the patch.

> 1. can't we directly have the code by having else in below loop.
> if (DataDir &&
> !ParseConfigFile(PG_AUTOCONF_FILENAME, NULL, false, 0, elevel,
> &head, &tail))

Done.

> 2.
> + if (DataDir == NULL)
> + {
> + ConfigVariable *prev = NULL;
> + for (item = head; item;)
> + {
> ..
> ..
> + }
>
> If data_directory is not present in list, then can't we directly return
> and leave the other work in function ProcessConfigFile() for second
> pass.

Done.

> 3.
> I think comments can be improved:
> a.
> + In this case,
> + * we should not pick up all the settings except the data_directory
> + * from postgresql.conf yet because they might be overwritten
> + * with the settings in PG_AUTOCONF_FILENAME file which will be
> + * read later.
>
> It would be better if we change it as below:
>
> In this case, we shouldn't pick any settings except the data_directory
> from postgresql.conf because they might be overwritten
> with the settings in PG_AUTOCONF_FILENAME file which will be
> read later.

Done.

> b.
> +Now only the data_directory needs to be picked up to
> +  * read PG_AUTOCONF_FILENAME file later.
>
> This part of comment seems to be repetitive as you already mentioned
> some part of it in first line as well as at one other location:

Okay, I just remove that line.

While updating the patch, I found that the ConfigVariable which
is removed from list has the fields that the memory has been already
allocated but we forgot to free them. So I included the code to free
them in the patch.

Regards,

--
Fujii Masao

On Tue, Aug 12, 2014 at 1:23 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Aug 11, 2014 at 11:08 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>>
>> While updating the patch, I found that the ConfigVariable which
>> is removed from list has the fields that the memory has been already
>> allocated but we forgot to free them. So I included the code to free
>> them in the patch.
>
> Yes, that is right.
>
> ! /*
> ! * Pick up only the data_directory if DataDir is not set, which
> ! * means that the configuration file is read for the first time and
> ! * PG_AUTOCONF_FILENAME file cannot be read yet. In this case,
> ! * we shouldn't pick any settings except the data_directory
> ! * from postgresql.conf because they might be overwritten
> ! * with the settings in PG_AUTOCONF_FILENAME file which will be
> ! * read later. OTOH, since it's ensured that data_directory doesn't
> ! * exist in PG_AUTOCONF_FILENAME file, it will never be overwritten
> ! * later.
> ! */
> ! else
>
> It is bit incovinient to read this part of code, some other places in
> same file use comment inside else construct which seems to be
> better. one example is as below:

Yep, updated that way.

>
> else
> {
> /*
> * ordinary variable, append to list.  For multiple items of
> * same parameter, retain only which comes later.
> */
>
>
> I have marked this as Ready For Committer.

Thanks for the review! Committed.

Regards,

-- 
Fujii Masao