Joachim Wieland wrote:
> On Mon, Jul 24, 2006 at 07:09:17PM +0200, Peter Eisentraut wrote:
> > Am Montag, 24. Juli 2006 16:55 schrieb Stephen Frost:
> > > #2: That variable can *not* be changed by a reload.
> > > Notice-level message is sent to the log notifying the admin
> > > that the change requested could not be performed.
> >
> > This already happens.
>
> Not if the option gets commented/deleted, i.e.:
>
> shared_buffers = 8000
> START
> #shared_buffers = 8000
> HUP
>
> This does not issue a message at the moment.

Because at the moment, the above does not change the value of
shared_buffers.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Joachim Wieland wrote:
> Zdenek,
>
> The general idea (at least my idea) is that
> whenever a SIGHUP is received and there is some difference between the
> config file and the active value that the server is using, a notice message
> is written to the log. That way, at every moment you can see if the active
> values coincide with the configuration file by sending a SIGHUP and if there
> are no such messages the admin can stop and restart the server and be sure
> that the settings will be the same after a restart.
>
> While testing, I specified a bunch of test cases that I attach below.

It would be nice to have this like regression test. I'm still thinking
how to realize it. There is problem how to change postgresql.conf
without backside effect on other test and for all platform.

> I also renamed the GUC_JUST_RELOAD to GUC_IN_CONFFILE because I did not
> really understand what GUC_JUST_RELOAD should mean. GUC_IN_CONFFILE means
> that the variable does show up in the configuration file and is active
> there, i.e. is not commented.

Yes, you have right, JUST_RELOAD is not much clear. I look that you
little bit changed function of this flag to. I think it is better and
more clear. However, I have some doubt that in the some special case -
SET/RESET usage in transaction - should generate some wrong output. I'm
working on some deep analyze and test case.

>
> Please check my changes, I'm pretty sure it can be cleaned up further.

Thanks for your improvement.

Zdenek

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

Joachim,

I checked your improvement and fix some problem with following scenario:

shared_buffers = 3301
START
share_buffers = 333.39
HUP
share_buffers requires integer value. Skip configuration file
#share_buffers = 3301
HUP
..... silent - no message

I performed some cleanup in my code as well. I reduced some conditions,
which cannot occur and fixed context validation in the
set_config_options function. I hope that It is final version of our patch.

Thanks for cooperation
Zdenek


---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Zdenek Kotala wrote:
> I performed some cleanup in my code as well. I reduced some
> conditions, which cannot occur and fixed context validation in the
> set_config_options function. I hope that It is final version of our
> patch.

The way I see it, combining a feature change with a code refactoring and
random white space changes is a pretty optimal way to get your patch
rejected. Please submit patches for these items separately.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Peter Eisentraut wrote:
>
> The way I see it, combining a feature change with a code refactoring and
> random white space changes is a pretty optimal way to get your patch
> rejected. Please submit patches for these items separately.
>

OK. I split patch to two parts. Part one is refactoring of
set_config_options function. Part two implements feature "Allow
commenting of variables in postgresql.conf to restore them to defaults".

Zdenek


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Zdenek Kotala wrote:
> OK. I split patch to two parts. Part one is refactoring of
> set_config_options function. Part two implements feature "Allow
> commenting of variables in postgresql.conf to restore them to
> defaults".

I'm having trouble wrapping my head around a code "refactoring" which
actually makes the code significantly *longer*. The only interface
change I could detect is the introduction of a function
verify_config_option(), which should just be a small variation on
set_config_option() as it currently exists.

I'm also about a relive a personal trauma if I see error messages like
this:

errmsg("configuration file is invalid")

I just had to deal with an unnamed product where this was all you got!

Please, explain again what this refactoring is supposed to achieve.

The second part of your patch actually looks pretty reasonable and does
not appear to require the refactoring.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Peter Eisentraut napsal(a):
> Zdenek Kotala wrote:
>> OK. I split patch to two parts. Part one is refactoring of
>> set_config_options function. Part two implements feature "Allow
>> commenting of variables in postgresql.conf to restore them to
>> defaults".
>
> I'm having trouble wrapping my head around a code "refactoring" which
> actually makes the code significantly *longer*. The only interface
> change I could detect is the introduction of a function
> verify_config_option(), which should just be a small variation on
> set_config_option() as it currently exists.

The main reason for refactoring was that set_config_option() was too
overloaded function and its behavior did not consistent. Old version of
set_config_function hides some messages. For example if you type:

tcp_port = 5432.1

then old implementation ignore this error without any message to log
file in the signal context (configuration reload). Main problem was that
semantic analysis of postgresql.conf is not perform in the
ProcessConfigFile function, but in the set_config_options *after*
context check. This skipped check for variables with PG_POSTMASTER
context. There was request from Joachim Wieland to add more messages
about ignored changes in the config file as well.


> I'm also about a relive a personal trauma if I see error messages like
> this:
>
> errmsg("configuration file is invalid")
>
> I just had to deal with an unnamed product where this was all you got!

Do you have any idea for better message? This message is last one during
parsing process. What is wrong is logged before this message.



Zdenek

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Patch applied. Thanks.

---------------------------------------------------------------------------


Zdenek Kotala wrote:
> Peter Eisentraut wrote:
> >
> > The way I see it, combining a feature change with a code refactoring and
> > random white space changes is a pretty optimal way to get your patch
> > rejected. Please submit patches for these items separately.
> >
>
> OK. I split patch to two parts. Part one is refactoring of
> set_config_options function. Part two implements feature "Allow
> commenting of variables in postgresql.conf to restore them to defaults".
>
> Zdenek



--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster