Re: Idea: closing the loop for "pg_ctl reload"

On 07/04/2015 04:24 AM, Jan de Visser wrote:
> On July 3, 2015 06:21:09 PM Tom Lane wrote:
>> Jan de Visser <jan(at)de-visser(dot)net> writes:
>>> Attached a new patch, rebased against the current head. Errors in
>>> pg_hba.conf and pg_ident.conf are now also noticed.
>>>
>>> I checked the documentation for pg_ctl reload, and the only place where
>>> it's explained seems to be runtime.sgml and that description is so
>>> high-level that adding this new bit of functionality wouldn't make much
>>> sense.
>>
>> BTW, it's probably worth pointing out that the recent work on the
>> pg_file_settings view has taken away a large part of the use-case for
>> this, in that you can find out more with less risk by inspecting
>> pg_file_settings before issuing SIGHUP, instead of SIGHUP'ing and
>> hoping you didn't break anything too nastily. Also, you can use
>> pg_file_settings remotely, unlike pg_ctl (though admittedly you
>> still need contrib/adminpack or something to allow uploading a
>> new config file if you're doing remote admin).
>>
>> I wonder whether we should consider inventing similar views for
>> pg_hba.conf and pg_ident.conf.
>>
>> While that's not necessarily a reason not to adopt this patch anyway,
>> it does mean that we should think twice about whether we need to add
>> a couple of hundred lines for a facility that's less useful than
>> what we already have.
>
> Since you were the one proposing the feature, I'm going to leave it to you
> whether or not I should continue with this.

It's handy that you can wait for the reload to complete, e.g. "pg_ctl
reload -w; psql ...", without having to put a "sleep 1" in there and
hope for the best. The view is useful too, but it's not the same thing.
This isn't the most exciting feature ever, but seems worthwhile to me.

>> BTW, this version of this patch neglects to update the comments in
>> miscadmin.h, and it makes the return convention for
>> ProcessConfigFileInternal completely unintelligible IMO; the inaccuracy
>> and inconsistency in the comments is a symptom of that. I didn't read it
>> in enough detail to say whether there are other problems.
>
> OK, miscadmin.h. I'll go and look what that's all about. And would it make
> sense to find a better solution for the ProcessConfigFileInternal return value
> (which is convoluted, I agree - I went for the solution with the least impact
> on existing code), or should I improve documentation?

Both ;-). I'd suggest adding a "bool *success" output parameter to that
function, and explaining it in the comments.

Other comments:

* A timestamp with one second granularity doesn't work if you reload the
config file twice within the same second. Or if the clock moves
backwards. Perhaps use a simple counter instead.

* Parsing the whole file into a struct in get_pidfile_contents() seems
like overkill, when you're only interested in the reload timestamp.
Granted, it might become useful in the future, but let's cross that
bridge when we get there, and keep this patch minimal.

* When "pg_ctl reload -w" returns, indicating that the configuration has
been reloaded, it only means that the postmaster has reloaded. Other
processes might not have. That's OK, but needs to be documented.

* There is no documentation.

- Heikki