From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: SIGQUIT handling, redux
In-reply-to: 
 <CA+TgmoZc6QQoFeWJUj0c5e7bqhMaq=LnwTSFtUin-euz-u1HZw@mail.gmail.com>
References: <1850884.1599601164@sss.pgh.pa.us>
 <148145.1599703626@sss.pgh.pa.us>
 <CA+TgmoZc6QQoFeWJUj0c5e7bqhMaq=LnwTSFtUin-euz-u1HZw@mail.gmail.com>
Comments: In-reply-to Robert Haas <robertmhaas@gmail.com>
	message dated "Thu, 10 Sep 2020 10:36:23 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <251829.1599757015.1@sss.pgh.pa.us>
Date: Thu, 10 Sep 2020 12:56:55 -0400
Message-ID: <251830.1599757015@sss.pgh.pa.us>
Precedence: bulk

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Sep 9, 2020 at 10:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> bgworker_die (SIGTERM)
>> Calls ereport(FATAL).  This is surely not any safer than, say,
>> quickdie().  No, it's worse, because at least that won't try
>> to go out via proc_exit().

> I think bgworker_die() is pretty much a terrible idea.

Yeah.  It'd likely be better to insist that bgworkers handle SIGTERM
the same way backends do, via CHECK_FOR_INTERRUPTS.  Not sure how big
a change that would be.

> I think that the only way this could actually
> be safe is if you have a background worker that never uses ereport()
> itself, so that the ereport() in the signal handler can't be
> interrupting one that's already happening.

That doesn't begin to make it safe, because quite aside from anything
that happens in elog.c, we're then going to go out via proc_exit()
which will invoke who knows what.

>> StandbyDeadLockHandler (from SIGALRM)
>> StandbyTimeoutHandler (ditto)
>> Calls CancelDBBackends, which just for starters tries to acquire
>> an LWLock.  I think the only reason we've gotten away with this
>> for this long is the high probability that by the time either
>> timeout fires, we're going to be blocked on a semaphore.

> Yeah, I'm not sure these are so bad. In fact, in the deadlock case, I
> believe the old coding was designed to make sure we *had to* be
> blocked on a semaphore, but I'm not sure whether that's still true.

Looking at this closer, the only code that could get interrupted
by these timeouts is a call to ProcWaitForSignal, which is

void
ProcWaitForSignal(uint32 wait_event_info)
{
    (void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
                     wait_event_info);
    ResetLatch(MyLatch);
    CHECK_FOR_INTERRUPTS();
}

Interrupting the latch operations might be safe enough,
although now I'm casting a side-eye at Munro's recent
changes to share WaitEvent state all over the place.
I wonder whether blocking on an LWLock inside the
signal handler will break an interrupted WaitLatch.

Also, man that CHECK_FOR_INTERRUPTS() looks like trouble.
Could we take that out?

			regards, tom lane