Allow commenting of variables in postgresql.conf to restore them to defaults

Started by Zdenek Kotalaalmost 20 years ago32 messageshackers
Jump to latest
#1Zdenek Kotala
Zdenek.Kotala@Sun.COM

There is path implements following item from todo list: "Allow
commenting of variables in postgresql.conf to restore them to defaults".
Main idea is:

General config structure is extend with default_val attribute to keep
really default value. (There is small conflict - for string boot_val has same meaning).
During reconfiguration all values which has reset source equal with
PGC_S_FILE are revert back to really default values. New values from
configuration files are set after this step and commented variables stay
with default value.

Zdenek

Index: src/backend/utils/misc/guc-file.l

===================================================================

RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc-file.l,v

retrieving revision 1.37

diff -r1.37 guc-file.l

115c115

< int elevel;

---

int elevel, i;

116a117

char *env;

145a147,182

/* Revert all options with reset source PGC_S_FILE to default value.

* This implementation is easier then implementing some change flag and verify

* what really was commented out.

* XXX When log_line_prefix is set in configuration file then log output

* is not correct during this phase - prefix is revert to empty value.

*/

for (i = 0; i < num_guc_variables; i++)

{

struct config_generic *gconf = guc_variables[i];

if ( gconf->reset_source == PGC_S_FILE )

{

set_config_option(gconf->name, NULL, context,

PGC_S_FILE, false, true);

}

}

/* Revert to environment variable. PGPORT is ignored, because it cannot be

* set in running state. PGC_S_FILE is used instead PGC_S_ENV so as

* set_config_option can override previous defined option in config file.

* If these options are still in config file They will be overridden in

* the following step.

*/

env = getenv("PGDATESTYLE");

if (env != NULL)

{

set_config_option("datestyle", env, context,

PGC_S_FILE, false, true);

}

env = getenv("PGCLIENTENCODING");

if (env != NULL)

{

set_config_option("client_encoding", env, context,

PGC_S_FILE, false, true);

}

Index: src/backend/utils/misc/guc.c

===================================================================

RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v

retrieving revision 1.319

diff -r1.319 guc.c

2651c2651

< if (!(*conf->assign_hook) (conf->reset_val, true,

---

if (!(*conf->assign_hook) (conf->default_val, true,

2654,2655c2654,2655

< conf->gen.name, (int) conf->reset_val);

< *conf->variable = conf->reset_val;

---

conf->gen.name, (int) conf->default_val);

*conf->variable = conf->reset_val = conf->default_val;

2665c2665

< if (!(*conf->assign_hook) (conf->reset_val, true,

---

if (!(*conf->assign_hook) (conf->default_val, true,

2668,2669c2668,2669

< conf->gen.name, conf->reset_val);

< *conf->variable = conf->reset_val;

---

conf->gen.name, conf->default_val);

*conf->variable = conf->reset_val = conf->default_val;

2679c2679

< if (!(*conf->assign_hook) (conf->reset_val, true,

---

if (!(*conf->assign_hook) (conf->default_val, true,

2682,2683c2682,2683

< conf->gen.name, conf->reset_val);

< *conf->variable = conf->reset_val;

---

conf->gen.name, conf->default_val);

*conf->variable = conf->reset_val = conf->default_val;

3650c3650

< if (changeVal && !is_newvalue_equal(record, value))

---

if (changeVal && value != NULL && !is_newvalue_equal(record, value))

3726c3726

< makeDefault = changeVal && (source <= PGC_S_OVERRIDE) && (value != NULL);

---

makeDefault = changeVal && (source <= PGC_S_OVERRIDE) && (value != NULL || source == PGC_S_FILE);

3769,3770c3769,3781

< newval = conf->reset_val;

< source = conf->gen.reset_source;

---

/* Revert value to default if source is configuration file. It is used when

* configuration parameter is removed/commented out in the config file. Else

* RESET or SET TO DEFAULT command is called and reset_val is used.

*/

if( source == PGC_S_FILE )

{

newval = conf->default_val;

}

else

{

newval = conf->reset_val;

source = conf->gen.reset_source;

}

3853,3854c3864,3876

< newval = conf->reset_val;

< source = conf->gen.reset_source;

---

/* Revert value to default if source is configuration file. It is used when

* configuration parameter is removed/commented out in the config file. Else

* RESET or SET TO DEFAULT command is called and reset_val is used.

*/

if( source == PGC_S_FILE )

{

newval = conf->default_val;

}

else

{

newval = conf->reset_val;

source = conf->gen.reset_source;

}

3937,3938c3959,3971

< newval = conf->reset_val;

< source = conf->gen.reset_source;

---

/* Revert value to default if source is configuration file. It is used when

* configuration parameter is removed/commented out in the config file. Else

* RESET or SET TO DEFAULT command is called and reset_val is used.

*/

if( source == PGC_S_FILE )

{

newval = conf->default_val;

}

else

{

newval = conf->reset_val;

source = conf->gen.reset_source;

}

4011a4045,4058

else if (source == PGC_S_FILE)

{

/* Revert value to default when item is removed from config file. */

if ( conf->boot_val != NULL )

{

newval = guc_strdup(elevel, conf->boot_val);

if (newval == NULL)

return false;

}

else

{

return false;

}

}

5113a5161,5165

if( !newvalue )

{

return false;

}

Index: src/include/utils/guc_tables.h

===================================================================

RCS file: /projects/cvsroot/pgsql/src/include/utils/guc_tables.h,v

retrieving revision 1.22

diff -r1.22 guc_tables.h

145c145

< bool reset_val;

---

bool default_val;

149a150

bool reset_val;

158c159

< int reset_val;

---

int default_val;

164a166

int reset_val;

173c175

< double reset_val;

---

double default_val;

179a182,183

Show quoted text

double reset_val;

#2Andrew Dunstan
andrew@dunslane.net
In reply to: Zdenek Kotala (#1)
Re: Allow commenting of variables in postgresql.conf to

Zdenek Kotala wrote:

There is path implements following item from todo list: "Allow
commenting of variables in postgresql.conf to restore them to defaults".
Main idea is:

General config structure is extend with default_val attribute to keep
really default value. (There is small conflict - for string boot_val
has same meaning).
During reconfiguration all values which has reset source equal with
PGC_S_FILE are revert back to really default values. New values from
configuration files are set after this step and commented variables
stay with default value.

Please resubmit your patch as a context diff, as documented here:
http://www.postgresql.org/docs/faqs.FAQ_DEV.html#item1.5

cheers

andrew

#3Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Andrew Dunstan (#2)
Re: Allow commenting of variables in postgresql.conf to

Andrew Dunstan wrote:

Zdenek Kotala wrote:

There is path implements following item from todo list: "Allow
commenting of variables in postgresql.conf to restore them to defaults".
Main idea is:

General config structure is extend with default_val attribute to keep
really default value. (There is small conflict - for string boot_val
has same meaning).
During reconfiguration all values which has reset source equal with
PGC_S_FILE are revert back to really default values. New values from
configuration files are set after this step and commented variables
stay with default value.

Please resubmit your patch as a context diff, as documented here:
http://www.postgresql.org/docs/faqs.FAQ_DEV.html#item1.5

cheers

andrew

I am sorry. Here it is:

Index: src/backend/utils/misc/guc-file.l
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc-file.l,v
retrieving revision 1.37
diff -c -r1.37 guc-file.l
*** src/backend/utils/misc/guc-file.l    7 Mar 2006 01:03:12 -0000    1.37
--- src/backend/utils/misc/guc-file.l    24 May 2006 14:10:12 -0000
***************
*** 112,119 ****
  void
  ProcessConfigFile(GucContext context)
  {
!     int            elevel;
      struct name_value_pair *item, *head, *tail;

Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP);

--- 112,120 ----
  void
  ProcessConfigFile(GucContext context)
  {
!     int            elevel, i;
      struct name_value_pair *item, *head, *tail;
+     char       *env;

Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP);

***************
*** 143,148 ****
--- 144,185 ----
              goto cleanup_list;
      }
+     /* Revert all options with reset source PGC_S_FILE to default value.
+      * This implementation is easier then iplementing some change flag 
and verify
+      * what realy was commented out. 
+      * XXX When log_line_prefix is set in configuration file then log 
output
+      * is not correct during this phase - prefix is revert to empty value.
+      */
+     for (i = 0; i < num_guc_variables; i++)
+     {
+         struct config_generic *gconf = guc_variables[i];
+         if ( gconf->reset_source == PGC_S_FILE )
+         {
+             set_config_option(gconf->name, NULL, context,
+                   PGC_S_FILE, false, true);
+         }
+     }
+
+     /* Revert to environment variable. PGPORT is ignored, because it 
cannot be
+      * set in running state. PGC_S_FILE is used instead PGC_S_ENV so as
+      * set_config_option can override previous defined option in 
config file.
+      * If these options are still in config file They will be 
overriden in
+      * the following step.
+      */
+     env = getenv("PGDATESTYLE");
+     if (env != NULL)
+     {
+         set_config_option("datestyle", env, context,
+                             PGC_S_FILE, false, true);
+     }
+
+     env = getenv("PGCLIENTENCODING");
+     if (env != NULL)
+     {
+         set_config_option("client_encoding", env, context,
+                             PGC_S_FILE, false, true);
+     }
+
      /* If we got here all the options checked out okay, so apply them. */
      for (item = head; item; item = item->next)
      {
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.319
diff -c -r1.319 guc.c
*** src/backend/utils/misc/guc.c    11 May 2006 19:15:35 -0000    1.319
--- src/backend/utils/misc/guc.c    24 May 2006 14:10:12 -0000
***************
*** 2648,2658 ****
                      struct config_bool *conf = (struct config_bool *) 
gconf;
                      if (conf->assign_hook)
!                         if (!(*conf->assign_hook) (conf->reset_val, true,
                                                     PGC_S_DEFAULT))
                              elog(FATAL, "failed to initialize %s to %d",
!                                  conf->gen.name, (int) conf->reset_val);
!                     *conf->variable = conf->reset_val;
                      break;
                  }
              case PGC_INT:
--- 2648,2658 ----
                      struct config_bool *conf = (struct config_bool *) 
gconf;
                      if (conf->assign_hook)
!                         if (!(*conf->assign_hook) (conf->default_val, 
true,
                                                     PGC_S_DEFAULT))
                              elog(FATAL, "failed to initialize %s to %d",
!                                  conf->gen.name, (int) conf->default_val);
!                     *conf->variable = conf->reset_val = conf->default_val;
                      break;
                  }
              case PGC_INT:
***************
*** 2662,2672 ****
                      Assert(conf->reset_val >= conf->min);
                      Assert(conf->reset_val <= conf->max);
                      if (conf->assign_hook)
!                         if (!(*conf->assign_hook) (conf->reset_val, true,
                                                     PGC_S_DEFAULT))
                              elog(FATAL, "failed to initialize %s to %d",
!                                  conf->gen.name, conf->reset_val);
!                     *conf->variable = conf->reset_val;
                      break;
                  }
              case PGC_REAL:
--- 2662,2672 ----
                      Assert(conf->reset_val >= conf->min);
                      Assert(conf->reset_val <= conf->max);
                      if (conf->assign_hook)
!                         if (!(*conf->assign_hook) (conf->default_val, 
true,
                                                     PGC_S_DEFAULT))
                              elog(FATAL, "failed to initialize %s to %d",
!                                  conf->gen.name, conf->default_val);
!                     *conf->variable = conf->reset_val = 
conf->default_val;
                      break;
                  }
              case PGC_REAL:
***************
*** 2676,2686 ****
                      Assert(conf->reset_val >= conf->min);
                      Assert(conf->reset_val <= conf->max);
                      if (conf->assign_hook)
!                         if (!(*conf->assign_hook) (conf->reset_val, true,
                                                     PGC_S_DEFAULT))
                              elog(FATAL, "failed to initialize %s to %g",
!                                  conf->gen.name, conf->reset_val);
!                     *conf->variable = conf->reset_val;
                      break;
                  }
              case PGC_STRING:
--- 2676,2686 ----
                      Assert(conf->reset_val >= conf->min);
                      Assert(conf->reset_val <= conf->max);
                      if (conf->assign_hook)
!                         if (!(*conf->assign_hook) (conf->default_val, 
true,
                                                     PGC_S_DEFAULT))
                              elog(FATAL, "failed to initialize %s to %g",
!                                  conf->gen.name, conf->default_val);
!                     *conf->variable = conf->reset_val = 
conf->default_val;
                      break;
                  }
              case PGC_STRING:
***************
*** 3647,3653 ****
          case PGC_POSTMASTER:
              if (context == PGC_SIGHUP)
              {
!                 if (changeVal && !is_newvalue_equal(record, value))
                      ereport(elevel,
                              (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
                               errmsg("parameter \"%s\" cannot be 
changed after server start; configuration file change ignored",
--- 3647,3653 ----
          case PGC_POSTMASTER:
              if (context == PGC_SIGHUP)
              {
!                 if (changeVal && value != NULL && 
!is_newvalue_equal(record, value))
                      ereport(elevel,
                              (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
                               errmsg("parameter \"%s\" cannot be 
changed after server start; configuration file change ignored",
***************
*** 3723,3729 ****
       * Should we set reset/stacked values?    (If so, the behavior is not
       * transactional.)
       */
!     makeDefault = changeVal && (source <= PGC_S_OVERRIDE) && (value != 
NULL);
      /*
       * Ignore attempted set if overridden by previously processed setting.
--- 3723,3729 ----
       * Should we set reset/stacked values?    (If so, the behavior is not
       * transactional.)
       */
!     makeDefault = changeVal && (source <= PGC_S_OVERRIDE) && (value != 
NULL || source == PGC_S_FILE);

/*
* Ignore attempted set if overridden by previously processed setting.
***************
*** 3766,3773 ****
}
else
{
! newval = conf->reset_val;
! source = conf->gen.reset_source;
}

                  if (conf->assign_hook)
--- 3766,3784 ----
                  }
                  else
                  {
!                     /* Revert value to default if source is 
configuration file. It is used when
!                      * configuration parameter is removed/commented 
out in the config file. Else
!                      * RESET or SET TO DEFAULT command is called and 
reset_val is used.
!                      */
!                     if( source == PGC_S_FILE )
!                     {
!                         newval =  conf->default_val;
!                     }
!                     else
!                     {
!                         newval = conf->reset_val;
!                         source = conf->gen.reset_source;
!                     }
                  }

if (conf->assign_hook)
***************
*** 3850,3857 ****
}
else
{
! newval = conf->reset_val;
! source = conf->gen.reset_source;
}

                  if (conf->assign_hook)
--- 3861,3879 ----
                  }
                  else
                  {
!                     /* Revert value to default if source is 
configuration file. It is used when
!                      * configuration parameter is removed/commented 
out in the config file. Else
!                      * RESET or SET TO DEFAULT command is called and 
reset_val is used.
!                      */
!                     if( source == PGC_S_FILE )
!                     {
!                         newval =  conf->default_val;
!                     }
!                     else
!                     {
!                         newval = conf->reset_val;
!                         source = conf->gen.reset_source;
!                     }
                  }

if (conf->assign_hook)
***************
*** 3934,3941 ****
}
else
{
! newval = conf->reset_val;
! source = conf->gen.reset_source;
}

                  if (conf->assign_hook)
--- 3956,3974 ----
                  }
                  else
                  {
!                     /* Revert value to default if source is 
configuration file. It is used when
!                      * configuration parameter is removed/commented 
out in the config file. Else
!                      * RESET or SET TO DEFAULT command is called and 
reset_val is used.
!                      */
!                     if( source == PGC_S_FILE )
!                     {
!                         newval =  conf->default_val;
!                     }
!                     else
!                     {
!                         newval = conf->reset_val;
!                         source = conf->gen.reset_source;
!                     }
                  }
                  if (conf->assign_hook)
***************
*** 4009,4014 ****
--- 4042,4061 ----
                      if (conf->gen.flags & GUC_IS_NAME)
                          truncate_identifier(newval, strlen(newval), true);
                  }
+                 else if (source == PGC_S_FILE)
+                 {
+                     /* Revert value to default when item is removed 
from config file. */
+                     if ( conf->boot_val != NULL )
+                     {
+                         newval = guc_strdup(elevel, conf->boot_val);
+                         if (newval == NULL)
+                             return false;
+                     }
+                     else
+                     {
+                         return false;
+                     }
+                 }
                  else if (conf->reset_val)
                  {
                      /*
***************
*** 5111,5116 ****
--- 5158,5168 ----
  static bool
  is_newvalue_equal(struct config_generic *record, const char *newvalue)
  {
+     if( !newvalue )
+     {
+         return false;
+     }
+  
      switch (record->vartype)
      {
          case PGC_BOOL:
Index: src/include/utils/guc_tables.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/utils/guc_tables.h,v
retrieving revision 1.22
diff -c -r1.22 guc_tables.h
*** src/include/utils/guc_tables.h    5 Mar 2006 15:59:07 -0000    1.22
--- src/include/utils/guc_tables.h    24 May 2006 14:10:13 -0000
***************
*** 142,152 ****
      /* these fields must be set correctly in initial value: */
      /* (all but reset_val are constants) */
      bool       *variable;
!     bool        reset_val;
      GucBoolAssignHook assign_hook;
      GucShowHook show_hook;
      /* variable fields, initialized at runtime: */
      bool        tentative_val;
  };
  struct config_int
--- 142,153 ----
      /* these fields must be set correctly in initial value: */
      /* (all but reset_val are constants) */
      bool       *variable;
!     bool        default_val;
      GucBoolAssignHook assign_hook;
      GucShowHook show_hook;
      /* variable fields, initialized at runtime: */
      bool        tentative_val;
+     bool        reset_val;
  };

struct config_int
***************
*** 155,167 ****
/* these fields must be set correctly in initial value: */
/* (all but reset_val are constants) */
int *variable;
! int reset_val;
int min;
int max;
GucIntAssignHook assign_hook;
GucShowHook show_hook;
/* variable fields, initialized at runtime: */
int tentative_val;
};

  struct config_real
--- 156,169 ----
      /* these fields must be set correctly in initial value: */
      /* (all but reset_val are constants) */
      int           *variable;
!     int            default_val;
      int            min;
      int            max;
      GucIntAssignHook assign_hook;
      GucShowHook show_hook;
      /* variable fields, initialized at runtime: */
      int            tentative_val;
+     int            reset_val;
  };

struct config_real
***************
*** 170,182 ****
/* these fields must be set correctly in initial value: */
/* (all but reset_val are constants) */
double *variable;
! double reset_val;
double min;
double max;
GucRealAssignHook assign_hook;
GucShowHook show_hook;
/* variable fields, initialized at runtime: */
double tentative_val;
};

  struct config_string
--- 172,186 ----
      /* these fields must be set correctly in initial value: */
      /* (all but reset_val are constants) */
      double       *variable;
!     double        default_val;
      double        min;
      double        max;
      GucRealAssignHook assign_hook;
      GucShowHook show_hook;
      /* variable fields, initialized at runtime: */
      double        tentative_val;
+       double        reset_val;
+ 
  };

struct config_string

#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Zdenek Kotala (#3)
Re: Allow commenting of variables in postgresql.conf to

Zdenek Kotala wrote:

Andrew Dunstan wrote:

Zdenek Kotala wrote:

There is path implements following item from todo list: "Allow
commenting of variables in postgresql.conf to restore them to defaults".
Main idea is:

General config structure is extend with default_val attribute to keep
really default value. (There is small conflict - for string boot_val
has same meaning).
During reconfiguration all values which has reset source equal with
PGC_S_FILE are revert back to really default values. New values from
configuration files are set after this step and commented variables
stay with default value.

Please resubmit your patch as a context diff, as documented here:
http://www.postgresql.org/docs/faqs.FAQ_DEV.html#item1.5

I am sorry. Here it is:

Please resubmit as an attachment, or disallow your mail client from
munging whitespace ... I see wrapped words here, and apparently tab
expansion as well.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

#5Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Alvaro Herrera (#4)
Re: Allow commenting of variables in postgresql.conf to

Alvaro Herrera wrote:

Zdenek Kotala wrote:

Zdenek Kotala wrote:

There is path implements following item from todo list: "Allow
commenting of variables in postgresql.conf to restore them to defaults".
Main idea is:

General config structure is extend with default_val attribute to keep
really default value. (There is small conflict - for string boot_val
has same meaning).
During reconfiguration all values which has reset source equal with
PGC_S_FILE are revert back to really default values. New values from
configuration files are set after this step and commented variables
stay with default value.

Please resubmit as an attachment, or disallow your mail client from
munging whitespace ... I see wrapped words here, and apparently tab
expansion as well.

OK. Here is patch like attachment.

Zdenek

Attachments:

pg_conf.patchtext/x-patch; name=pg_conf.patchDownload+135-75
#6Joachim Wieland
joe@mcknight.de
In reply to: Zdenek Kotala (#5)
Re: Allow commenting of variables in postgresql.conf to

Zdenek,

On Wed, May 24, 2006 at 04:27:01PM +0200, Zdenek Kotala wrote:

General config structure is extend with default_val attribute to keep
really default value. (There is small conflict - for string boot_val
has same meaning).
During reconfiguration all values which has reset source equal with
PGC_S_FILE are revert back to really default values. New values from
configuration files are set after this step and commented variables
stay with default value.

Three points after a quick test:

- please compile with --enable-cassert, there are wrong assertions in your
code (you might just have to move some lines, there is an Assert() and an
assignment thereafter)

- changing a PGC_POSTMASTER should show a message:
=> parameter \"%s\" cannot be changed after server start;
configuration file change ignored
This seems to not show up anymore with your patch.

- with the same reasoning, I think it's a good idea to display a message
about which option falls back to its default value.

Joachim

#7Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Joachim Wieland (#6)
Re: Allow commenting of variables in postgresql.conf to

Joachim,

thanks for your comments. I am working on them.

Zdenek

Joachim Wieland wrote:

Show quoted text

Zdenek,

Three points after a quick test:

- please compile with --enable-cassert, there are wrong assertions in your
code (you might just have to move some lines, there is an Assert() and an
assignment thereafter)

- changing a PGC_POSTMASTER should show a message:
=> parameter \"%s\" cannot be changed after server start;
configuration file change ignored
This seems to not show up anymore with your patch.

- with the same reasoning, I think it's a good idea to display a message
about which option falls back to its default value.

Joachim

#8Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Joachim Wieland (#6)
Re: Allow commenting of variables in postgresql.conf to -

There is second version of patch. I made following changes:

- fix problem with assertion
- use boot_val instead default_val for all configuration data types
- add GUC_JUST_RELOAD status to track what is in configuration file or not
- revert only commented out variables
- add message what is revert to boot value

Joachim, could you explain me second point? I cannot determine
described problem. By my opinion my patch does not change this behavior.

Zdenek

Joachim Wieland wrote:

Show quoted text

Zdenek,

Three points after a quick test:

- please compile with --enable-cassert, there are wrong assertions in your
code (you might just have to move some lines, there is an Assert() and an
assignment thereafter)

- changing a PGC_POSTMASTER should show a message:
=> parameter \"%s\" cannot be changed after server start;
configuration file change ignored
This seems to not show up anymore with your patch.

- with the same reasoning, I think it's a good idea to display a message
about which option falls back to its default value.

Joachim

Attachments:

pg_conf.patchtext/x-patch; name=pg_conf.patchDownload+168-89
#9Joachim Wieland
joe@mcknight.de
In reply to: Zdenek Kotala (#8)
Re: Allow commenting of variables in postgresql.conf to - try2

Zdenek,

On Wed, May 31, 2006 at 06:13:04PM +0200, Zdenek Kotala wrote:

Joachim, could you explain me second point? I cannot determine
described problem. By my opinion my patch does not change this behavior.

I guess what I saw was another phenomenon:

I do the following:

- vi postgresql.conf => "allow_system_table_mods = true"
- start postmaster
- vi postgresql.conf => "# allow_system_table_mods = true" (commented)
- killall -HUP postmaster

Then I get _no_ message. After another killall -HUP I do indeed get a
message. So I don't get it just for the first time which is strange, do you
see that as well?

However the message I get is that it got reset to its default value which is
wrong because its a PGC_POSTMASTER variable that can only be set at server
start (set_config_option() returns true in this case as well).

Consequently I expect to get it for every other signal I send (because the
old value is still active and differs from what is in the configuration
file).

Joachim

#10Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Joachim Wieland (#9)
Re: Allow commenting of variables in postgresql.conf to -

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.

Zdenek

Joachim Wieland wrote:

Show quoted text

Zdenek,

On Wed, May 31, 2006 at 06:13:04PM +0200, Zdenek Kotala wrote:

Joachim, could you explain me second point? I cannot determine
described problem. By my opinion my patch does not change this behavior.

I guess what I saw was another phenomenon:

I do the following:

- vi postgresql.conf => "allow_system_table_mods = true"
- start postmaster
- vi postgresql.conf => "# allow_system_table_mods = true" (commented)
- killall -HUP postmaster

Then I get _no_ message. After another killall -HUP I do indeed get a
message. So I don't get it just for the first time which is strange, do you
see that as well?

However the message I get is that it got reset to its default value which is
wrong because its a PGC_POSTMASTER variable that can only be set at server
start (set_config_option() returns true in this case as well).

Consequently I expect to get it for every other signal I send (because the
old value is still active and differs from what is in the configuration
file).

Joachim

Attachments:

pg_conf_3.patchtext/x-patch; name=pg_conf_3.patchDownload+170-89
#11Joachim Wieland
joe@mcknight.de
In reply to: Zdenek Kotala (#10)
Re: Allow commenting of variables in postgresql.conf to - try3

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

#12Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Joachim Wieland (#11)
Re: Allow commenting of variables in postgresql.conf to -

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

#13Tom Lane
tgl@sss.pgh.pa.us
In reply to: Zdenek Kotala (#12)
Re: Allow commenting of variables in postgresql.conf to -

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

#14Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Zdenek Kotala (#12)
Re: Allow commenting of variables in postgresql.conf to -

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

Attachments:

pg_conf_04.patchtext/x-patch; name=pg_conf_04.patchDownload+750-556
#15Joachim Wieland
joe@mcknight.de
In reply to: Zdenek Kotala (#14)
Re: Allow commenting of variables in postgresql.conf to - try 4

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

Attachments:

pg_guc_default.rev.1.difftext/plain; charset=us-asciiDownload+796-645
#16Tom Lane
tgl@sss.pgh.pa.us
In reply to: Joachim Wieland (#15)
Re: Allow commenting of variables in postgresql.conf to - try 4

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

#17Stephen Frost
sfrost@snowman.net
In reply to: Tom Lane (#16)
Re: Allow commenting of variables in postgresql.conf to - try 4

* 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

#18Peter Eisentraut
peter_e@gmx.net
In reply to: Stephen Frost (#17)
Re: Allow commenting of variables in postgresql.conf to - try 4

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/

#19Joachim Wieland
joe@mcknight.de
In reply to: Peter Eisentraut (#18)
Re: Allow commenting of variables in postgresql.conf to - try 4

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

#20Joachim Wieland
joe@mcknight.de
In reply to: Stephen Frost (#17)
Re: Allow commenting of variables in postgresql.conf to - try 4

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

#21Peter Eisentraut
peter_e@gmx.net
In reply to: Joachim Wieland (#19)
#22Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Joachim Wieland (#15)
#23Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Joachim Wieland (#15)
#24Peter Eisentraut
peter_e@gmx.net
In reply to: Zdenek Kotala (#23)
#25Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Peter Eisentraut (#24)
#26Peter Eisentraut
peter_e@gmx.net
In reply to: Zdenek Kotala (#25)
#27Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Peter Eisentraut (#26)
#28Bruce Momjian
bruce@momjian.us
In reply to: Zdenek Kotala (#25)
#29Zdenek Kotala
Zdenek.Kotala@Sun.COM
In reply to: Peter Eisentraut (#26)
#30Bruce Momjian
bruce@momjian.us
In reply to: Zdenek Kotala (#29)
#31Peter Eisentraut
peter_e@gmx.net
In reply to: Zdenek Kotala (#29)
#32Bruce Momjian
bruce@momjian.us
In reply to: Peter Eisentraut (#31)