Обсуждение: Idea: closing the loop for "pg_ctl reload"

Bug #12788 reminded me of a problem I think we've discussed before:
if you use "pg_ctl reload" to trigger reload of the postmaster's
config files, and there's something wrong with those files, there's
no warning to you of that.  The postmaster just bleats to its log and
keeps running.  If you don't think to look in the log to confirm
successful reload, you're left with a time bomb: the next attempt
to restart the postmaster will fail altogether because of the bad
config file.

I commented in the bug thread that there wasn't much that pg_ctl
could do about this, but on reflection it seems like something we
could fix, because pg_ctl must be able to read the postmaster.pid
file in order to issue a reload signal.  Consider a design like this:

1. Extend the definition of the postmaster.pid file to add another
line, which will contain the time of the last postmaster configuration
load attempt (might as well be a numeric Unix-style timestamp) and
a boolean indication of whether that attempt succeeded or not.

2. Change pg_ctl so that after sending a reload signal, it sleeps
for a second and checks for a change in the config load timestamp
(repeat as necessary till timeout).  Once it sees the timestamp
change, it's in a position to report success or failure using the
boolean.  I think this should become the default behavior, though
you could define -W to mean that there should be no wait or feedback.

It's tempting to think of storing a whole error message rather than
just a boolean, but I think that would complicate the pidfile definition
undesirably.  A warning to look in the postmaster log ought to be
sufficient here.

For extra credit, the pg_reload_conf() function could be made to behave
similarly.

I don't have the time to pursue this idea myself, but perhaps someone
looking for a not-too-complicated project could take it on.
        regards, tom lane

On February 19, 2015 08:26:45 PM Tom Lane wrote:
> I don't have the time to pursue this idea myself, but perhaps someone
> looking for a not-too-complicated project could take it on.

I can have a crack at this. What's the process? Do I add it to a CF once I 
have a patch, or do I do that beforehand?

jan

Jan de Visser <jan@de-visser.net> writes:
> I can have a crack at this. What's the process? Do I add it to a CF once I 
> have a patch, or do I do that beforehand?

The CF process is for reviewing things, so until you have either a patch
or a design sketch you want feedback on, there's no need for a CF entry.
        regards, tom lane

On February 19, 2015 08:26:45 PM Tom Lane wrote:
> Bug #12788 reminded me of a problem I think we've discussed before:
> if you use "pg_ctl reload" to trigger reload of the postmaster's
> config files, and there's something wrong with those files, there's
> no warning to you of that.  The postmaster just bleats to its log and
> keeps running.  If you don't think to look in the log to confirm
> successful reload, you're left with a time bomb: the next attempt
> to restart the postmaster will fail altogether because of the bad
> config file.
>
> I commented in the bug thread that there wasn't much that pg_ctl
> could do about this, but on reflection it seems like something we
> could fix, because pg_ctl must be able to read the postmaster.pid
> file in order to issue a reload signal.  Consider a design like this:
>
> 1. Extend the definition of the postmaster.pid file to add another
> line, which will contain the time of the last postmaster configuration
> load attempt (might as well be a numeric Unix-style timestamp) and
> a boolean indication of whether that attempt succeeded or not.
>
> 2. Change pg_ctl so that after sending a reload signal, it sleeps
> for a second and checks for a change in the config load timestamp
> (repeat as necessary till timeout).  Once it sees the timestamp
> change, it's in a position to report success or failure using the
> boolean.  I think this should become the default behavior, though
> you could define -W to mean that there should be no wait or feedback.
>
> It's tempting to think of storing a whole error message rather than
> just a boolean, but I think that would complicate the pidfile definition
> undesirably.  A warning to look in the postmaster log ought to be
> sufficient here.
>
> For extra credit, the pg_reload_conf() function could be made to behave
> similarly.
>
> I don't have the time to pursue this idea myself, but perhaps someone
> looking for a not-too-complicated project could take it on.
>
>             regards, tom lane

Here's my first crack at this. Notes:
1/ I haven't done the '-W' flag Tom mentions yet.
2/ Likewise haven't touched pg_reload_conf()
3/ Design details: I introduced a new struct in pg_ctl containing the parsed-
out data from postmaster.pid, with functions to retrieve the data and to
dispose memory allocated for it. This made the change in do_reload fairly
straightforward. I think this struct can be used in other places in pg_ctl as
well, and potentially in other files as well.
4/ Question: Can I assume pg_malloc allocated memory is set to zero? If not,
is it OK to do a memset(..., 0, ...)? I don't have much experience on any of
the esoteric platforms pgsql supports...

jan

On March 2, 2015 12:56:23 AM Jan de Visser wrote:
... stuff ...

I seem to have mis-clicked something in the CF app - I created two entries 
somehow. I think one got created when I entered the msgid of Tom's original 
message with the enclosing '<...>'. If that's the case, then that may be a 
bug.

jan

On Mon, Mar 2, 2015 at 3:01 PM, Jan de Visser <jan@de-visser.net> wrote:
> On March 2, 2015 12:56:23 AM Jan de Visser wrote:
> ... stuff ...
>
> I seem to have mis-clicked something in the CF app - I created two entries
> somehow. I think one got created when I entered the msgid of Tom's original
> message with the enclosing '<...>'. If that's the case, then that may be a
> bug.

Don't worry for that. I just removed the duplicated entry.
-- 
Michael

Jan de Visser <jan@de-visser.net> writes:
> 4/ Question: Can I assume pg_malloc allocated memory is set to zero? If not, 
> is it OK to do a memset(..., 0, ...)? I don't have much experience on any of 
> the esoteric platforms pgsql supports...

No, you need the memset.  You might accidentally get already-zeroed memory
from malloc, but it's not something you can rely on.

However, you could and should use pg_malloc0, which takes care of that
for you...
        regards, tom lane

On March 2, 2015 09:50:49 AM Tom Lane wrote:
> However, you could and should use pg_malloc0, which takes care of that
> for you...

I am (using pg_malloc, that is). So, just to be sure: pg_malloc memsets the 
block to 0, right? 

My question was more along the lines if memsetting to 0 to ensure that pointer 
fields are NULL and int/long fields are 0. I know they are on Linux, but don't 
know if that applies to other platforms as well, or if I need to set fields 
explicitly to those 'zero'/'uninitialized' values.

jan

Jan de Visser <jan@de-visser.net> writes:
> On March 2, 2015 09:50:49 AM Tom Lane wrote:
>> However, you could and should use pg_malloc0, which takes care of that
>> for you...

> I am (using pg_malloc, that is). So, just to be sure: pg_malloc memsets the 
> block to 0, right? 

No, it doesn't, but pg_malloc0 does.  Consult the code if you're confused:
src/common/fe_memutils.c

> My question was more along the lines if memsetting to 0 to ensure that pointer 
> fields are NULL and int/long fields are 0.

Yes, we do assume that widely, and so does a heck of a lot of other code.
In principle the C standard doesn't require that a NULL pointer be
all-zero-bits, only that casting "0" to a pointer yield a NULL pointer.
But certainly there are no modern implementations that don't represent
NULL as 0.  Anybody who tried to do it differently would soon find that
hardly any real-world C code would run on their platform.
        regards, tom lane

On March 2, 2015 12:44:40 PM Tom Lane wrote:
> No, it doesn't, but pg_malloc0 does.  Consult the code if you're confused:
> src/common/fe_memutils.c

Doh! I  read pg_malloc( ), not pg_malloc0. 

Jan de Visser <jan@de-visser.net> wrote:
> On March 2, 2015 09:50:49 AM Tom Lane wrote:
>> However, you could and should use pg_malloc0, which takes care
>> of that for you...
>
> I am (using pg_malloc, that is). So, just to be sure: pg_malloc
> memsets the block to 0, right?

I think you may have misread a zero character as an empty pair of
parentheses.  Tom pointed out that the pg_malloc() function gives
you uninitialized memory -- you cannot count on the contents.  He
further pointed out that if you need it to be initialized to '0'
bytes you should call the pg_malloc0() function rather than calling
the pg_malloc() function and running memset separately.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On March 2, 2015 12:56:23 AM Jan de Visser wrote:
>
> Here's my first crack at this. Notes:
> 1/ I haven't done the '-W' flag Tom mentions yet.
> 2/ Likewise haven't touched pg_reload_conf()
> 3/ Design details: I introduced a new struct in pg_ctl containing the
> parsed- out data from postmaster.pid, with functions to retrieve the data
> and to dispose memory allocated for it. This made the change in do_reload
> fairly straightforward. I think this struct can be used in other places in
> pg_ctl as well, and potentially in other files as well.
> 4/ Question: Can I assume pg_malloc allocated memory is set to zero? If not,
> is it OK to do a memset(..., 0, ...)? I don't have much experience on any
> of the esoteric platforms pgsql supports...

Slight tweaks to better deal with pre-9.5 (6?) servers that don't write the
reload timestamp. I tested with a 9.4 server and it seemed to work...

Also implemented -W/-w. Haven't done pg_reload_conf() yet.

On Fri, Feb 20, 2015 at 1:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 1. Extend the definition of the postmaster.pid file to add another
> line, which will contain the time of the last postmaster configuration
> load attempt (might as well be a numeric Unix-style timestamp) and
> a boolean indication of whether that attempt succeeded or not.


Fwiw this concerns me slightly. I'm sure a lot of people are doing
things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.

We do have the external_pid_file GUC so perhaps this just means we
should warn people in the release notes or somewhere and point them at
that GUC.

-- 
greg

On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
> Fwiw this concerns me slightly. I'm sure a lot of people are doing
> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.

postmaster.pid already contains considerably more than just the pid. e.g.
4071
/srv/dev/pgdev-master
1425396089
5440
/tmp
localhost 5440001  82345992

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
>> Fwiw this concerns me slightly. I'm sure a lot of people are doing
>> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.

> postmaster.pid already contains considerably more than just the pid. e.g.

Yeah, that ship sailed long ago.  I'm sure anyone who's doing this is
using "head -1" not just "cat", and what I suggest wouldn't break that.
        regards, tom lane

On 3/3/15 9:26 AM, Andres Freund wrote:
> On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
>> Fwiw this concerns me slightly. I'm sure a lot of people are doing
>> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
>
> postmaster.pid already contains considerably more than just the pid. e.g.
> 4071
> /srv/dev/pgdev-master
> 1425396089
> 5440
> /tmp
> localhost
>    5440001  82345992

If we already have all this extra stuff, why not include an actual error 
message then, or at least the first line of an error (or maybe just swap 
any newlines with spaces)?
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On March 3, 2015 10:29:43 AM Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
> >> Fwiw this concerns me slightly. I'm sure a lot of people are doing
> >> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
> > 
> > postmaster.pid already contains considerably more than just the pid. e.g.
> 
> Yeah, that ship sailed long ago.  I'm sure anyone who's doing this is
> using "head -1" not just "cat", and what I suggest wouldn't break that.
> 
>             regards, tom lane

And what I've implemented doesn't either. The extra line is added to the back 
of the file.

On March 3, 2015 11:09:29 AM Jim Nasby wrote:
> On 3/3/15 9:26 AM, Andres Freund wrote:
> > On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
> >> Fwiw this concerns me slightly. I'm sure a lot of people are doing
> >> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
> > 
> > postmaster.pid already contains considerably more than just the pid. e.g.
> > 4071
> > /srv/dev/pgdev-master
> > 1425396089
> > 5440
> > /tmp
> > localhost
> > 
> >    5440001  82345992
> 
> If we already have all this extra stuff, why not include an actual error
> message then, or at least the first line of an error (or maybe just swap
> any newlines with spaces)?

Not impossible. I can play around with that and see if it's as straightforward 
as I think it is.

On 3/3/15 11:15 AM, Jan de Visser wrote:
> On March 3, 2015 11:09:29 AM Jim Nasby wrote:
>> On 3/3/15 9:26 AM, Andres Freund wrote:
>>> On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
>>>> Fwiw this concerns me slightly. I'm sure a lot of people are doing
>>>> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
>>>
>>> postmaster.pid already contains considerably more than just the pid. e.g.
>>> 4071
>>> /srv/dev/pgdev-master
>>> 1425396089
>>> 5440
>>> /tmp
>>> localhost
>>>
>>>     5440001  82345992
>>
>> If we already have all this extra stuff, why not include an actual error
>> message then, or at least the first line of an error (or maybe just swap
>> any newlines with spaces)?
>
> Not impossible. I can play around with that and see if it's as straightforward
> as I think it is.

I'm sure the code side of this is trivial; it's a question of why Tom 
was objecting. It would probably be better for us to come to a 
conclusion before working on this.

On a related note... something else we could do here would be to keep a 
last-known-good copy of the config files around. That way if you flubbed 
something at least the server would still start. I do think that any 
admin worth anything would notice an error from pg_ctl, but maybe others 
have a different opinion.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 2015-03-03 11:09:29 -0600, Jim Nasby wrote:
> On 3/3/15 9:26 AM, Andres Freund wrote:
> >On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
> >>Fwiw this concerns me slightly. I'm sure a lot of people are doing
> >>things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
> >
> >postmaster.pid already contains considerably more than just the pid. e.g.
> >4071
> >/srv/dev/pgdev-master
> >1425396089
> >5440
> >/tmp
> >localhost
> >   5440001  82345992
> 
> If we already have all this extra stuff, why not include an actual error
> message then, or at least the first line of an error (or maybe just swap any
> newlines with spaces)?

It's often several errors and such. I think knowing that it failed and
that you should look into the error log is already quite helpful
already.

Generally we obviously need some status to indicate that the config file
has been reloaded, but that could be easily combined with storing the
error message.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 3/3/15 11:33 AM, Andres Freund wrote:
> On 2015-03-03 11:09:29 -0600, Jim Nasby wrote:
>> On 3/3/15 9:26 AM, Andres Freund wrote:
>>> On 2015-03-03 15:21:24 +0000, Greg Stark wrote:
>>>> Fwiw this concerns me slightly. I'm sure a lot of people are doing
>>>> things like "kill -HUP `cat .../postmaster.pid`" or the equivalent.
>>>
>>> postmaster.pid already contains considerably more than just the pid. e.g.
>>> 4071
>>> /srv/dev/pgdev-master
>>> 1425396089
>>> 5440
>>> /tmp
>>> localhost
>>>    5440001  82345992
>>
>> If we already have all this extra stuff, why not include an actual error
>> message then, or at least the first line of an error (or maybe just swap any
>> newlines with spaces)?
>
> It's often several errors and such. I think knowing that it failed and
> that you should look into the error log is already quite helpful
> already.

It's certainly better than now, but why put DBAs through an extra step 
for no reason? Though, in the case of multiple errors perhaps it would 
be best to just report a count and point them at the log.

> Generally we obviously need some status to indicate that the config file
> has been reloaded, but that could be easily combined with storing the
> error message.

Not sure I'm following... are you saying we should include the error 
message in postmaster.pid?
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 2015-03-03 11:43:46 -0600, Jim Nasby wrote:
> It's certainly better than now, but why put DBAs through an extra step for
> no reason?

Because it makes it more complicated than it already is? It's nontrivial
to capture the output, escape it to somehow fit into a delimited field,
et al.  I'd rather have a committed improvement, than talks about a
bigger one.

> Though, in the case of multiple errors perhaps it would be best
> to just report a count and point them at the log.

It'll be confusing to have different interfaces in one/multiple error cases.

> >Generally we obviously need some status to indicate that the config file
> >has been reloaded, but that could be easily combined with storing the
> >error message.
> 
> Not sure I'm following... are you saying we should include the error message
> in postmaster.pid?

I'm saying that you'll need a way to notice that a reload was processed
or not. And that can't really be the message itself, there has to be
some other field; like the timestamp Tom proposes.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 3/3/15 11:48 AM, Andres Freund wrote:
> On 2015-03-03 11:43:46 -0600, Jim Nasby wrote:
>> >It's certainly better than now, but why put DBAs through an extra step for
>> >no reason?
> Because it makes it more complicated than it already is? It's nontrivial
> to capture the output, escape it to somehow fit into a delimited field,
> et al.  I'd rather have a committed improvement, than talks about a
> bigger one.

I don't see how this would be significantly more complex, but of course 
that's subjective.

>> >Though, in the case of multiple errors perhaps it would be best
>> >to just report a count and point them at the log.
> It'll be confusing to have different interfaces in one/multiple error cases.

If we simply don't want the code complexity then fine, but I just don't 
buy this argument. How could it possibly be confusing? Either you'll get 
the actual error message or a message that says "Didn't work, check the 
log." And the error will always be in the log no matter what. I really 
can't imagine that anyone would be unhappy that we just up-front told 
them what the error was if we could. Why make them jump through needless 
hoops?
> I'm saying that you'll need a way to notice that a reload was processed> or not. And that can't really be the message
itself,there has to be> some other field; like the timestamp Tom proposes.
 

Ahh, right. We should make sure we don't go brain-dead if the system 
clock moves backwards. I assume we couldn't just fstat the file...
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 3/3/15 11:48 AM, Andres Freund wrote:
>> It'll be confusing to have different interfaces in one/multiple error cases.

> If we simply don't want the code complexity then fine, but I just don't 
> buy this argument. How could it possibly be confusing?

What I'm concerned about is confusing the code.  There is a lot of stuff
that looks at pidfiles and a lot of it is not very bright (note upthread
argument about "cat" vs "head -1").  I don't want possibly localized
(non-ASCII) text in there, especially when there's not going to be any
sane way to know which encoding it's in.  And I definitely don't want
multiline error messages in there.

It's possible we could dumb things down enough to meet these restrictions,
but I'd really rather not go there at all.
        regards, tom lane

On March 3, 2015 04:57:58 PM Jim Nasby wrote:
> On 3/3/15 11:48 AM, Andres Freund wrote:
> >  I'm saying that you'll need a way to notice that a reload was processed
>  > or not. And that can't really be the message itself, there has to be
>  > some other field; like the timestamp Tom proposes.
> 
> Ahh, right. We should make sure we don't go brain-dead if the system 
> clock moves backwards. I assume we couldn't just fstat the file...

The timestamp field is already there (in my patch). It gets populated when the 
server starts and repopulated by SIGHUP_handler. It's the only timestamp 
pg_ctl pays attention to.

On 3/3/15 5:13 PM, Tom Lane wrote:
> Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
>> On 3/3/15 11:48 AM, Andres Freund wrote:
>>> It'll be confusing to have different interfaces in one/multiple error cases.
>
>> If we simply don't want the code complexity then fine, but I just don't
>> buy this argument. How could it possibly be confusing?
>
> What I'm concerned about is confusing the code.  There is a lot of stuff
> that looks at pidfiles and a lot of it is not very bright (note upthread
> argument about "cat" vs "head -1").  I don't want possibly localized
> (non-ASCII) text in there, especially when there's not going to be any
> sane way to know which encoding it's in.  And I definitely don't want
> multiline error messages in there.

Definitely no multi-line. If we keep that restriction, couldn't we just 
dedicate one entire line to the error message? ISTM that would be safe.

> It's possible we could dumb things down enough to meet these restrictions,
> but I'd really rather not go there at all.

IMHO the added DBA convenience would be worth it (assuming we can make 
it safe). I know I'd certainly appreciate it...

On 3/3/15 5:24 PM, Jan de Visser wrote:> On March 3, 2015 04:57:58 PM 
Jim Nasby wrote:>> On 3/3/15 11:48 AM, Andres Freund wrote:>>>   I'm saying that you'll need a way to notice that a
reloadwas 
 
processed>>   > or not. And that can't really be the message itself, there has to be>>   > some other field; like the
timestampTom proposes.>>>> Ahh, right. We should make sure we don't go brain-dead if the system>> clock moves
backwards.I assume we couldn't just fstat the file...>> The timestamp field is already there (in my patch). It gets
populated
 
when the> server starts and repopulated by SIGHUP_handler. It's the only timestamp> pg_ctl pays attention to.

What happens if someone does a reload sometime in the future (perhaps 
because they've messed with the system clock for test purposes). Do we 
still detect a new reload?
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On March 3, 2015 06:34:33 PM Jim Nasby wrote:
> On 3/3/15 5:24 PM, Jan de Visser wrote:> On March 3, 2015 04:57:58 PM 
> Jim Nasby wrote:
>  >> On 3/3/15 11:48 AM, Andres Freund wrote:
>  >>>   I'm saying that you'll need a way to notice that a reload was 
> processed
>  >>   > or not. And that can't really be the message itself, there has to be
> >>   > some other field; like the timestamp Tom proposes.
>  >>
>  >> Ahh, right. We should make sure we don't go brain-dead if the system
>  >> clock moves backwards. I assume we couldn't just fstat the file...
>  >
>  > The timestamp field is already there (in my patch). It gets populated 
> when the
>  > server starts and repopulated by SIGHUP_handler. It's the only timestamp
>  > pg_ctl pays attention to.
> 
> What happens if someone does a reload sometime in the future (perhaps 
> because they've messed with the system clock for test purposes). Do we 
> still detect a new reload?

Good point, and I had that scenario in the back of my head. What I do right 
now is read the 'last reload' timestamp from postmaster.pid (which can be the 
same as the startup time, since I make postmaster write an initial timestamp 
when it starts), send the SIGHUP, and wait until I read a later one or until 
timeout. What I could do is wait until I read a *different* one, and not just 
a later one. In that case you're only SOL if you manage to time it just so 
that your new server time is *exactly* the same as your old server time when 
you did your last reload (or startup). But even in that case it would time out 
and recommend you check the log.

That whole error message thing has one gotcha BTW; it's not hard, it's just 
that I have to find a way to make it bubble up from guc_file.l. Triggering an 
error was just changing the return value from void to bool, but returning the 
error string would mean returning a char buffer which would then need to be 
freed by other callers of ProcessConfig etc etc.

* /me scratches head

But I could just rename the current ProcessConfig, make it return a buffer, 
and have a *new*, void, ProcessConfig which calls the renamed function and 
just discards the result.

As an aside: I always found it interesting how feature threads "explode" 
around here. But it figures if even something as "simple" as this gets such 
detailed input. Definitely something to get used to...

On 3/3/15 7:34 PM, Jim Nasby wrote:
> Definitely no multi-line. If we keep that restriction, couldn't we just
> dedicate one entire line to the error message? ISTM that would be safe.

But we have multiline error messages.  If we put only the first line in
the pid file, then all the tools that build on this will effectively
show users truncated information, which won't be a good experience.  If
we don't put any error message in there, then users or tools will be
more likely to look up the full message somewhere else.

Peter Eisentraut wrote:
> On 3/3/15 7:34 PM, Jim Nasby wrote:
> > Definitely no multi-line. If we keep that restriction, couldn't we just
> > dedicate one entire line to the error message? ISTM that would be safe.
> 
> But we have multiline error messages.  If we put only the first line in
> the pid file, then all the tools that build on this will effectively
> show users truncated information, which won't be a good experience.  If
> we don't put any error message in there, then users or tools will be
> more likely to look up the full message somewhere else.

Would it work to have it be multiline using here-doc syntax?  It could
be printed similarly to what psql does to error messages, with a prefix
in front of each component (ERROR, STATEMENT, etc) and a leading tab for
continuation lines.  The last line is a terminator that matches
something specified in the first error line.  This becomes pretty
complicated for a PID file, mind you.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2015-03-04 19:04:02 -0300, Alvaro Herrera wrote:
> This becomes pretty complicated for a PID file, mind you.

Yes.

Let's get the basic feature (notification of failed reloads) done
first. That will be required with or without including the error
message.  Then we can get more fancy later, if somebody really wants to
invest the time.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On March 4, 2015 11:08:09 PM Andres Freund wrote:
> Let's get the basic feature (notification of failed reloads) done
> first. That will be required with or without including the error
> message.  Then we can get more fancy later, if somebody really wants to
> invest the time.

Except for also fixing pg_reload_conf() to pay attention to what happens with 
postmaster.pid, the patch I submitted does exactly what you want I think.

And I don't mind spending time on stuff like this. I'm not smart enough to deal 
with actual, you know, database technology.

On 3/4/15 7:13 PM, Jan de Visser wrote:
> On March 4, 2015 11:08:09 PM Andres Freund wrote:
>> Let's get the basic feature (notification of failed reloads) done
>> first. That will be required with or without including the error
>> message.  Then we can get more fancy later, if somebody really wants to
>> invest the time.
>
> Except for also fixing pg_reload_conf() to pay attention to what happens with
> postmaster.pid, the patch I submitted does exactly what you want I think.
>
> And I don't mind spending time on stuff like this. I'm not smart enough to deal
> with actual, you know, database technology.

Yeah, lets at least get this wrapped and we can see about improving it.

I like the idea of doing a here-doc or similar in the .pid, though I 
think it'd be sufficient to just prefix all the continuation lines with 
a tab. An uglier option would be just striping the newlines out.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

(Please don't top post)

On April 21, 2015 07:32:05 PM Payal Singh wrote:
> I'm trying to review this patch and applied
> http://www.postgresql.org/message-id/attachment/37123/Let_pg_ctl_check_the_r
> esult_of_a_postmaster_config_reload.patch to postgres. gmake check passed
> but while starting postgres I see:
> 
> [postgres@vagrant-centos65 data]$ LOG:  incomplete data in
> "postmaster.pid": found only 5 newlines while trying to add line 8
> LOG:  redirecting log output to logging collector process
> HINT:  Future log output will appear in directory "pg_log".
> 
> 
> Also, a simple syntax error test gave no warning at all on doing a reload,
> but just as before there was an error message in the logs:
> 
> [postgres@vagrant-centos65 data]$ /usr/local/pgsql/bin/pg_ctl -D
> /usr/local/pgsql/data reload
> server signaled
> [postgres@vagrant-centos65 data]$ cd pg_log
> [postgres@vagrant-centos65 pg_log]$ ls
> postgresql-2015-04-21_232328.log  postgresql-2015-04-21_232858.log
> [postgres@vagrant-centos65 pg_log]$ grep error
> postgresql-2015-04-21_232858.log
> LOG:  syntax error in file "/usr/local/pgsql/data/postgresql.conf" line
> 211, near token "/"
> LOG:  configuration file "/usr/local/pgsql/data/postgresql.conf" contains
> errors; no changes were applied
> 
> I'm guessing since this is a patch submitted to the commitfest after the
> current one, am I too early to start reviewing it?
> 
> Payal

But, but, but...  it worked for me... :-)

I'll have a look. I'll apply my patch to a clean tree and see if any bits have 
rotted in the last month or so. 

One thing to note is that you won't get the actual error; just a message that 
reloading failed.

jan

On April 21, 2015 09:01:14 PM Jan de Visser wrote:
> On April 21, 2015 07:32:05 PM Payal Singh wrote:
> > I'm trying to review this patch and applied
> > http://www.postgresql.org/message-id/attachment/37123/Let_pg_ctl_check_the
> > _r esult_of_a_postmaster_config_reload.patch to postgres. gmake check
> > passed but while starting postgres I see:
> > 
> > [postgres@vagrant-centos65 data]$ LOG:  incomplete data in
> > "postmaster.pid": found only 5 newlines while trying to add line 8
> > LOG:  redirecting log output to logging collector process
> > HINT:  Future log output will appear in directory "pg_log".
> > 
> > 
> > Also, a simple syntax error test gave no warning at all on doing a reload,
> > but just as before there was an error message in the logs:
> > 
> > [postgres@vagrant-centos65 data]$ /usr/local/pgsql/bin/pg_ctl -D
> > /usr/local/pgsql/data reload
> > server signaled
> > [postgres@vagrant-centos65 data]$ cd pg_log
> > [postgres@vagrant-centos65 pg_log]$ ls
> > postgresql-2015-04-21_232328.log  postgresql-2015-04-21_232858.log
> > [postgres@vagrant-centos65 pg_log]$ grep error
> > postgresql-2015-04-21_232858.log
> > LOG:  syntax error in file "/usr/local/pgsql/data/postgresql.conf" line
> > 211, near token "/"
> > LOG:  configuration file "/usr/local/pgsql/data/postgresql.conf" contains
> > errors; no changes were applied
> > 
> > I'm guessing since this is a patch submitted to the commitfest after the
> > current one, am I too early to start reviewing it?
> > 
> > Payal
> 
> But, but, but...  it worked for me... :-)
> 
> I'll have a look. I'll apply my patch to a clean tree and see if any bits
> have rotted in the last month or so.
> 
> One thing to note is that you won't get the actual error; just a message
> that reloading failed.
> 
> jan

Urgh. It appears you are right. Will fix.

jan

On April 21, 2015 09:34:51 PM Jan de Visser wrote:
> On April 21, 2015 09:01:14 PM Jan de Visser wrote:
> > On April 21, 2015 07:32:05 PM Payal Singh wrote:
... snip ...
>
> Urgh. It appears you are right. Will fix.
>
> jan

Attached a new attempt. This was one from the category 'I have no idea how
that ever worked", but whatever. For reference, this is how it looks for me
(magic man-behind-the-curtain postgresql.conf editing omitted):

jan@wolverine:~/Projects/postgresql$ initdb -D data
... Bla bla bla ...
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data -l logfile start
server starting
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG:  database system was shut down at 2015-04-21 22:03:33 EDT
LOG:  database system is ready to accept connections
LOG:  autovacuum launcher started
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data reload
server signaled
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG:  database system was shut down at 2015-04-21 22:03:33 EDT
LOG:  database system is ready to accept connections
LOG:  autovacuum launcher started
LOG:  received SIGHUP, reloading configuration files
jan@wolverine:~/Projects/postgresql$ pg_ctl -D data reload
server signaled
pg_ctl: Reload of server with PID 14656 FAILED
Consult the server log for details.
jan@wolverine:~/Projects/postgresql$ tail -5 logfile
LOG:  autovacuum launcher started
LOG:  received SIGHUP, reloading configuration files
LOG:  received SIGHUP, reloading configuration files
LOG:  syntax error in file "/home/jan/Projects/postgresql/data/postgresql.conf"
line 1, near end of line
LOG:  configuration file "/home/jan/Projects/postgresql/data/postgresql.conf"
contains errors; no changes were applied
jan@wolverine:~/Projects/postgresql$

<p dir="ltr">Ah sorry, didn't realize I top posted. I'll test this new one. <p dir="ltr">Payal.<div
class="gmail_quote">OnApr 21, 2015 10:23 PM, "Jan de Visser" <<a
href="mailto:jan@de-visser.net">jan@de-visser.net</a>>wrote:<br type="attribution" /><blockquote class="gmail_quote"
style="margin:00 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On April 21, 2015 09:34:51 PM Jan de Visser
wrote:<br/> > On April 21, 2015 09:01:14 PM Jan de Visser wrote:<br /> > > On April 21, 2015 07:32:05 PM Payal
Singhwrote:<br /> ... snip ...<br /> ><br /> > Urgh. It appears you are right. Will fix.<br /> ><br /> >
jan<br/><br /> Attached a new attempt. This was one from the category 'I have no idea how<br /> that ever worked", but
whatever.For reference, this is how it looks for me<br /> (magic man-behind-the-curtain postgresql.conf editing
omitted):<br/><br /> jan@wolverine:~/Projects/postgresql$ initdb -D data<br /> ... Bla bla bla ...<br />
jan@wolverine:~/Projects/postgresql$pg_ctl -D data -l logfile start<br /> server starting<br />
jan@wolverine:~/Projects/postgresql$tail -5 logfile<br /> LOG:  database system was shut down at 2015-04-21 22:03:33
EDT<br/> LOG:  database system is ready to accept connections<br /> LOG:  autovacuum launcher started<br />
jan@wolverine:~/Projects/postgresql$pg_ctl -D data reload<br /> server signaled<br />
jan@wolverine:~/Projects/postgresql$tail -5 logfile<br /> LOG:  database system was shut down at 2015-04-21 22:03:33
EDT<br/> LOG:  database system is ready to accept connections<br /> LOG:  autovacuum launcher started<br /> LOG: 
receivedSIGHUP, reloading configuration files<br /> jan@wolverine:~/Projects/postgresql$ pg_ctl -D data reload<br />
serversignaled<br /> pg_ctl: Reload of server with PID 14656 FAILED<br /> Consult the server log for details.<br />
jan@wolverine:~/Projects/postgresql$tail -5 logfile<br /> LOG:  autovacuum launcher started<br /> LOG:  received
SIGHUP,reloading configuration files<br /> LOG:  received SIGHUP, reloading configuration files<br /> LOG:  syntax
errorin file "/home/jan/Projects/postgresql/data/postgresql.conf"<br /> line 1, near end of line<br /> LOG: 
configurationfile "/home/jan/Projects/postgresql/data/postgresql.conf"<br /> contains errors; no changes were
applied<br/> jan@wolverine:~/Projects/postgresql$ </blockquote></div> 

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       tested, failed
Spec compliant:           not tested
Documentation:            tested, failed

Error in postgresql.conf gives the expected result on pg_ctl reload, although errors in pg_hba.conf file don't. Like
before,reload completes fine without any information that pg_hba failed to load and only information is present in the
logfile. I'm assuming pg_ctl reload should prompt user if file fails to load irrespective of which file it is -
postgresql.confor pg_hba.conf. 
 

There is no documentation change so far, but I guess that's not yet necessary. 

gmake check passed all tests.

The new status of this patch is: Waiting on Author

On April 22, 2015 06:04:42 PM Payal Singh wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:       tested, failed
> Spec compliant:           not tested
> Documentation:            tested, failed
> 
> Error in postgresql.conf gives the expected result on pg_ctl reload,
> although errors in pg_hba.conf file don't. Like before, reload completes
> fine without any information that pg_hba failed to load and only
> information is present in the log file. I'm assuming pg_ctl reload should
> prompt user if file fails to load irrespective of which file it is -
> postgresql.conf or pg_hba.conf.

Will fix. Not hard, just move the code that writes the .pid file to after the 
pg_hba reload.

> 
> There is no documentation change so far, but I guess that's not yet
> necessary.

I will update the page for pg_ctl. At least the behaviour of -w/-W in relation 
to the reload command needs explained.

> 
> gmake check passed all tests.
> 
> The new status of this patch is: Waiting on Author

jan

Jan de Visser <jan@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.

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.
        regards, tom lane

On July 3, 2015 06:21:09 PM Tom Lane wrote:
> Jan de Visser <jan@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. I have no use for this feature; 
for me it just seemed like a nice bite-sized feature to get myself acquainted 
with the code base and the development process. As far as I'm concerned that 
goal has already been achieved, even though finalizing a patch towards commit 
(and having my name in the release notes ha ha) would be the icing on the 
cake.

> 
> 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?

> 
>             regards, tom lane

On July 3, 2015 09:24:36 PM Jan de Visser wrote:
> On July 3, 2015 06:21:09 PM Tom Lane wrote:
> > 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?
> 

Heh. I actually touched that file. I completely missed those comments (or saw 
them, thought that I should update them, and then forgot about them - just as 
likely). I'll obviously fix this if we carry this to completion.

On July 3, 2015 06:21:09 PM Tom Lane wrote:
> I wonder whether we should consider inventing similar views for
> pg_hba.conf and pg_ident.conf.

(Apologies for the flurry of emails).

Was there not an attempt at a view for pg_hba.conf which ended in a lot of 
bikeshedding and no decisions?

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Jan de Visser <jan@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.

Yes.  That's definitely something that I'd been hoping someone would
work on.

Also, thanks for the work on the pg_file_settings code; I agree with all
you did there.
Thanks again!
    Stephen

On July 6, 2015 09:23:12 AM Stephen Frost wrote:
> > I wonder whether we should consider inventing similar views for
> > pg_hba.conf and pg_ident.conf.
> 
> Yes.  That's definitely something that I'd been hoping someone would
> work on.

There actually is a patch in the current CF that provides a view for pg_hba. I 
could look at one for pg_ident, which seems much simpler.

* Jan de Visser (jan@de-visser.net) wrote:
> On July 6, 2015 09:23:12 AM Stephen Frost wrote:
> > > I wonder whether we should consider inventing similar views for
> > > pg_hba.conf and pg_ident.conf.
> >
> > Yes.  That's definitely something that I'd been hoping someone would
> > work on.
>
> There actually is a patch in the current CF that provides a view for pg_hba. I
> could look at one for pg_ident, which seems much simpler.

That would be good, as would a review of the patch for pg_hba with
particular consideration of what's been done with the pg_file_settings
view.
Thanks!
    Stephen

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@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

On Thu, Jul 23, 2015 at 5:06 PM, Heikki Linnakangas wrote:
> Other comments:
> [...]

This patch had feedback, but there has been no update in the last
month, so I am now marking it as returned with feedback.
-- 
Michael

On August 25, 2015 09:31:35 PM Michael Paquier wrote:
> On Thu, Jul 23, 2015 at 5:06 PM, Heikki Linnakangas wrote:
> > Other comments:
> > [...]
> 
> This patch had feedback, but there has been no update in the last
> month, so I am now marking it as returned with feedback.

It was suggested that this mechanism became superfluous with the inclusion of 
the view postgresql.conf (pg_settings?) in 9.5. I left it to Tom (being the 
one that suggested the feature) to indicate if he still thinks it's useful.

jan

Jan de Visser <jan@de-visser.net> writes:
> On August 25, 2015 09:31:35 PM Michael Paquier wrote:
>> This patch had feedback, but there has been no update in the last
>> month, so I am now marking it as returned with feedback.

> It was suggested that this mechanism became superfluous with the inclusion of 
> the view postgresql.conf (pg_settings?) in 9.5. I left it to Tom (being the 
> one that suggested the feature) to indicate if he still thinks it's useful.

I think there's still a fair argument for "pg_ctl reload" being able to
return a simple yes-or-no result.  We had talked about trying to shoehorn
textual error messages into the protocol, and I'm now feeling that that
complication isn't needed, but a bare-bones feature would still be worth
the trouble IMO.
        regards, tom lane

On August 25, 2015 08:36:52 PM Tom Lane wrote:
> Jan de Visser <jan@de-visser.net> writes:
> > On August 25, 2015 09:31:35 PM Michael Paquier wrote:
> >> This patch had feedback, but there has been no update in the last
> >> month, so I am now marking it as returned with feedback.
> > 
> > It was suggested that this mechanism became superfluous with the inclusion
> > of the view postgresql.conf (pg_settings?) in 9.5. I left it to Tom
> > (being the one that suggested the feature) to indicate if he still thinks
> > it's useful.
> I think there's still a fair argument for "pg_ctl reload" being able to
> return a simple yes-or-no result.  We had talked about trying to shoehorn
> textual error messages into the protocol, and I'm now feeling that that
> complication isn't needed, but a bare-bones feature would still be worth
> the trouble IMO.

OK, I'll have a look at the review comments and submit something updated soon.