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 <j...@de-visser.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
