Re: Issue with PGC_BACKEND parameters

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

I will upload this patch to next CF, so that it can be tracked.
Kindly let me know your suggestions or if you have any objections?

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

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

Andrew Dunstan <andrew(at)dunslane(dot)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

On 01/30/2014 03:12 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)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

On Fri, Jan 31, 2014 at 2:01 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
> On 01/30/2014 03:12 PM, Tom Lane wrote:
>>
>> Andrew Dunstan <andrew(at)dunslane(dot)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(dot)kapila16(at)gmail(dot)com> writes:
> [ set_guc_backend_params_v2.patch ]

Committed with minor cosmetic adjustments.

regards, tom lane