Re: [HACKERS] Add pg_settings.pending_restart column

2015-05-14 Thread Peter Eisentraut 
On 4/22/15 2:32 AM, Michael Paquier wrote:
> Attached is a rebased patch with previous comments addressed as I was
> looking at it.
> Switching this patch as "Ready for committer".

Committed, thanks.



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

Re: [HACKERS] Add pg_settings.pending_restart column

2015-04-21 Thread Michael Paquier 
On Thu, Mar 5, 2015 at 12:04 PM, Peter Eisentraut  wrote:
> On 2/17/15 10:45 AM, Robert Haas wrote:
>> You don't really need the "else" here, and in parallel cases:
>>
>>  if (*conf->variable != newval)
>>  {
>> +record->status |= GUC_PENDING_RESTART;
>>  ereport(elevel,
>>  (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
>>   errmsg("parameter \"%s\" cannot be
>> changed without restarting the server",
>>  name)));
>>  return 0;
>>  }
>> +else
>> +record->status &= ~GUC_PENDING_RESTART;
>>  return -1;
>>
>> The if-statement ends with "return 0" so there is no reason for the "else".
>
> I kind of liked the symmetry of if/else, but I can change it.

This feature looks useful to me. I had a quick look and it is working
as intended: issuing SIGHUP to reload parameters updates the
pending_restart status correctly.

One additional comment on top of what has already been mentioned is
that this lacks parenthesis IMO:
- values[16] = conf->status & GUC_PENDING_RESTART ? "t" : "f";
+ values[16] = (conf->status & GUC_PENDING_RESTART) ? "t" : "f";
Also, documentation was not correctly formatted.

Changes with ALTER SYSTEM (and include files) get recognized as well.
For example:
=# \! echo max_prepared_transactions = 100 >> $PGDATA/postgresql.conf
=# select pg_reload_conf();
 pg_reload_conf

 t
(1 row)
=# select name from pg_settings where pending_restart;
   name
---
 max_prepared_transactions
(1 row)
=# alter system set max_connections = 1000;
ALTER SYSTEM
=# select pg_reload_conf();
 pg_reload_conf

 t
(1 row)
=# select name from pg_settings where pending_restart;
   name
---
 max_connections
 max_prepared_transactions
(2 rows)

Attached is a rebased patch with previous comments addressed as I was
looking at it.
Switching this patch as "Ready for committer".
Regards,
-- 
Michael
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index d0b78f2..53d3f4f 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -8822,6 +8822,14 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
   or when examined by a non-superuser)
   
  
+ 
+  pending_restart
+  boolean
+  true if the value has been changed in the
+  configuration file but needs a restart; or false
+  otherwise.
+  
+ 
 

   
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f43aff2..e09b021 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5897,12 +5897,14 @@ set_config_option(const char *name, const char *value,
 {
 	if (*conf->variable != newval)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -5985,12 +5987,14 @@ set_config_option(const char *name, const char *value,
 {
 	if (*conf->variable != newval)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -6073,12 +6077,14 @@ set_config_option(const char *name, const char *value,
 {
 	if (*conf->variable != newval)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -6179,12 +6185,14 @@ set_config_option(const char *name, const char *value,
 	if (*conf->variable == NULL || newval == NULL ||
 		strcmp(*conf->variable, newval) != 0)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -6272,12 +6280,14 @@ set_config_option(const char *name, const char *value,
 {
 	if (*conf->variable != newval)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNT

Re: [HACKERS] Add pg_settings.pending_restart column

2015-03-04 Thread Peter Eisentraut 
On 2/15/15 3:41 AM, David G Johnston wrote:
> Otherwise it seems fine but I cannot help but feel that false positives are
> possible; though nothing that doesn't already exist.  Mainly, is the change
> going to end up only affect the reset or default value but not the currently
> active value?

I don't understand what you mean by this.  We already have the logic to
detect the situation concerned.  I'm only extending the reporting.  Of
course, if you can think of a case where it doesn't work correctly,
let's hear it.

> Instead of a boolean I'd rather have a string/enum that can capture the fact
> that a reboot is required and the expected outcome.  The same field can then
> communicate non-reboot stuff too - like "SIGHUP required" or "session
> scope".
> 
> A simple reboot required boolean api could just as easily be done via a
> function; and why limit to just reboots and not reconnection or SIGHUP
> required?
> 
> Scope creeping but the reboot case doesn't seem that special overall; other
> than the effort needed to realize the updated value.

It is special because we apply the context restrictions differently in
this case.  Normally, when you try to set a parameter that you can't
set, you get an error.  But in the case of restart-only, we just ignore
it (and log a message).  The point here is to make that more easily
detectable.

What you describe sounds more like that you want to see the complete
stack of overridden and possibly overriding settings.  That's a bit of a
different feature, I think.



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

Re: [HACKERS] Add pg_settings.pending_restart column

2015-03-04 Thread Peter Eisentraut 
On 2/17/15 10:45 AM, Robert Haas wrote:
> You don't really need the "else" here, and in parallel cases:
> 
>  if (*conf->variable != newval)
>  {
> +record->status |= GUC_PENDING_RESTART;
>  ereport(elevel,
>  (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
>   errmsg("parameter \"%s\" cannot be
> changed without restarting the server",
>  name)));
>  return 0;
>  }
> +else
> +record->status &= ~GUC_PENDING_RESTART;
>  return -1;
> 
> The if-statement ends with "return 0" so there is no reason for the "else".

I kind of liked the symmetry of if/else, but I can change it.




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

Re: [HACKERS] Add pg_settings.pending_restart column

2015-02-17 Thread Robert Haas 
On Sat, Feb 14, 2015 at 10:18 PM, Peter Eisentraut  wrote:
> When managing configuration changes through automatic systems like Chef
> or Puppet, there is a problem: How do you manage changes requiring a
> restart?
>
> Generally, you'd set it up so that when a configuration file is changed,
> the server is reloaded.  But for settings that require a restart, well,
> I don't know.  From discussions with others, it emerged that a way to
> ask the server whether a restart is necessary would be useful.  Then you
> can either automate the restart, or have a monitoring system warn you
> about it, and possibly schedule a restart separately or undo the
> configuration file change.
>
> So here is a patch for that.  It adds a column pending_restart to
> pg_settings that is true when the configuration file contains a changed
> setting that requires a restart.  We already had the logic to detect
> such changes, for producing the log entry.  I have also set it up so
> that if you change your mind and undo the setting and reload the server,
> the pending_restart flag is reset to false.

You don't really need the "else" here, and in parallel cases:

 if (*conf->variable != newval)
 {
+record->status |= GUC_PENDING_RESTART;
 ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be
changed without restarting the server",
 name)));
 return 0;
 }
+else
+record->status &= ~GUC_PENDING_RESTART;
 return -1;

The if-statement ends with "return 0" so there is no reason for the "else".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Add pg_settings.pending_restart column

2015-02-15 Thread David G Johnston 
Peter Eisentraut-2 wrote
> So here is a patch for that.  It adds a column pending_restart to
> pg_settings that is true when the configuration file contains a changed
> setting that requires a restart.  We already had the logic to detect
> such changes, for producing the log entry.  I have also set it up so
> that if you change your mind and undo the setting and reload the server,
> the pending_restart flag is reset to false.

Doc typo: s/of/if/

Otherwise it seems fine but I cannot help but feel that false positives are
possible; though nothing that doesn't already exist.  Mainly, is the change
going to end up only affect the reset or default value but not the currently
active value?

Instead of a boolean I'd rather have a string/enum that can capture the fact
that a reboot is required and the expected outcome.  The same field can then
communicate non-reboot stuff too - like "SIGHUP required" or "session
scope".

A simple reboot required boolean api could just as easily be done via a
function; and why limit to just reboots and not reconnection or SIGHUP
required?

Scope creeping but the reboot case doesn't seem that special overall; other
than the effort needed to realize the updated value.

David J.






--
View this message in context: 
http://postgresql.nabble.com/Add-pg-settings-pending-restart-column-tp5838009p5838020.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

[HACKERS] Add pg_settings.pending_restart column

2015-02-14 Thread Peter Eisentraut 
When managing configuration changes through automatic systems like Chef
or Puppet, there is a problem: How do you manage changes requiring a
restart?

Generally, you'd set it up so that when a configuration file is changed,
the server is reloaded.  But for settings that require a restart, well,
I don't know.  From discussions with others, it emerged that a way to
ask the server whether a restart is necessary would be useful.  Then you
can either automate the restart, or have a monitoring system warn you
about it, and possibly schedule a restart separately or undo the
configuration file change.

So here is a patch for that.  It adds a column pending_restart to
pg_settings that is true when the configuration file contains a changed
setting that requires a restart.  We already had the logic to detect
such changes, for producing the log entry.  I have also set it up so
that if you change your mind and undo the setting and reload the server,
the pending_restart flag is reset to false.
>From dd477dba05c5fc3a6e51535bb8280a67d22271f5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Sat, 14 Feb 2015 21:58:59 -0500
Subject: [PATCH] Add pg_settings.pending_restart column

---
 doc/src/sgml/catalogs.sgml  |  6 ++
 src/backend/utils/misc/guc.c| 21 -
 src/include/catalog/catversion.h|  2 +-
 src/include/catalog/pg_proc.h   |  2 +-
 src/include/utils/guc_tables.h  |  1 +
 src/test/regress/expected/rules.out |  5 +++--
 6 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 515a40e..4d1a401 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -8841,6 +8841,12 @@ pg_settings Columns
   or when examined by a non-superuser)
   
  
+ 
+  pending_restart
+  boolean
+  Set to true of the value has been changed in the configuration
+   file but needs a restart.
+ 
 

   
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9572777..44a2fc2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5834,12 +5834,15 @@ set_config_option(const char *name, const char *value,
 {
 	if (*conf->variable != newval)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	else
+		record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -5922,12 +5925,15 @@ set_config_option(const char *name, const char *value,
 {
 	if (*conf->variable != newval)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	else
+		record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -6010,12 +6016,15 @@ set_config_option(const char *name, const char *value,
 {
 	if (*conf->variable != newval)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	else
+		record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -6116,12 +6125,15 @@ set_config_option(const char *name, const char *value,
 	if (*conf->variable == NULL || newval == NULL ||
 		strcmp(*conf->variable, newval) != 0)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	else
+		record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -6209,12 +6221,15 @@ set_config_option(const char *name, const char *value,
 {
 	if (*conf->variable != newval)
 	{
+		record->status |= GUC_PENDING_RESTART;
 		ereport(elevel,
 (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  errmsg("parameter \"%s\" cannot be changed without restarting the server",
 		name)));
 		return 0;
 	}
+	else
+		record->status &= ~GUC_PENDING_RESTART;
 	return -1;
 }
 
@@ -7907,6 +7922,8 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
 		values[14] = NULL;
 		values[15] = NULL;
 	}
+
+	values[16] = conf->status & GUC_PENDING_RESTART ? "t" : "f";
 }
 
 /*
@@ -7942,7 +7959,7 @@ show_config_by_name(PG_FUNCTION_ARGS)
  * show_all_settings - equiv to SHOW ALL command but implemented as
  * a Table Function.
  */
-#define NUM_PG_SETTINGS_ATTS	16
+#define NUM_PG_SETTINGS_ATTS	17
 
 Datum
 show_all_se