Re: Issue with PGC_BACKEND parameters

On 12/22/2013 11:30 PM, Amit Kapila wrote:

I had observed one problem with PGC_BACKEND parameters while testing patch
for ALTER SYSTEM command.
Problem statement: If I change PGC_BACKEND parameters directly in
postgresql.conf and then do pg_reload_conf() and reconnect, it will
still show the old value.
Detailed steps
1. Start server with default settings
2. Connect Client
3. show log_connections; -- it will show as off, this is correct.
4. Change log_connections in postgresql.conf to on
5. issue command select pg_reload_conf() in client (which is started in step-2)
6. Connect a new client
7. show log_connections; -- it will show as off, this is "in-correct".
The problem is in step-7, it should show as on.
This problem occur only in Windows.
The reason for this problem is that in WINDOWS, when a new session is
started it will load the changed parameters in new backend by
global/config_exec_params file. The flow is in
SubPostmasterMain()->read_nondefault_variables()->set_config_option().
In below code in function set_config_option(), it will not allow to change
PGC_BACKEND variable and even in comments it has mentioned that only
postmaster will be allowed to change and the same will propagate to
subsequently started backends, but this is not TRUE for Windows.
switch (record->context)
{
..
..
case PGC_BACKEND:
if (context == PGC_SIGHUP)
{
/*
* If a PGC_BACKEND parameter is changed in
the config file,
* we want to accept the new value in the
postmaster (whence
* it will propagate to subsequently-started
backends), but
* ignore it in existing backends.
This is a tad klugy, but
* necessary because we don't re-read the
config file during
* backend start.
*/
if (IsUnderPostmaster)
return -1;
}
}

I think to fix the issue we need to pass the information whether PGC_BACKEND
parameter is allowed to change in set_config_option() function.
One way is to pass a new parameter.

Yesterday, I again thought about this issue and found that we can handle it by
checking IsInitProcessingMode() which will be True only during backend startup
which is what we need here.

Please find the attached patch to fix this issue.

I think that this issue should be fixed in PostgreSQL, because
currently PGC_BACKEND
parameters doesn't work on Windows.

--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5605,9 +5605,12 @@ set_config_option(const char *name, const char 
*value,
                   * it will propagate to subsequently-started 
backends), but
                   * ignore it in existing backends.    This is a tad 
klugy, but
                   * necessary because we don't re-read the config file 
during
-                 * backend start.
+                 * backend start. However for windows, we need to process
+                 * config file during backend start for non-default 
parameters,
+                 * so we need to allow change of PGC_BACKEND during backend
+                 * startup.
                   */
-                if (IsUnderPostmaster)
+                if (IsUnderPostmaster && !IsInitProcessingMode())
                      return -1;
              }
              else if (context != PGC_POSTMASTER && context != 
PGC_BACKEND &&

I think this change looks OK. Does anyone else have any comments before
I test and apply it? I presume it's a bugfix that should be backpatched.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andrew Dunstan <andrew@dunslane.net> writes:

On 12/22/2013 11:30 PM, Amit Kapila wrote:
-                 * backend start.
+                 * backend start. However for windows, we need to process
+                 * config file during backend start for non-default 
parameters,
+                 * so we need to allow change of PGC_BACKEND during backend
+                 * startup.
*/
-                if (IsUnderPostmaster)
+                if (IsUnderPostmaster && !IsInitProcessingMode())
return -1;
}

I think this change looks OK.

The comment is pretty awful, since this is neither Windows-specific nor
a read of the config file. Perhaps more like "However, in EXEC_BACKEND
builds we load nondefault settings from the CONFIG_EXEC_PARAMS file during
backend start. In that situation we should accept PGC_SIGHUP
settings, so as to have the same value as if we'd forked from the
postmaster."

Also, I think that the extra test should only be made #ifdef EXEC_BACKEND,
so as to minimize the risk of breaking things. Not that this isn't pretty
darn fragile anyway; I think testing IsInitProcessingMode here is a very
random way to detect this case. I wonder if it'd be better to pass down
an explicit flag indicating that we're doing read_nondefault_variables().
If we don't do that, maybe an Assert(IsInitProcessingMode()) in
read_nondefault_variables() would be a good thing.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/30/2014 03:12 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 12/22/2013 11:30 PM, Amit Kapila wrote:
-                 * backend start.
+                 * backend start. However for windows, we need to process
+                 * config file during backend start for non-default
parameters,
+                 * so we need to allow change of PGC_BACKEND during backend
+                 * startup.
*/
-                if (IsUnderPostmaster)
+                if (IsUnderPostmaster && !IsInitProcessingMode())
return -1;
}
I think this change looks OK.

The comment is pretty awful, since this is neither Windows-specific nor
a read of the config file. Perhaps more like "However, in EXEC_BACKEND
builds we load nondefault settings from the CONFIG_EXEC_PARAMS file during
backend start. In that situation we should accept PGC_SIGHUP
settings, so as to have the same value as if we'd forked from the
postmaster."

Also, I think that the extra test should only be made #ifdef EXEC_BACKEND,
so as to minimize the risk of breaking things. Not that this isn't pretty
darn fragile anyway; I think testing IsInitProcessingMode here is a very
random way to detect this case. I wonder if it'd be better to pass down
an explicit flag indicating that we're doing read_nondefault_variables().
If we don't do that, maybe an Assert(IsInitProcessingMode()) in
read_nondefault_variables() would be a good thing.

OK, I've added your comment to the commitfest item and marked it as
"Waiting on Author".

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 31, 2014 at 2:01 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

On 01/30/2014 03:12 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 12/22/2013 11:30 PM, Amit Kapila wrote:
-                 * backend start.
+                 * backend start. However for windows, we need to
process
+                 * config file during backend start for non-default
parameters,
+                 * so we need to allow change of PGC_BACKEND during
backend
+                 * startup.
*/
-                if (IsUnderPostmaster)
+                if (IsUnderPostmaster && !IsInitProcessingMode())
return -1;
}
I think this change looks OK.

The comment is pretty awful, since this is neither Windows-specific nor
a read of the config file. Perhaps more like "However, in EXEC_BACKEND
builds we load nondefault settings from the CONFIG_EXEC_PARAMS file during
backend start. In that situation we should accept PGC_SIGHUP
settings, so as to have the same value as if we'd forked from the
postmaster."

Changed as per suggestion.

Also, I think that the extra test should only be made #ifdef EXEC_BACKEND,
so as to minimize the risk of breaking things.

Agreed and changed the patch as per suggestion.

Not that this isn't pretty
darn fragile anyway; I think testing IsInitProcessingMode here is a very
random way to detect this case. I wonder if it'd be better to pass down
an explicit flag indicating that we're doing read_nondefault_variables().

My first idea was to add a parameter, but set_config_option is getting called
from multiple places and this case doesn't seem to be generic enough to
add a parameter to commonly used function, so I found another way of doing
it. I agree that adding a new parameter would be a better fix, but just seeing
the places from where it get called, I thought of doing it other way, however
if you feel strongly about it, I can change the patch to pass a new parameter
to set_config_option().

If we don't do that, maybe an Assert(IsInitProcessingMode()) in
read_nondefault_variables() would be a good thing.

Added Assert in read_nondefault_variables().

OK, I've added your comment to the commitfest item and marked it as "Waiting
on Author".

Thanks for Review.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:

[ set_guc_backend_params_v2.patch ]

Committed with minor cosmetic adjustments.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers