Re: bgworker crashed or not?

In 9.3 I noticed that postmaster considers bgworker crashed (and
therefore tries to restart it) even if it has exited with zero status code.

I first thought about a patch like the one below, but then noticed that
postmaster.c:bgworker_quickdie() signal handler exits with 0 too (when
there's no success). Do we need my patch, my patch + <something for the
handler> or no patch at all?

// Antonin Houska (Tony)

diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 0957e91..0313fd7 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -2791,11 +2814,7 @@ reaper(SIGNAL_ARGS)

/* Was it one of our background workers? */
if (CleanupBackgroundWorker(pid, exitstatus))
- {
- /* have it be restarted */
- HaveCrashedWorker = true;
continue;
- }

/*
* Else do standard backend child cleanup.
@@ -2851,7 +2870,10 @@ CleanupBackgroundWorker(int pid,

/* Delay restarting any bgworker that exits with a
nonzero status. */
if (!EXIT_STATUS_0(exitstatus))
+ {
rw->rw_crashed_at = GetCurrentTimestamp();
+ HaveCrashedWorker = true;
+ }
else
rw->rw_crashed_at = 0;

On Fri, Jan 31, 2014 at 12:44 PM, Antonin Houska
<antonin(dot)houska(at)gmail(dot)com> wrote:
> In 9.3 I noticed that postmaster considers bgworker crashed (and
> therefore tries to restart it) even if it has exited with zero status code.
>
> I first thought about a patch like the one below, but then noticed that
> postmaster.c:bgworker_quickdie() signal handler exits with 0 too (when
> there's no success). Do we need my patch, my patch + <something for the
> handler> or no patch at all?

I think the word "crashed" here doesn't actually mean "crashed". If a
worker dies with an exit status of other than 0 or 1, we call
HandleChildCrash(), which effects a system-wide restart. But if it
exits with code 0, we don't do that. We just set HaveCrashedWorker so
that it gets restarted immediately, and that's it. In other words,
exit(0) in a bgworker causes an immediate relaunch, and exit(1) causes
the restart interval to be respected, and exit(other) causes a
system-wide crash-and-restart cycle.

This is admittedly a weird API, and we've had some discussion of
whether to change it, but I don't know that we've reached any final
conclusion. I'm tempted to propose exactly inverting the current
meaning of exit(0). That is, make it mean "don't restart me, ever,
even if I have a restart interval configured" rather than "restart me
right away, even if I have a restart interval configured". That way,
a background process that wants to run until it accomplishes some task
could be written to exit(1) on error and exit(0) on success, which
seems quite natural.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> This is admittedly a weird API, and we've had some discussion of
> whether to change it, but I don't know that we've reached any final
> conclusion. I'm tempted to propose exactly inverting the current
> meaning of exit(0). That is, make it mean "don't restart me, ever,
> even if I have a restart interval configured" rather than "restart me
> right away, even if I have a restart interval configured". That way,
> a background process that wants to run until it accomplishes some task
> could be written to exit(1) on error and exit(0) on success, which
> seems quite natural.

So
exit(0) - done, permanently
exit(1) - done until restart interval
exit(other) - crash
and there's no way to obtain the "restart immediately" behavior?

I think this is an improvement, but it probably depends on what
you think the use-cases are for bgworkers. I can definitely see
that there is a need for a bgworker to be just plain done, though.

regards, tom lane

On Mon, Feb 3, 2014 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So
> exit(0) - done, permanently
> exit(1) - done until restart interval
> exit(other) - crash
> and there's no way to obtain the "restart immediately" behavior?

That's what I was thinking about, yes. Of course, when the restart
interval is 0, "done until restart interval" is the same as "restart
immediately", so for anyone who wants to *always* restart immediately
there is no problem. Where you will run into trouble is if you
sometimes want to wait for the restart interval and other times want
to restart immediately. But I'm not sure that's a real use case. If
it is, I suggest that we assign it some other, more obscure exit code
and reserve 0 and 1 for what I believe will probably be the common
cases.

It would be potentially more useful and more general to have a
function BackgroundWorkerSetMyRestartInterval() or similar. That way,
a background worker could choose to get restarted after whatever
amount of time it likes in a particular instance, rather than only 0
or the interval configured at registration time. But I'm not that
excited about implementing that, and certainly not for 9.4. It's not
clear that there's a real need, and there are thorny questions like
"should a postmaster crash and restart cycle cause an immediate
restart?" and "what if some other process wants to poke that
background worker and cause it to restart sooner?". There are lots of
interesting and potentially useful behaviors here, but I'm wary of
letting the engineering getting out ahead of the use cases.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Feb 3, 2014 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> So
>> exit(0) - done, permanently
>> exit(1) - done until restart interval
>> exit(other) - crash
>> and there's no way to obtain the "restart immediately" behavior?

> That's what I was thinking about, yes. Of course, when the restart
> interval is 0, "done until restart interval" is the same as "restart
> immediately", so for anyone who wants to *always* restart immediately
> there is no problem. Where you will run into trouble is if you
> sometimes want to wait for the restart interval and other times want
> to restart immediately. But I'm not sure that's a real use case. If
> it is, I suggest that we assign it some other, more obscure exit code
> and reserve 0 and 1 for what I believe will probably be the common
> cases.

Agreed, but after further reflection it seems like if you've declared
a restart interval, then "done until restart interval" is probably the
common case. So how about

exit(0) - done until restart interval, or permanently if there is none
exit(1) - done permanently, even if a restart interval was declared
exit(other) - crash

I don't offhand see a point in an "exit and restart immediately" case.
Why exit at all, if you could just keep running? If you *can't* just
keep running, it probably means you know you've bollixed something,
so that the crash case is probably what to do anyway.

> It would be potentially more useful and more general to have a
> function BackgroundWorkerSetMyRestartInterval() or similar.

That might be a good idea too, but I think it's orthogonal to what
the exit codes mean.

regards, tom lane

On Mon, Feb 3, 2014 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Feb 3, 2014 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> So
>>> exit(0) - done, permanently
>>> exit(1) - done until restart interval
>>> exit(other) - crash
>>> and there's no way to obtain the "restart immediately" behavior?
>
>> That's what I was thinking about, yes. Of course, when the restart
>> interval is 0, "done until restart interval" is the same as "restart
>> immediately", so for anyone who wants to *always* restart immediately
>> there is no problem. Where you will run into trouble is if you
>> sometimes want to wait for the restart interval and other times want
>> to restart immediately. But I'm not sure that's a real use case. If
>> it is, I suggest that we assign it some other, more obscure exit code
>> and reserve 0 and 1 for what I believe will probably be the common
>> cases.
>
> Agreed, but after further reflection it seems like if you've declared
> a restart interval, then "done until restart interval" is probably the
> common case. So how about
>
> exit(0) - done until restart interval, or permanently if there is none
> exit(1) - done permanently, even if a restart interval was declared
> exit(other) - crash

I think what I proposed is better for two reasons:

1. It doesn't change the meaning of exit(1) vs. 9.3. All background
worker code I've seen or heard about (which is admittedly not all
there is) does exit(1) because the current exit(0) behavior doesn't
seem to be what anyone wants. So I don't think it matters much
whether we redefine exit(0), but redefining exit(1) will require
version-dependent changes to the background workers in contrib and,
most likely, every other background worker anyone has written.

2. If you want a worker that starts up, does something, and doesn't
need to be further restarted when that succeeds, then under your
definition you return 0 when you fail and 1 when you succeed. Under
my definition you do the opposite, which I think is more natural.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Feb 3, 2014 at 11:29 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Agreed, but after further reflection it seems like if you've declared
>> a restart interval, then "done until restart interval" is probably the
>> common case. So how about ...

> I think what I proposed is better for two reasons:

> 1. It doesn't change the meaning of exit(1) vs. 9.3. All background
> worker code I've seen or heard about (which is admittedly not all
> there is) does exit(1) because the current exit(0) behavior doesn't
> seem to be what anyone wants.

Hm. If that's actually the case, then I agree that preserving the
current behavior of exit(1) is useful. I'd been assuming we were
breaking things anyway.

regards, tom lane

On 2014-02-03 11:29:22 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, Feb 3, 2014 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> So
> >> exit(0) - done, permanently
> >> exit(1) - done until restart interval
> >> exit(other) - crash
> >> and there's no way to obtain the "restart immediately" behavior?
>
> > That's what I was thinking about, yes. Of course, when the restart
> > interval is 0, "done until restart interval" is the same as "restart
> > immediately", so for anyone who wants to *always* restart immediately
> > there is no problem. Where you will run into trouble is if you
> > sometimes want to wait for the restart interval and other times want
> > to restart immediately. But I'm not sure that's a real use case. If
> > it is, I suggest that we assign it some other, more obscure exit code
> > and reserve 0 and 1 for what I believe will probably be the common
> > cases.
>
> Agreed, but after further reflection it seems like if you've declared
> a restart interval, then "done until restart interval" is probably the
> common case. So how about
>
> exit(0) - done until restart interval, or permanently if there is none
> exit(1) - done permanently, even if a restart interval was declared
> exit(other) - crash
>
> I don't offhand see a point in an "exit and restart immediately" case.
> Why exit at all, if you could just keep running? If you *can't* just
> keep running, it probably means you know you've bollixed something,
> so that the crash case is probably what to do anyway.

There's the case where you want to quickly go over all the databases,
but only use one bgworker for it. I don't think there's another way to
do that.

I think we really should bite the bullet and change this before 9.4
comes out. The current static bgworker facility has only been out there
for one release, and dynamic bgworkers aren't released yet at all. If we
wait with this for 9.5, we'll annoy many more people.

Greetings,

Andres Freund

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

On 16/04/14 15:10, Andres Freund wrote:
>
> I think we really should bite the bullet and change this before 9.4
> comes out. The current static bgworker facility has only been out there
> for one release, and dynamic bgworkers aren't released yet at all. If we
> wait with this for 9.5, we'll annoy many more people.
>

+1

> On 2014-02-03 11:29:22 -0500, Tom Lane wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Mon, Feb 3, 2014 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> So
>>>> exit(0) - done, permanently
>>>> exit(1) - done until restart interval
>>>> exit(other) - crash
>>>> and there's no way to obtain the "restart immediately" behavior?
>>

Also I think if we do it this way, the incompatibility impact is rather
small for most existing bgworkers, like Robert I haven't seen anybody
actually using the exit code 0 currently - I am sure somebody does, but
it seems to be very small minority.

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

On Wed, Apr 16, 2014 at 9:10 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Agreed, but after further reflection it seems like if you've declared
>> a restart interval, then "done until restart interval" is probably the
>> common case. So how about
>>
>> exit(0) - done until restart interval, or permanently if there is none
>> exit(1) - done permanently, even if a restart interval was declared
>> exit(other) - crash
>>
>> I don't offhand see a point in an "exit and restart immediately" case.
>> Why exit at all, if you could just keep running? If you *can't* just
>> keep running, it probably means you know you've bollixed something,
>> so that the crash case is probably what to do anyway.
>
> There's the case where you want to quickly go over all the databases,
> but only use one bgworker for it. I don't think there's another way to
> do that.
>
> I think we really should bite the bullet and change this before 9.4
> comes out. The current static bgworker facility has only been out there
> for one release, and dynamic bgworkers aren't released yet at all. If we
> wait with this for 9.5, we'll annoy many more people.

So, exactly what do you want to change? If you want to keep the
restart-immediately behavior, then that argues for NOT changing
exit(0).

I actually think the right answer here might be to give background
workers a way to change their configured restart interval. We've
already got a shared memory area that the postmaster reads to know how
what to do when starting a dynamic background worker, so it probably
wouldn't be that hard. But I'm not volunteering to undertake that on
April 16th.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-04-16 10:37:12 -0400, Robert Haas wrote:
> On Wed, Apr 16, 2014 at 9:10 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> Agreed, but after further reflection it seems like if you've declared
> >> a restart interval, then "done until restart interval" is probably the
> >> common case. So how about
> >>
> >> exit(0) - done until restart interval, or permanently if there is none
> >> exit(1) - done permanently, even if a restart interval was declared
> >> exit(other) - crash
> >>
> >> I don't offhand see a point in an "exit and restart immediately" case.
> >> Why exit at all, if you could just keep running? If you *can't* just
> >> keep running, it probably means you know you've bollixed something,
> >> so that the crash case is probably what to do anyway.
> >
> > There's the case where you want to quickly go over all the databases,
> > but only use one bgworker for it. I don't think there's another way to
> > do that.
> >
> > I think we really should bite the bullet and change this before 9.4
> > comes out. The current static bgworker facility has only been out there
> > for one release, and dynamic bgworkers aren't released yet at all. If we
> > wait with this for 9.5, we'll annoy many more people.
>
> So, exactly what do you want to change? If you want to keep the
> restart-immediately behavior, then that argues for NOT changing
> exit(0).

I don't neccessarily want to keep that behaviour. It can be emulated
easily enough by setting the restart interval to 0. I just wanted to
explain that it's not completely unreasonable to want such a short
restart interval.

> I actually think the right answer here might be to give background
> workers a way to change their configured restart interval. We've
> already got a shared memory area that the postmaster reads to know how
> what to do when starting a dynamic background worker, so it probably
> wouldn't be that hard. But I'm not volunteering to undertake that on
> April 16th.

That seems like a separate feature - and I don't see a need to rush that
into 9.4. All I want that if we decide to change the API, we should do
it now.

What I dislike about the status quo is that the
1) The regular exit(0) behaves all but regular.
2) The only way to regularly exit is logged as a failure. Thus there's
no real way to flag a failure that's actually a failure.

So I think your proposal above already is an improvement upon the status
quo. Maybe we also should change the logging of bgworker exits.

I think we probably also need a way to exit that's treated as an error,
but doesn't lead to a PANIC restart.

Greetings,

Andres Freund

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

On Wed, Apr 16, 2014 at 11:28 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I actually think the right answer here might be to give background
>> workers a way to change their configured restart interval. We've
>> already got a shared memory area that the postmaster reads to know how
>> what to do when starting a dynamic background worker, so it probably
>> wouldn't be that hard. But I'm not volunteering to undertake that on
>> April 16th.
>
> That seems like a separate feature - and I don't see a need to rush that
> into 9.4. All I want that if we decide to change the API, we should do
> it now.

Well, I'm pretty OK with that, since I proposed it before and still
think it's important, but I'd rather leave it alone for another
release than rush into something we'll just end up changing again.

> What I dislike about the status quo is that the
> 1) The regular exit(0) behaves all but regular.
> 2) The only way to regularly exit is logged as a failure. Thus there's
> no real way to flag a failure that's actually a failure.
>
> So I think your proposal above already is an improvement upon the status
> quo.

I don't have time to write a patch for that, but I'm OK with
committing it (or having someone else commit it) even at this late
date, unless someone objects.

> Maybe we also should change the logging of bgworker exits.

It's pretty clear that the logging around bgworkers is way too verbose
for anything other than a long-running daemon, but I don't think we
should try to fix that problem right now. It needs more careful
thought than we have time to give it at this juncture. One idea might
be to let the registrant of the background worker specify a logging
level, or maybe just a flag bit to indicate verbose (LOG) or quiet
(say, DEBUG1 or DEBUG2) logging.

> I think we probably also need a way to exit that's treated as an error,
> but doesn't lead to a PANIC restart.

Why can't that be handled through ereport(ERROR/FATAL) rather than
through the choice of exit status? It seems to me that the only point
of the exit status is or should be to provide feedback to the
postmaster on how it should respond to the background worker's
untimely demise. If any other information needs to be conveyed, the
worker should log that itself rather than trying to tell the
postmaster what to log.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-04-16 11:37:47 -0400, Robert Haas wrote:
> > I think we probably also need a way to exit that's treated as an error,
> > but doesn't lead to a PANIC restart.
>
> Why can't that be handled through ereport(ERROR/FATAL) rather than
> through the choice of exit status? It seems to me that the only point
> of the exit status is or should be to provide feedback to the
> postmaster on how it should respond to the background worker's
> untimely demise. If any other information needs to be conveyed, the
> worker should log that itself rather than trying to tell the
> postmaster what to log.

I dislike that because it essentially requires the bgworker to have a
full error catching environment like PostgresMain() has. That seems
bad for many cases.

Greetings,

Andres Freund

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-04-16 11:37:47 -0400, Robert Haas wrote:
>> Why can't that be handled through ereport(ERROR/FATAL) rather than
>> through the choice of exit status?

> I dislike that because it essentially requires the bgworker to have a
> full error catching environment like PostgresMain() has. That seems
> bad for many cases.

That sounds like utter nonsense. No sane bgwriter code is going to be
able to discount the possibility of something throwing an elog(ERROR).
Now, you might not need the ability to do a transaction abort, but
you'll have to at least be able to terminate the process cleanly.

regards, tom lane

On 2014-04-16 11:54:25 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2014-04-16 11:37:47 -0400, Robert Haas wrote:
> >> Why can't that be handled through ereport(ERROR/FATAL) rather than
> >> through the choice of exit status?
>
> > I dislike that because it essentially requires the bgworker to have a
> > full error catching environment like PostgresMain() has. That seems
> > bad for many cases.
>
> That sounds like utter nonsense. No sane bgwriter code is going to be
> able to discount the possibility of something throwing an elog(ERROR).
> Now, you might not need the ability to do a transaction abort, but
> you'll have to at least be able to terminate the process cleanly.

Why? Throwing an error without an exception stack seems to work
sensibly? The error is promoted to FATAL and the normal FATAL handling
is performed? C.f. pg_re_throw() called via errfinish() in the ERRROR
case.

Greetings,

Andres Freund

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

On Wed, Apr 16, 2014 at 12:00 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-04-16 11:54:25 -0400, Tom Lane wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> > On 2014-04-16 11:37:47 -0400, Robert Haas wrote:
>> >> Why can't that be handled through ereport(ERROR/FATAL) rather than
>> >> through the choice of exit status?
>>
>> > I dislike that because it essentially requires the bgworker to have a
>> > full error catching environment like PostgresMain() has. That seems
>> > bad for many cases.
>>
>> That sounds like utter nonsense. No sane bgwriter code is going to be
>> able to discount the possibility of something throwing an elog(ERROR).
>> Now, you might not need the ability to do a transaction abort, but
>> you'll have to at least be able to terminate the process cleanly.
>
> Why? Throwing an error without an exception stack seems to work
> sensibly? The error is promoted to FATAL and the normal FATAL handling
> is performed? C.f. pg_re_throw() called via errfinish() in the ERRROR
> case.

And... so what's the problem? You seemed to be saying that the
background worker would need to a more developed error-handling
environment in order to do proper logging, but here you're saying
(rightly, I believe) that it doesn't. Even if it did, though, I think
the right solution is to install one, not make it the postmaster's job
to try to read the tea leaves in the worker's exit code.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-04-16 12:04:26 -0400, Robert Haas wrote:
> And... so what's the problem? You seemed to be saying that the
> background worker would need to a more developed error-handling
> environment in order to do proper logging, but here you're saying
> (rightly, I believe) that it doesn't. Even if it did, though, I think
> the right solution is to install one, not make it the postmaster's job
> to try to read the tea leaves in the worker's exit code.

Well, currently it will log the message that has been thrown, that might
lack context. LogChildExit() already has code to print activity of the
bgworker after it crashed.

Note that a FATAL error will always use a exit(1) - so we better not
make that a special case in the code :/.

Greetings,

Andres Freund

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

On Wed, Apr 16, 2014 at 12:11 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-04-16 12:04:26 -0400, Robert Haas wrote:
>> And... so what's the problem? You seemed to be saying that the
>> background worker would need to a more developed error-handling
>> environment in order to do proper logging, but here you're saying
>> (rightly, I believe) that it doesn't. Even if it did, though, I think
>> the right solution is to install one, not make it the postmaster's job
>> to try to read the tea leaves in the worker's exit code.
>
> Well, currently it will log the message that has been thrown, that might
> lack context. LogChildExit() already has code to print activity of the
> bgworker after it crashed.

I'm still not seeing the problem. It's the background worker's job to
make sure that the right stuff gets logged, just as it would be for
any other backend. Trying to bolt some portion of the responsibility
for that onto the postmaster is 100% wrong.

> Note that a FATAL error will always use a exit(1) - so we better not
> make that a special case in the code :/.

Well, everything's a special case, in that every error code has (and
should have) its own meaning. But the handling we give to exit code 1
is not obviously incompatible with what you probably want to have
happen after a FATAL.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-04-16 12:20:01 -0400, Robert Haas wrote:
> On Wed, Apr 16, 2014 at 12:11 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-04-16 12:04:26 -0400, Robert Haas wrote:
> >> And... so what's the problem? You seemed to be saying that the
> >> background worker would need to a more developed error-handling
> >> environment in order to do proper logging, but here you're saying
> >> (rightly, I believe) that it doesn't. Even if it did, though, I think
> >> the right solution is to install one, not make it the postmaster's job
> >> to try to read the tea leaves in the worker's exit code.
> >
> > Well, currently it will log the message that has been thrown, that might
> > lack context. LogChildExit() already has code to print activity of the
> > bgworker after it crashed.
>
> I'm still not seeing the problem. It's the background worker's job to
> make sure that the right stuff gets logged, just as it would be for
> any other backend. Trying to bolt some portion of the responsibility
> for that onto the postmaster is 100% wrong.

Well, it already has taken on that responsibility, it's not my idea to
add it. I merely want to control more precisely what happens.

> > Note that a FATAL error will always use a exit(1) - so we better not
> > make that a special case in the code :/.
>
> Well, everything's a special case, in that every error code has (and
> should have) its own meaning. But the handling we give to exit code 1
> is not obviously incompatible with what you probably want to have
> happen after a FATAL.

That was essentially directed at Tom's proposal in
http://www.postgresql.org/message-id/32224.1391444962@sss.pgh.pa.us - I
don't think that'd be compatible with FATAL errors.

Greetings,

Andres Freund

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

On Wed, Apr 16, 2014 at 12:28 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-04-16 12:20:01 -0400, Robert Haas wrote:
>> On Wed, Apr 16, 2014 at 12:11 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > On 2014-04-16 12:04:26 -0400, Robert Haas wrote:
>> >> And... so what's the problem? You seemed to be saying that the
>> >> background worker would need to a more developed error-handling
>> >> environment in order to do proper logging, but here you're saying
>> >> (rightly, I believe) that it doesn't. Even if it did, though, I think
>> >> the right solution is to install one, not make it the postmaster's job
>> >> to try to read the tea leaves in the worker's exit code.
>> >
>> > Well, currently it will log the message that has been thrown, that might
>> > lack context. LogChildExit() already has code to print activity of the
>> > bgworker after it crashed.
>>
>> I'm still not seeing the problem. It's the background worker's job to
>> make sure that the right stuff gets logged, just as it would be for
>> any other backend. Trying to bolt some portion of the responsibility
>> for that onto the postmaster is 100% wrong.
>
> Well, it already has taken on that responsibility, it's not my idea to
> add it. I merely want to control more precisely what happens.s

I think that's doubling down on an already-questionable design principle.

Or if I may be permitted a more colloquial idiom:

Luke, it's a trap.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 16/04/14 18:34, Robert Haas wrote:
> On Wed, Apr 16, 2014 at 12:28 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> On 2014-04-16 12:20:01 -0400, Robert Haas wrote:
>>> On Wed, Apr 16, 2014 at 12:11 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> I'm still not seeing the problem. It's the background worker's job to
>>> make sure that the right stuff gets logged, just as it would be for
>>> any other backend. Trying to bolt some portion of the responsibility
>>> for that onto the postmaster is 100% wrong.
>>
>> Well, it already has taken on that responsibility, it's not my idea to
>> add it. I merely want to control more precisely what happens.s
>
> I think that's doubling down on an already-questionable design principle.
>
> Or if I may be permitted a more colloquial idiom:
>
> Luke, it's a trap.
>

Well the logging is just too spammy in general when it comes to dynamic
bgworkers but that's easy to fix in the future, no need to make
decisions for 9.4.

However I really don't like that I have to exit with exit code 1, which
is normally used as failure, if I want to shutdown my dynamic bgworker
once it has finished the work. And this behavior is something we can set
properly only once...

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

On 04/17/2014 04:47 AM, Petr Jelinek wrote:
>
> Well the logging is just too spammy in general when it comes to dynamic
> bgworkers but that's easy to fix in the future, no need to make
> decisions for 9.4.

Agreed - it's the *API* that we need sorted out for 9.4, and log output
isn't something Pg tends to consider part of the API.

> However I really don't like that I have to exit with exit code 1, which
> is normally used as failure, if I want to shutdown my dynamic bgworker
> once it has finished the work. And this behavior is something we can set
> properly only once...

As far as I can tell we have a couple of options:

- Redefine what the exit codes mean so that exit 0 suppresses
auto-restart and exits silently. Probably simplest.

or

- Expose a worker's BackgroundWorkerHandle as a global within the
worker, and let it TerminateBackroundWorker(my_bgw_handle) its self.

Of those, just changing the meaning of the exit code seems simpler and
easier. It's not clear to me why it appears to be contentious.

I don't think the status quo, with no way to exit a dynamic bgworker w/o
an error, is OK. It's like those delightful "Error: Success" messages
one gets when using perror() inappropriately - deeply confusing to
users. Lets try not to be stuck with that when we can avoid it.

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

On 04/17/2014 08:35 AM, Craig Ringer wrote:
> On 04/17/2014 04:47 AM, Petr Jelinek wrote:
>>
>> Well the logging is just too spammy in general when it comes to dynamic
>> bgworkers but that's easy to fix in the future, no need to make
>> decisions for 9.4.
>
> Agreed - it's the *API* that we need sorted out for 9.4, and log output
> isn't something Pg tends to consider part of the API.
>
>> However I really don't like that I have to exit with exit code 1, which
>> is normally used as failure, if I want to shutdown my dynamic bgworker
>> once it has finished the work. And this behavior is something we can set
>> properly only once...
>
> As far as I can tell we have a couple of options:
>
> - Redefine what the exit codes mean so that exit 0 suppresses
> auto-restart and exits silently. Probably simplest.

I'm now strongly in favour of this alternative.

I've just noticed that the bgworker control interfaces do not honour
bgw.bgw_restart_time = BGW_NEVER_RESTART if you exit with status zero.

This means that it's not simply a problem where you can't say "restart
me if I crash, but not if I exit normally".

You also can't even say "never restart me at all". Because
"BGW_NEVER_RESTART" seems to really mean "BGW_NO_RESTART_ON_CRASH".

This _needs_fixing before 9.4.

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

On 24/04/14 07:39, Craig Ringer wrote:
> On 04/17/2014 08:35 AM, Craig Ringer wrote:
>>
>> As far as I can tell we have a couple of options:
>>
>> - Redefine what the exit codes mean so that exit 0 suppresses
>> auto-restart and exits silently. Probably simplest.
>
> I'm now strongly in favour of this alternative.
>
> I've just noticed that the bgworker control interfaces do not honour
> bgw.bgw_restart_time = BGW_NEVER_RESTART if you exit with status zero.
>
> This means that it's not simply a problem where you can't say "restart
> me if I crash, but not if I exit normally".
>
> You also can't even say "never restart me at all". Because
> "BGW_NEVER_RESTART" seems to really mean "BGW_NO_RESTART_ON_CRASH".
>

Yes this was one of the reasons why I complained also. Maybe it would be
enough to just honor the bgw_restart_time always.

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

On Thu, Apr 24, 2014 at 1:39 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 04/17/2014 08:35 AM, Craig Ringer wrote:
>> On 04/17/2014 04:47 AM, Petr Jelinek wrote:
>>>
>>> Well the logging is just too spammy in general when it comes to dynamic
>>> bgworkers but that's easy to fix in the future, no need to make
>>> decisions for 9.4.
>>
>> Agreed - it's the *API* that we need sorted out for 9.4, and log output
>> isn't something Pg tends to consider part of the API.
>>
>>> However I really don't like that I have to exit with exit code 1, which
>>> is normally used as failure, if I want to shutdown my dynamic bgworker
>>> once it has finished the work. And this behavior is something we can set
>>> properly only once...
>>
>> As far as I can tell we have a couple of options:
>>
>> - Redefine what the exit codes mean so that exit 0 suppresses
>> auto-restart and exits silently. Probably simplest.
>
> I'm now strongly in favour of this alternative.
>
> I've just noticed that the bgworker control interfaces do not honour
> bgw.bgw_restart_time = BGW_NEVER_RESTART if you exit with status zero.
>
> This means that it's not simply a problem where you can't say "restart
> me if I crash, but not if I exit normally".
>
> You also can't even say "never restart me at all". Because
> "BGW_NEVER_RESTART" seems to really mean "BGW_NO_RESTART_ON_CRASH".
>
> This _needs_fixing before 9.4.

It seems we have consensus on what to do about this, but what we
haven't got is a patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 28/04/14 16:27, Robert Haas wrote:
> On Thu, Apr 24, 2014 at 1:39 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> On 04/17/2014 08:35 AM, Craig Ringer wrote:
>> I've just noticed that the bgworker control interfaces do not honour
>> bgw.bgw_restart_time = BGW_NEVER_RESTART if you exit with status zero.
>>
>> This means that it's not simply a problem where you can't say "restart
>> me if I crash, but not if I exit normally".
>>
>> You also can't even say "never restart me at all". Because
>> "BGW_NEVER_RESTART" seems to really mean "BGW_NO_RESTART_ON_CRASH".
>>
>> This _needs_fixing before 9.4.
>
> It seems we have consensus on what to do about this, but what we
> haven't got is a patch.
>

If you mean the consensus that exit status 0 should mean permanent stop
then I think the patch can be as simple as attached.

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

On Mon, Apr 28, 2014 at 4:19 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> On 28/04/14 16:27, Robert Haas wrote:
>> On Thu, Apr 24, 2014 at 1:39 AM, Craig Ringer <craig(at)2ndquadrant(dot)com>
>> wrote:
>>> On 04/17/2014 08:35 AM, Craig Ringer wrote:
>>> I've just noticed that the bgworker control interfaces do not honour
>>> bgw.bgw_restart_time = BGW_NEVER_RESTART if you exit with status zero.
>>>
>>> This means that it's not simply a problem where you can't say "restart
>>> me if I crash, but not if I exit normally".
>>>
>>> You also can't even say "never restart me at all". Because
>>> "BGW_NEVER_RESTART" seems to really mean "BGW_NO_RESTART_ON_CRASH".
>>>
>>> This _needs_fixing before 9.4.
>>
>>
>> It seems we have consensus on what to do about this, but what we
>> haven't got is a patch.
>
> If you mean the consensus that exit status 0 should mean permanent stop then
> I think the patch can be as simple as attached.

Hmm. Well, at the very least, you need to update the comment.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 30/04/14 23:35, Robert Haas wrote:
> On Mon, Apr 28, 2014 at 4:19 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> On 28/04/14 16:27, Robert Haas wrote:
>>>
>>> It seems we have consensus on what to do about this, but what we
>>> haven't got is a patch.
>>
>> If you mean the consensus that exit status 0 should mean permanent stop then
>> I think the patch can be as simple as attached.
>
> Hmm. Well, at the very least, you need to update the comment.
>

Obviously, also docs, the above was just demonstration that the patch is
simple (and also all I could manage to write and test on the way from
airport to hotel).

The attached patch is more proper.

One thing I hit during going through the code is the quickdie handler.
Originally it exited with exit code 0 so that bgworker was always
restarted immediately, but since we changed meaning of exit code 0 to
keep bgworker dead forever I changed the exit code in quickdie to 2
which seems to be consistent with how other backends work. One side
effect is that when we get to normal backed crash recovery where
everything gets restarted, bgworker restart will be affected by
bgw_restart_time.

This should hopefully not be too big issue for normal bgworkers as it's
an exceptional situation. And for dynamic bgworkers this is probably
more proper behavior anyway since currently they would get restarted
immediately without their parent process knowing about them anymore (as
it also got restarted). The above BTW leads me to thinking that we might
want to include a note somewhere in the docs saying that for most
use-cases dynamic bgworkers should use bgw_restart_time =
BGW_NEVER_RESTART (this is IMHO true for basically all use-cases except
where you use them as static ones and just need to run dynamic number of
them), but I didn't include such note in the patch.

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

On Sun, May 4, 2014 at 9:57 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> On 30/04/14 23:35, Robert Haas wrote:
>> On Mon, Apr 28, 2014 at 4:19 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com>
>> wrote:
>>> On 28/04/14 16:27, Robert Haas wrote:
>>>> It seems we have consensus on what to do about this, but what we
>>>> haven't got is a patch.
>>>
>>>
>>> If you mean the consensus that exit status 0 should mean permanent stop
>>> then
>>> I think the patch can be as simple as attached.
>>
>> Hmm. Well, at the very least, you need to update the comment.
>
> Obviously, also docs, the above was just demonstration that the patch is
> simple (and also all I could manage to write and test on the way from
> airport to hotel).
>
> The attached patch is more proper.

Hmm, I see some problems here. The current comment for
bgworker_quickdie() says that we do exit(0) there rather than exit(2)
to avoid having the postmaster delay restart based on
bgw_restart_time, but with the proposed change in the meaning of
exit(2), that would have the effect of unregistering the worker, which
I guess is why you've changed it to call exit(2) instead, but then the
bgw_restart_delay applies, which I think we do not want. To make
matters more confusing, the existing comment is actually only correct
for non-shmem-connected workers - shmem-connected workers will treat
an exit status of anything other than 0 or 1 in exactly the same
fashion as a failure to disengage the deadman switch.

Which brings up another point: the behavior of non-shmem-connected
workers is totally bizarre. An exit status other than 0 or 1 is not
treated as a crash requiring a restart, but failure to disengage the
deadman switch is still treated as a crash requiring a restart. Why?
If the workers are not shmem-connected, then no crash requires a
system-wide restart. Of course, there's the tiny problem that we
aren't actually unmapping shared memory from supposedly non-shmem
connected workers, which is a different bug, but ignoring that for the
moment there's no reason for this logic to be like this.

What I'm inclined to do is change the logic so that:

(1) After a crash-and-restart sequence, zero rw->rw_crashed_at, so
that anything which is still registered gets restarted immediately.

(2) If a shmem-connected backend fails to release the deadman switch
or exits with an exit code other than 0 or 1, we crash-and-restart. A
non-shmem-connected backend never causes a crash-and-restart.

(3) When a background worker exits without triggering a
crash-and-restart, an exit code of precisely 0 causes the worker to be
unregistered; any other exit code has no special effect, so
bgw_restart_time controls.

Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/05/14 19:05, Robert Haas wrote:
> Which brings up another point: the behavior of non-shmem-connected
> workers is totally bizarre. An exit status other than 0 or 1 is not
> treated as a crash requiring a restart, but failure to disengage the
> deadman switch is still treated as a crash requiring a restart. Why?
> If the workers are not shmem-connected, then no crash requires a
> system-wide restart. Of course, there's the tiny problem that we
> aren't actually unmapping shared memory from supposedly non-shmem
> connected workers, which is a different bug, but ignoring that for the
> moment there's no reason for this logic to be like this.

Agreed.

> What I'm inclined to do is change the logic so that:
>
> (1) After a crash-and-restart sequence, zero rw->rw_crashed_at, so
> that anything which is still registered gets restarted immediately.

Yes, that's quite obvious change which I missed completely :).

> (2) If a shmem-connected backend fails to release the deadman switch
> or exits with an exit code other than 0 or 1, we crash-and-restart. A
> non-shmem-connected backend never causes a crash-and-restart.

+1

> (3) When a background worker exits without triggering a
> crash-and-restart, an exit code of precisely 0 causes the worker to be
> unregistered; any other exit code has no special effect, so
> bgw_restart_time controls.

+1

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

On 07/05/14 02:25, Petr Jelinek wrote:
> On 06/05/14 19:05, Robert Haas wrote:
>> What I'm inclined to do is change the logic so that:
>>
>> (1) After a crash-and-restart sequence, zero rw->rw_crashed_at, so
>> that anything which is still registered gets restarted immediately.
>
> Yes, that's quite obvious change which I missed completely :).
>
>> (2) If a shmem-connected backend fails to release the deadman switch
>> or exits with an exit code other than 0 or 1, we crash-and-restart. A
>> non-shmem-connected backend never causes a crash-and-restart.
>
> +1
>
>> (3) When a background worker exits without triggering a
>> crash-and-restart, an exit code of precisely 0 causes the worker to be
>> unregistered; any other exit code has no special effect, so
>> bgw_restart_time controls.
>
> +1
>

Ok, attached patch is my try at the proposed changes.

I don't like the reset_bgworker_crash_state function name, maybe you'll
come up with something better...

I passes my tests for the desired behavior, hopefully I didn't miss some
scenario.

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

On Tue, May 6, 2014 at 8:25 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> What I'm inclined to do is change the logic so that:
>>
>> (1) After a crash-and-restart sequence, zero rw->rw_crashed_at, so
>> that anything which is still registered gets restarted immediately.
>
> Yes, that's quite obvious change which I missed completely :).

I've committed the portion of your patch which does this, with pretty
extensive changes. I moved the function which resets the crash times
to bgworker.c, renamed it, and made it just reset all of the crash
times unconditionally; there didn't seem to be any point in skipping
the irrelevant ones, so it seemed best to keep things simple. I also
moved the call from reaper() where you had it to
PostmasterStateMachine(), because the old placement did not work for
me when I carried out the following steps:

1. Kill a background worker with a 60-second restart time using
SIGTERM, so that it does exit(1).
2. Before it restarts, kill a regular backend with SIGQUIT.

Let me know if you think I've got it wrong.

>> (2) If a shmem-connected backend fails to release the deadman switch
>> or exits with an exit code other than 0 or 1, we crash-and-restart. A
>> non-shmem-connected backend never causes a crash-and-restart.
>
> +1

I did this as a separate commit,
e2ce9aa27bf20eff2d991d0267a15ea5f7024cd7, just moving the check for
ReleasePostmasterChildSlot inside the if statement. Then I realized
that was bogus, so I just pushed
eee6cf1f337aa488a20e9111df446cdad770e645 to fix that. Hopefully it's
OK now.

>> (3) When a background worker exits without triggering a
>> crash-and-restart, an exit code of precisely 0 causes the worker to be
>> unregistered; any other exit code has no special effect, so
>> bgw_restart_time controls.
>
> +1

This isn't done yet.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 07/05/14 22:32, Robert Haas wrote:
> On Tue, May 6, 2014 at 8:25 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> I've committed the portion of your patch which does this, with pretty
> extensive changes. I moved the function which resets the crash times
> to bgworker.c, renamed it, and made it just reset all of the crash
> times unconditionally; there didn't seem to be any point in skipping
> the irrelevant ones, so it seemed best to keep things simple. I also
> moved the call from reaper() where you had it to
> PostmasterStateMachine(), because the old placement did not work for
> me when I carried out the following steps:
>
> 1. Kill a background worker with a 60-second restart time using
> SIGTERM, so that it does exit(1).
> 2. Before it restarts, kill a regular backend with SIGQUIT.
>
> Let me know if you think I've got it wrong.
>

No I think it's fine, I didn't try that combination and just wanted to
put it as deep in the call as possible.

>>> (2) If a shmem-connected backend fails to release the deadman switch
>>> or exits with an exit code other than 0 or 1, we crash-and-restart. A
>>> non-shmem-connected backend never causes a crash-and-restart.
>>
>> +1
>
> I did this as a separate commit,
> e2ce9aa27bf20eff2d991d0267a15ea5f7024cd7, just moving the check for
> ReleasePostmasterChildSlot inside the if statement. Then I realized
> that was bogus, so I just pushed
> eee6cf1f337aa488a20e9111df446cdad770e645 to fix that. Hopefully it's
> OK now.

The fixed one looks ok to me.

>
>>> (3) When a background worker exits without triggering a
>>> crash-and-restart, an exit code of precisely 0 causes the worker to be
>>> unregistered; any other exit code has no special effect, so
>>> bgw_restart_time controls.
>>
>> +1
>
> This isn't done yet.
>

Unless I am missing something this change was included in every patch I
sent - setting rw->rw_terminate = true; in CleanupBackgroundWorker for
zero exit code + comment changes. Or do you have objections to this
approach?

Anyway missing parts attached.

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

On Wed, May 7, 2014 at 4:55 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> This isn't done yet.
>
> Unless I am missing something this change was included in every patch I sent
> - setting rw->rw_terminate = true; in CleanupBackgroundWorker for zero exit
> code + comment changes. Or do you have objections to this approach?
>
> Anyway missing parts attached.

It was, but I felt that the different pieces of this should be
separated into separate commits, and (regardless of how I committed
it) I needed to review each change separately. I wasn't saying it was
your fault that it wasn't done; just that I hadn't gotten it committed
yet.

I've pushed your rebased patch now with some further kibitzing,
especially in regards to the documentation.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 07/05/14 23:45, Robert Haas wrote:
> It was, but I felt that the different pieces of this should be
> separated into separate commits, and (regardless of how I committed
> it) I needed to review each change separately. I wasn't saying it was
> your fault that it wasn't done; just that I hadn't gotten it committed
> yet.
>

Oh ok, I got confused and thought you might have overlooked that part
while modifying related parts of code.

> I've pushed your rebased patch now with some further kibitzing,
> especially in regards to the documentation.
>

Cool, thanks :)

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

On 05/08/2014 05:45 AM, Robert Haas wrote:
> I've pushed your rebased patch now with some further kibitzing,
> especially in regards to the documentation.

Thanks for tacking that Robert. It's great to have that API sorted out
before 9.4 hits and it's locked in.

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