bug in SignalSomeChildren
Hi,
I found a bug which always prevents SignalSomeChildren with
BACKEND_TYPE_WALSND from sending a signal to walsender.
Though currently SignalSomeChildren with BACKEND_TYPE_WALSND
has not been called anywhere, it's not hard to believe that will
be called in the future. So we should apply the following change.
----------------------
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 6f934ee..2d86fb6 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3162,7 +3162,8 @@ SignalSomeChildren(int signal, int target) if (bp->dead_end)
continue;
- if (!(target & BACKEND_TYPE_NORMAL) && !bp->is_autovacuum)
+ if (!(target & BACKEND_TYPE_NORMAL) && !bp->is_autovacuum &&
+ !IsPostmasterChildWalSender(bp->child_slot))
continue;
if (!(target & BACKEND_TYPE_AUTOVAC) && bp->is_autovacuum)
continue;
----------------------Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Tue, Dec 14, 2010 at 10:54 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
I found a bug which always prevents SignalSomeChildren with
BACKEND_TYPE_WALSND from sending a signal to walsender.Though currently SignalSomeChildren with BACKEND_TYPE_WALSND
has not been called anywhere, it's not hard to believe that will
be called in the future. So we should apply the following change.
I think the attached might be a little tidier. Thoughts?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
signal-some-children.patchapplication/octet-stream; name=signal-some-children.patchDownload+8-6
Excerpts from Robert Haas's message of vie dic 17 10:08:04 -0300 2010:
On Tue, Dec 14, 2010 at 10:54 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
I found a bug which always prevents SignalSomeChildren with
BACKEND_TYPE_WALSND from sending a signal to walsender.Though currently SignalSomeChildren with BACKEND_TYPE_WALSND
has not been called anywhere, it's not hard to believe that will
be called in the future. So we should apply the following change.I think the attached might be a little tidier. Thoughts?
Yeah, that looks more readable to me.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Robert Haas <robertmhaas@gmail.com> writes:
I think the attached might be a little tidier. Thoughts?
I'm not really thrilled at the idea of calling
IsPostmasterChildWalSender for every child whether or not it will have
any impact on the decision. That involves touching shared memory which
can be rather expensive (see previous discussions about shared cache
lines and so forth).
The existing coding is pretty ugly, I agree.
regards, tom lane
On Fri, Dec 17, 2010 at 10:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I think the attached might be a little tidier. Thoughts?
I'm not really thrilled at the idea of calling
IsPostmasterChildWalSender for every child whether or not it will have
any impact on the decision. That involves touching shared memory which
can be rather expensive (see previous discussions about shared cache
lines and so forth).
The existing code already does that, unless I'm missing something. We
could improve on my proposed patch a bit by doing the is_autovacuum
test first and the walsender test second. I'm not sure how to improve
on it beyond that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Tom Lane's message of vie dic 17 12:27:30 -0300 2010:
Robert Haas <robertmhaas@gmail.com> writes:
I think the attached might be a little tidier. Thoughts?
I'm not really thrilled at the idea of calling
IsPostmasterChildWalSender for every child whether or not it will have
any impact on the decision. That involves touching shared memory which
can be rather expensive (see previous discussions about shared cache
lines and so forth).
Is it possible to save the "is walsender" flag in the Backend struct?
That would make it possible to solve the problem very easily.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Dec 17, 2010 at 10:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm not really thrilled at the idea of calling
IsPostmasterChildWalSender for every child whether or not it will have
any impact on the decision. �That involves touching shared memory which
can be rather expensive (see previous discussions about shared cache
lines and so forth).
The existing code already does that, unless I'm missing something.
No, it only makes that call when it's actually relevant to the decision
and it's exhausted all other possible tests. In particular, the call is
never made for target = ALL which I think is the common case.
regards, tom lane
Alvaro Herrera <alvherre@commandprompt.com> writes:
Is it possible to save the "is walsender" flag in the Backend struct?
That would make it possible to solve the problem very easily.
Yeah, I was wondering about that too, but the problem is that the
postmaster doesn't know that at the time it forks the child. The
flag in shared memory will get set later, but it's hard to tell
how much later.
Of course, that observation also means that anyplace the postmaster
tries to distinguish walsenders from other children is fundamentally
broken anyhow: a walsender that hasn't set the flag yet will get
treated like a regular backend.
I think what we ought to be looking to do is get rid of the distinction,
so that the postmaster treats walsenders the same as other children.
regards, tom lane
On Fri, Dec 17, 2010 at 11:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think what we ought to be looking to do is get rid of the distinction,
so that the postmaster treats walsenders the same as other children.
It's not apparent to me that the existing places where postmaster.c
makes that distinction are in fact correct.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Fujii Masao's message of mié dic 15 00:54:39 -0300 2010:
Hi,
I found a bug which always prevents SignalSomeChildren with
BACKEND_TYPE_WALSND from sending a signal to walsender.Though currently SignalSomeChildren with BACKEND_TYPE_WALSND
has not been called anywhere, it's not hard to believe that will
be called in the future. So we should apply the following change.
BTW doesn't CountChildren have the same problem?
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Excerpts from Tom Lane's message of vie dic 17 13:18:35 -0300 2010:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Is it possible to save the "is walsender" flag in the Backend struct?
That would make it possible to solve the problem very easily.Yeah, I was wondering about that too, but the problem is that the
postmaster doesn't know that at the time it forks the child. The
flag in shared memory will get set later, but it's hard to tell
how much later.
Yeah, I arrived at the same conclusion. I was wondering if we could
cache the result of the shared mem lookup the first time it was done,
but as you say there would still be a race condition.
I think what we ought to be looking to do is get rid of the distinction,
so that the postmaster treats walsenders the same as other children.
I think the problem with this is that walsenders are treated in a very
special way during shutdown -- they need to stay up until after bgwriter
is gone.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Tom Lane's message of vie dic 17 13:18:35 -0300 2010:
I think what we ought to be looking to do is get rid of the distinction,
so that the postmaster treats walsenders the same as other children.
I think the problem with this is that walsenders are treated in a very
special way during shutdown -- they need to stay up until after bgwriter
is gone.
Why do they need to survive the bgwriter? And more to the point, why
does that logic need to be implemented on the postmaster side? Since
only the child process really knows reliably whether it's a walsender,
it'd be far safer if the behavioral difference could be handled on the
child side. I haven't looked at the details, but I'm wondering if we
couldn't make this go by having walsender children react differently
to the same signals the postmaster sends other children.
regards, tom lane
On Fri, Dec 17, 2010 at 11:43 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Tom Lane's message of vie dic 17 13:18:35 -0300 2010:
I think what we ought to be looking to do is get rid of the distinction,
so that the postmaster treats walsenders the same as other children.I think the problem with this is that walsenders are treated in a very
special way during shutdown -- they need to stay up until after bgwriter
is gone.Why do they need to survive the bgwriter? And more to the point, why
does that logic need to be implemented on the postmaster side? Since
only the child process really knows reliably whether it's a walsender,
it'd be far safer if the behavioral difference could be handled on the
child side. I haven't looked at the details, but I'm wondering if we
couldn't make this go by having walsender children react differently
to the same signals the postmaster sends other children.
I'm not too sure we're shutting down the WAL senders right now. I
think they may just be exiting on their own when the postmaster goes
away.
/*
* Emergency bailout if postmaster has died. This is
to avoid the
* necessity for manual cleanup of all postmaster children.
*/
if (!PostmasterIsAlive(true))
exit(1);
I'm having a bit of trouble confirming this on MacOS X, though.
Attaching gdb to either the startup process or a WAL sender causes
PostmasterIsAlive to return false, resulting in a near-immediate exit.
Seems pretty stupid for attaching gdb to change the return value of
getppid() but it seems like that must be what's happening.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 17.12.2010 19:08, Robert Haas wrote:
On Fri, Dec 17, 2010 at 11:43 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera<alvherre@commandprompt.com> writes:
Excerpts from Tom Lane's message of vie dic 17 13:18:35 -0300 2010:
I think what we ought to be looking to do is get rid of the distinction,
so that the postmaster treats walsenders the same as other children.I think the problem with this is that walsenders are treated in a very
special way during shutdown -- they need to stay up until after bgwriter
is gone.Why do they need to survive the bgwriter?
Because we want the shutdown checkpoint record that bgwriter writes just
before it dies to be replicated to the standbys.
And more to the point, why
does that logic need to be implemented on the postmaster side? Since
only the child process really knows reliably whether it's a walsender,
it'd be far safer if the behavioral difference could be handled on the
child side. I haven't looked at the details, but I'm wondering if we
couldn't make this go by having walsender children react differently
to the same signals the postmaster sends other children.I'm not too sure we're shutting down the WAL senders right now.
Sure we do. postmaster sends walsenders SIGUSR2 when bgwriter dies. When
a walsender receives SIGUSR2, it tries to send all pending WAL, and
terminates after that. The postmaster goes into PM_SHUTDOWN_2 state,
waiting for all the walsenders and the archiver process to die.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Sat, Dec 18, 2010 at 1:24 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Fujii Masao's message of mié dic 15 00:54:39 -0300 2010:
Hi,
I found a bug which always prevents SignalSomeChildren with
BACKEND_TYPE_WALSND from sending a signal to walsender.Though currently SignalSomeChildren with BACKEND_TYPE_WALSND
has not been called anywhere, it's not hard to believe that will
be called in the future. So we should apply the following change.BTW doesn't CountChildren have the same problem?
Oh, yes. We should fix that, too.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Excerpts from Robert Haas's message of vie dic 17 14:08:04 -0300 2010:
I'm having a bit of trouble confirming this on MacOS X, though.
Attaching gdb to either the startup process or a WAL sender causes
PostmasterIsAlive to return false, resulting in a near-immediate exit.
Seems pretty stupid for attaching gdb to change the return value of
getppid() but it seems like that must be what's happening.
Yeah, this problem has been known for some time and causes quite some
pain. We have an open problem report on autovacuum failing to run after
some time, and we haven't been able to get a backtrace or strace because
of this issue -- trying to attach to it causes a full system restart
(PostmasterIsAlive returns false, autovac launcher dies, this death
causes postmaster to shut everything down and restart). This is on
FreeBSD.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
... We have an open problem report on autovacuum failing to run after
some time, and we haven't been able to get a backtrace or strace because
of this issue ...
I wonder whether that's the already-fixed problem with autovacuum cost
limit going to zero in long-lived workers.
regards, tom lane
On Mon, Dec 20, 2010 at 8:20 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Robert Haas's message of vie dic 17 14:08:04 -0300 2010:
I'm having a bit of trouble confirming this on MacOS X, though.
Attaching gdb to either the startup process or a WAL sender causes
PostmasterIsAlive to return false, resulting in a near-immediate exit.
Seems pretty stupid for attaching gdb to change the return value of
getppid() but it seems like that must be what's happening.Yeah, this problem has been known for some time and causes quite some
pain. We have an open problem report on autovacuum failing to run after
some time, and we haven't been able to get a backtrace or strace because
of this issue -- trying to attach to it causes a full system restart
(PostmasterIsAlive returns false, autovac launcher dies, this death
causes postmaster to shut everything down and restart). This is on
FreeBSD.
Can we add a develop option to force use of the kill(0) method?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
Attaching gdb to either the startup process or a WAL sender causes
PostmasterIsAlive to return false, resulting in a near-immediate exit.
�Seems pretty stupid for attaching gdb to change the return value of
getppid() but it seems like that must be what's happening.
Can we add a develop option to force use of the kill(0) method?
How will that avoid needing to have an honest answer from getppid()?
Without that you can't know what to issue kill() against.
Seems like the correct path here is to complain to gdb and/or BSD
upstreams about this misbehavior.
regards, tom lane
On Mon, Dec 20, 2010 at 1:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
Attaching gdb to either the startup process or a WAL sender causes
PostmasterIsAlive to return false, resulting in a near-immediate exit.
Seems pretty stupid for attaching gdb to change the return value of
getppid() but it seems like that must be what's happening.Can we add a develop option to force use of the kill(0) method?
How will that avoid needing to have an honest answer from getppid()?
Without that you can't know what to issue kill() against.
The answer to this question will probably be entirely self-evident if
you stare at PostmasterIsAlive() for, well, it took me about 10
seconds. So probably less than five for you.
Seems like the correct path here is to complain to gdb and/or BSD
upstreams about this misbehavior.
That might be a good thing to do too, but even if they agree to fix it
and do in fact fix it right away, it'll take months or years before
all of the major PostgreSQL contributors can benefit from those fixes,
as opposed to, say, this afternoon.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
