Re: Rethinking placement of latch self-pipe initialization

Sean Chittenden recently reported that 9.2 can crash after logging
"FATAL: pipe() failed" if the kernel is short of file descriptors:
http://archives.postgresql.org/pgsql-general/2012-10/msg00202.php

The only match to that error text is in initSelfPipe(). What I believe
is happening is that InitProcess is calling OwnLatch which calls
initSelfPipe, and the latter fails, and then the postmaster thinks that
was a backend crash because we have armed the dead-man switch but not
set up on_shmem_exit(ProcKill) which would disarm it.

It's possible we could fix this by changing the order of operations
in InitProcess and OwnLatch, but it'd be tricky, not least because
ProcKill calls DisownLatch which asserts that OwnLatch was done.

What I think would be a better idea is to fix things so that OwnLatch
cannot fail except as a result of internal logic errors, by splitting
out the acquisition of the self-pipe into a separate function named say
InitializeLatchSupport. The question then becomes where to put the
InitializeLatchSupport calls. My first thought is to put them near the
signal-setup stanzas for the various processes (ie, the pqsignal calls)
similarly to what we did recently for initialization of timeout support.
However there might be a better idea.

Comments?

regards, tom lane

On Sunday, October 07, 2012 10:58 PM Tom Lane wrote:
> Sean Chittenden recently reported that 9.2 can crash after logging
> "FATAL: pipe() failed" if the kernel is short of file descriptors:
> http://archives.postgresql.org/pgsql-general/2012-10/msg00202.php
>
> The only match to that error text is in initSelfPipe(). What I believe
> is happening is that InitProcess is calling OwnLatch which calls
> initSelfPipe, and the latter fails, and then the postmaster thinks that
> was a backend crash because we have armed the dead-man switch but not
> set up on_shmem_exit(ProcKill) which would disarm it.
>
> It's possible we could fix this by changing the order of operations
> in InitProcess and OwnLatch, but it'd be tricky, not least because
> ProcKill calls DisownLatch which asserts that OwnLatch was done.

Why can't before Disowning, it can be checked that if it's already owned
or not.
Something similar is done for lwlockreleaseall() where it checks if it
holds any locks
then only it frees it. Also similar is done in SyncRepCleanupAtProcExit().

> What I think would be a better idea is to fix things so that OwnLatch
> cannot fail except as a result of internal logic errors, by splitting
> out the acquisition of the self-pipe into a separate function named say
> InitializeLatchSupport. The question then becomes where to put the
> InitializeLatchSupport calls. My first thought is to put them near the
> signal-setup stanzas for the various processes (ie, the pqsignal calls)
> similarly to what we did recently for initialization of timeout support.
> However there might be a better idea.
>
> Comments?

With Regards,
Amit Kapila.

On 7 October 2012 18:27, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Sean Chittenden recently reported that 9.2 can crash after logging
> "FATAL: pipe() failed" if the kernel is short of file descriptors:
> http://archives.postgresql.org/pgsql-general/2012-10/msg00202.php
>
> The only match to that error text is in initSelfPipe(). What I believe
> is happening is that InitProcess is calling OwnLatch which calls
> initSelfPipe, and the latter fails, and then the postmaster thinks that
> was a backend crash because we have armed the dead-man switch but not
> set up on_shmem_exit(ProcKill) which would disarm it.
>
> It's possible we could fix this by changing the order of operations
> in InitProcess and OwnLatch, but it'd be tricky, not least because
> ProcKill calls DisownLatch which asserts that OwnLatch was done.
>
> What I think would be a better idea is to fix things so that OwnLatch
> cannot fail except as a result of internal logic errors, by splitting
> out the acquisition of the self-pipe into a separate function named say
> InitializeLatchSupport. The question then becomes where to put the
> InitializeLatchSupport calls. My first thought is to put them near the
> signal-setup stanzas for the various processes (ie, the pqsignal calls)
> similarly to what we did recently for initialization of timeout support.
> However there might be a better idea.
>
> Comments?

We still have to consider how Postgres would operate without the
latches. I don't see that it can, so a shutdown seems appropriate. Is
the purpose of this just to allow a cleaner and more informative
shutdown? Or do you think we can avoid?

If we did move the init calls, would that alter things for code that
creates new used defined latches?

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

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> We still have to consider how Postgres would operate without the
> latches. I don't see that it can, so a shutdown seems appropriate. Is
> the purpose of this just to allow a cleaner and more informative
> shutdown? Or do you think we can avoid?

The point is that right now, if a new backend fails its initialization
at this specific step, that gets translated into a database-wide crash
and restart cycle, for no good reason. It should just result in that
particular session failing.

> If we did move the init calls, would that alter things for code that
> creates new used defined latches?

Only to the extent that it'd have to make sure it called the
initialization function at some appropriate point. It's not like
required initialization functions are a foreign concept in our code.

regards, tom lane

I wrote:
> Sean Chittenden recently reported that 9.2 can crash after logging
> "FATAL: pipe() failed" if the kernel is short of file descriptors:
> http://archives.postgresql.org/pgsql-general/2012-10/msg00202.php
> ...
> What I think would be a better idea is to fix things so that OwnLatch
> cannot fail except as a result of internal logic errors, by splitting
> out the acquisition of the self-pipe into a separate function named say
> InitializeLatchSupport. The question then becomes where to put the
> InitializeLatchSupport calls. My first thought is to put them near the
> signal-setup stanzas for the various processes (ie, the pqsignal calls)
> similarly to what we did recently for initialization of timeout support.

I experimented with that approach, and found out it didn't work at all,
because there are assorted code paths in which InitProcess and
InitAuxiliaryProcess are called well before we enable signals.
I find that a bit disturbing; it means there is a significant amount
of process-startup code that has a latch available but can't actually
wait on the latch. (Well, it could, but it would never awaken because
it would never react to the signal.) At some point in the future we
may have to do some refactoring in this area. On the other hand, moving
signal-enabling earlier carries its own hazards: the signal handlers
might expect yet other infrastructure to be alive already, and any bug
of that ilk would probably be a lot harder to reproduce and diagnose.

Anyway, the simplest working solution proved to be to put the
InitializeLatchSupport calls in InitProcess and InitAuxiliaryProcess,
plus add them in a few background process types that use InitLatch but
don't call either of those functions. Patch attached. Any objections?

regards, tom lane

On 14 October 2012 19:17, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Anyway, the simplest working solution proved to be to put the
> InitializeLatchSupport calls in InitProcess and InitAuxiliaryProcess,
> plus add them in a few background process types that use InitLatch but
> don't call either of those functions. Patch attached. Any objections?

Looks good to me.

--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services