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