Thread: Checkpoint process signal handling seems wrong

I am currently looking at a frozen system: a backend crashed during XLOG
write (which I was deliberately provoking, via running it out of disk
space), and now the postmaster is unable to recover because it's waiting
around for a checkpoint process that it had launched milliseconds before
the crash.  The checkpoint process, unfortunately, is not going to quit
anytime soon because it's hung up trying to get a spinlock that the
crashing backend left locked.

Eventually the checkpoint process will time out the spinlock and abort
(but please note that this is true only because I insisted --- Vadim
wanted to have infinite timeouts on the WAL spinlocks.  I think this is
good evidence that that's a bad idea).  However, while sitting here
looking at it I can't help wondering whether the checkpoint process
shouldn't have responded to the SIGTERM that the postmaster sent it
when the other backend crashed.

Is it really such a good idea for the checkpoint process to ignore
SIGTERM?

While we're at it: is it really such a good idea to use elog(STOP)
all over the place in the WAL stuff?  If XLogFileInit had chosen
to exit with elog(FATAL), then we would have released the spinlock
on the way out of the failing backend, and the checkpointer wouldn't
be stuck.
        regards, tom lane

> Eventually the checkpoint process will time out the spinlock and abort
> (but please note that this is true only because I insisted --- Vadim
> wanted to have infinite timeouts on the WAL spinlocks. I think this is
> good evidence that that's a bad idea).

I disagree - this is evidence of bug in implementation -:)
Timeout will take too long time - so it's not solution.
Years ago we used timeouts for deadlock detection.
Spin timeouts are yesterday too -:)

> However, while sitting here looking at it I can't help wondering whether
> the checkpoint process shouldn't have responded to the SIGTERM that the
> postmaster sent it when the other backend crashed.
> 
> Is it really such a good idea for the checkpoint process to ignore
> SIGTERM?

Seems not, SIGTERM --> elog(STOP) should be Ok here.

> While we're at it: is it really such a good idea to use elog(STOP)
> all over the place in the WAL stuff?  If XLogFileInit had chosen

I just hadn't time to consider each particular case.
It's better to restart than to break WAL rules (only bogus disks
are allowed to do this -:)).

> to exit with elog(FATAL), then we would have released the spinlock
> on the way out of the failing backend, and the checkpointer wouldn't
> be stuck.

I didn't use elog(FATAL) exactly because of it releases spins!
Who knows what and how much other backend will have time to do
if we'll release spins when things are bad. Each particular case
must be carefully considered.

Example: one backend failed to insert log record for modified data
page, releases that page write spin lock, concurrent checkpoint maker
takes read spin lock on that buffer and write it to disk before
postmaster kill it...

Vadim

"Mikheev, Vadim" <vmikheev@SECTORBASE.COM> writes:
>> However, while sitting here looking at it I can't help wondering whether
>> the checkpoint process shouldn't have responded to the SIGTERM that the
>> postmaster sent it when the other backend crashed.
>> 
>> Is it really such a good idea for the checkpoint process to ignore
>> SIGTERM?

> Seems not, SIGTERM --> elog(STOP) should be Ok here.

Yes, after further thought this seems not only desirable but
*necessary*.  Else the checkpoint maker might be writing bad data
from corrupted shmem structures, which is exactly what the system-wide
restart mechanism is supposed to prevent.

I'll fix the checkpoint process to accept SIGTERM and SIGUSR1 (but
not SIGINT) from the postmaster.


>> While we're at it: is it really such a good idea to use elog(STOP)
>> all over the place in the WAL stuff?  If XLogFileInit had chosen

> I just hadn't time to consider each particular case.

Okay.  You're right, that probably needs case-by-case thought that
we haven't time for right now.
        regards, tom lane