Zdenek,

On Thu, Jun 01, 2006 at 04:56:10PM +0200, Zdenek Kotala wrote:
> Thanks, You have right. It is problem with silent skip some option in
> set_config_option function when PGC_SIGHUP is running context.

> I fix it and I attach third version of patch.

Still it does not what I think it should do. I might have been unclear
before. If you put a comment in front of a PGC_POSTMASTER variable (and if
its value differs from the default) then this should be treated as if the
variable got changed and it should emmit a warning "<varname> can only be
changed on server start" or similar. This warning should be kept for every
other SIGHUP that gets sent just like it is done already when you change the
value (but do not comment the variable).

I still have the problem that the first signal I send does not trigger any
message (i get the "SIGHUP received", but nothing about the variable
changes) but I haven't looked into it yet.


Joachim

---------------------------(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

Joachim Wieland wrote:
>
> Still it does not what I think it should do. I might have been unclear
> before. If you put a comment in front of a PGC_POSTMASTER variable (and if
> its value differs from the default) then this should be treated as if the
> variable got changed and it should emmit a warning "<varname> can only be
> changed on server start" or similar. This warning should be kept for every
> other SIGHUP that gets sent just like it is done already when you change the
> value (but do not comment the variable).
>
Thanks for explanation. I overlooked this variant. When I analyzed
set_config_option I found some other bugs or strange things:

1) Try to change internal variable in the config file is silently
ignored during reconfiguration.
2) GUC_DISALLOW_IN_FILE flag is ignored during configuration file parsing.
3) If option is PGC_POSTMASTER type and value is not syntax valid, It
only generates warning message that value cannot be change and file
parsing continue.

Could some one validates my findings?

I think that set_config_options is too huge and very overloaded. By my
opinion divide to small functions is necessary to fix this behavior and
its increase maintainability.

Zdenek

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

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

Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> 2) GUC_DISALLOW_IN_FILE flag is ignored during configuration file parsing.

Yeah, that's not enforced at the moment, it's just documentation.
Most of the variables that are marked that way have other defenses
against being changed from the file, so I'm not sure that it's real
important to have a specific check for it.

> 1) Try to change internal variable in the config file is silently
> ignored during reconfiguration.

Note that there are two separate behaviors: during postmaster startup,
errors in the config file are cause for aborting. During SIGHUP reread,
errors in the config file may NOT cause an abort. This is intentional.
The postmaster (and only the postmaster, not the N backends that are
also rereading the file) is supposed to emit a log message about any
problems, but it mustn't error out.

> I think that set_config_options is too huge and very overloaded. By my
> opinion divide to small functions is necessary to fix this behavior and
> its increase maintainability.

Feel free to propose a suitable refactoring.

regards, tom lane

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

There is last version of patch with following changes/improvements:

1) I divide set_config_option to more smaller functions without backside
effect.

2) Behavior of reconfiguration is "same" in SIG_HUP context (exclude
elevel). All errors are reported and full validity of file is checked too.

Zdenek




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

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

Zdenek,

On Fri, Jul 14, 2006 at 12:17:55AM +0200, Zdenek Kotala wrote:
> There is last version of patch with following changes/improvements:

> 1) I divide set_config_option to more smaller functions without backside
> effect.

I did not check the changes you have done to set_config_option and the like
but tested the commenting / uncommenting / changing of guc variables and the
behavior and log output. 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.

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.

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


Joachim


Test cases for "guc falls back to default":

guc_context <= PGC_POSTMASTER (shared_buffers is an example, Default: 1000)

Commented value gets un-commented (value != default)
=> message every time a sighup is received

Example:
#shared_buffers = 3301
START
shared_buffers = 3301
HUP

Output:
LOG: parameter "shared_buffers" cannot be changed after server start;
configuration file change ignored



Value gets changed (to != initial).
=> message every time a sighup is received

Example:
shared_buffers = 3301
START
shared_buffers = 3302
HUP

Output:
LOG: parameter "max_prepared_transactions" cannot be changed after
server start; configuration file change ignored



Value gets commented (initial != default).
=> message every time a sighup is received

Example:
shared_buffers = 3301
START
#shared_buffers = 3301
HUP

Output:
LOG: parameter "max_prepared_transactions" cannot be changed
(commented) after server start; configuration file change ignored



Commented value (not applied) gets changed back to initial setting:
=> no more messages after SIGHUP

Example:
shared_buffers = 3301
START
#shared_buffers = 3301
HUP (value does not get applied)
shared_buffers = 3301
HUP

Output:
None



Commented value (not applied) gets changed to != initial:
=> message every time a SIGHUP is received

Example:
shared_buffers = 3301
START
#shared_buffers = 3301
HUP
shared_buffers = 3302
HUP

Output:
LOG: parameter "shared_buffers" cannot be changed after server start;
configuration file change ignored





guc_context <= PGC_SIGHUP set (fsync is an example, Default: true)

Value (== default) gets commented
=> nothing happens

Example:
fsync = true
START
#fsync = true
HUP

Output:
None



Value (!= default) gets commented
=> falls back to default on first HUP that is received)

Example:
fsync = false
START
fsync = true
HUP
(subsequent HUPs do not show output anymore)

Output:
LOG: configuration option fsync falls back to default value



Commented value gets un-commented (value != default)

Example:
#fsync = false
START
fsync = false
HUP

Output:
None



Commented value gets un-commented (value == default)

Example:
#fsync = true
START
fsync = true
HUP

Output:
None



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

Joachim Wieland <joe@mcknight.de> writes:
> I did not check the changes you have done to set_config_option and the like
> but tested the commenting / uncommenting / changing of guc variables and the
> behavior and log output. 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.

Notice message? Where did that come from? The behavior I thought
people were after was just that variables previously defined by the file
would revert to reset values if not any longer defined by the file.

From a reviewer's point of view, it'd be nice if the patch did not
contain so many useless changes of whitespace.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Joachim Wieland <joe@mcknight.de> writes:
> > I did not check the changes you have done to set_config_option and the like
> > but tested the commenting / uncommenting / changing of guc variables and the
> > behavior and log output. 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.
>
> Notice message? Where did that come from? The behavior I thought
> people were after was just that variables previously defined by the file
> would revert to reset values if not any longer defined by the file.

There's two issues here, I believe. There's the
'revert-to-reset-values' issue for things which can be changed with a
reload and then there's also the 'notice-message-if-unable-to-change'
a given variable without a reset.

On reload a variable is changed:

#1: That variable can be changed by a reload.
If the variable has been removed/commented-out then it is reverted
to the reset-value. Otherwise, the new value is used.

#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 change could be
either a revert to reset-value if it was removed/commented-out or an
explicit change request to a different value.

Personally, I'm very interested in having both. I'm about 90% sure both
were discussed previously on hackers and that the general consensus was
that both were good. It's possible the second point wasn't noticed by
everyone involved though. Of course, I might be misunderstanding what
Joachim was referring to also.

Thanks,

Stephen

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)

iD8DBQFExN9zrzgMPqB3kigRAqrTAJ9WyUkY6af0ZAdhf9OuAw pfZB/1JQCeNjI7
iHNhu3ua2z8DpXYtSxKF5Jg=
=AeT9
-----END PGP SIGNATURE-----

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.

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

---------------------------(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

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.


Joachim


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

On Mon, Jul 24, 2006 at 10:55:47AM -0400, Stephen Frost wrote:
> #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 change could be
> either a revert to reset-value if it was removed/commented-out or an
> explicit change request to a different value.

Right. And what I am voting for is to not only issue such a message once but
every time a SIGHUP is received as long as the actively-used value differs
from the value in the configuration file. One of the reasons for having this
fall-back-to-default-value stuff is to make sure that an admin can restart a
server and be sure that it will behave in the same way as when it was
shut down.

Moreover it's just clearer to send the notice message every time a SIGHUP is
received since every reload is the admin's request to apply all of the
values in the configuration file independently of what has happened in the
past.


Joachim


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

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