Re: Add pg_settings.pending_restart column

On Thu, Mar 5, 2015 at 12:04 PM, Peter Eisentraut <peter_e@gmx.net> 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