Обсуждение: Re: pgsql: Use SIGURG rather than SIGUSR1 for latches.

Thomas Munro <tmunro@postgresql.org> writes:
> Use SIGURG rather than SIGUSR1 for latches.

I notice that postmaster.c still does

#ifdef SIGURG
    pqsignal_pm(SIGURG, SIG_IGN);          /* ignored */
#endif

It appears to me that this should now read

    pqsignal_pm(SIGURG, dummy_handler);    /* unused, reserve for children */

for the reasons explained in the comment for dummy_handler.

It's possible that that argument doesn't apply to the way SIGURG is used
in this patch, but I don't see a good reason to ignore the convention of
setting up the handler this way.

            regards, tom lane

On Thu, Apr 15, 2021 at 2:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It's possible that that argument doesn't apply to the way SIGURG is used
> in this patch, but I don't see a good reason to ignore the convention of
> setting up the handler this way.

Yeah, will fix.  I don't think there is a bug here given the way
latches use shared memory flags, but it might as well be consistent.

On Thu, Apr 15, 2021 at 10:50 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Thu, Apr 15, 2021 at 2:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > It's possible that that argument doesn't apply to the way SIGURG is used
> > in this patch, but I don't see a good reason to ignore the convention of
> > setting up the handler this way.
>
> Yeah, will fix.  I don't think there is a bug here given the way
> latches use shared memory flags, but it might as well be consistent.

Here's a patch to change that.  But... on second thoughts, and after
coming up with a commit message to explain the change, I'm not 100%
convinced it's worth committing.  You can't get SIGURG without
explicitly asking for it (by setting maybe_sleeping), which makes it a
bit more like SIGALRM than SIGUSR2.  I don't feel very strongly about
this though.  What do you think?

Thomas Munro <thomas.munro@gmail.com> writes:
> Here's a patch to change that.  But... on second thoughts, and after
> coming up with a commit message to explain the change, I'm not 100%
> convinced it's worth committing.  You can't get SIGURG without
> explicitly asking for it (by setting maybe_sleeping), which makes it a
> bit more like SIGALRM than SIGUSR2.  I don't feel very strongly about
> this though.  What do you think?

Hmm, yeah, after looking more closely at InitializeLatchSupport I agree.
There's so much platform variability in whether we use the signal handler
at all that it seems best to keep it SIGIGN'd until we reach that code.

It might be good to extend the comment in postmaster.c though, perhaps
along the lines of "Ignore SIGURG for now.  Child processes may change
this (see InitializeLatchSupport), but they will not receive any such
signals until they wait on a latch".

Is it really necessary to mess with UnBlockSig?

            regards, tom lane

On Sat, Apr 17, 2021 at 12:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It might be good to extend the comment in postmaster.c though, perhaps
> along the lines of "Ignore SIGURG for now.  Child processes may change
> this (see InitializeLatchSupport), but they will not receive any such
> signals until they wait on a latch".

Ok, will do.

> Is it really necessary to mess with UnBlockSig?

It's necessary to keep it blocked, because, to quote signalfd(2):

       Normally,  the  set  of  signals to be received via the file descriptor
       should be blocked using sigprocmask(2), to prevent  the  signals  being
       handled according to their default dispositions.

It does seem a little strange to have a sigset_t called UnBlockSig
that leaves one signal blocked, but it still fits this description
(from which I removed the parenthetical question):

  * UnBlockSig is the set of signals to block when we don't want to block
- * signals (is this ever nonzero??)
+ * signals.

There is also clear warning near that:

+       /* Note: InitializeLatchSupport() modifies UnBlockSig. */

I admit that it's a little unpleasant that it does that, but I
couldn't find a better way, considering the dependency on build
options and details known only to latch.c.  In earlier versions I
posted of that patch set, I did consider an alternative where
pqsignal.c would ask latch.c if it should be blocked, but it seemed
uglier.

The kqueue designers made a different choice for EVFILT_SIGNAL:

                         ...  This coexists with the signal() and
                         sigaction() facilities, and has a lower precedence.
                         The filter will record all attempts to deliver a
                         signal to a process, even if the signal has been
                         marked as SIG_IGN, ...

So in kqueue builds, it's not necessary to block it, because SIG_IGN
is enough to redirect the signal to the kqueue (and blocking it would
prevent kqueue from receiving it IIRC).  All the calls to set the
disposition to SIG_IGN explicitly are probably unnecessary since
that's the default disposition, but I figured that was somehow useful
as documentation, and a place to hang a comment.

Thomas Munro <thomas.munro@gmail.com> writes:
> On Sat, Apr 17, 2021 at 12:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Is it really necessary to mess with UnBlockSig?

> It's necessary to keep it blocked, because, to quote signalfd(2):

>        Normally,  the  set  of  signals to be received via the file descriptor
>        should be blocked using sigprocmask(2), to prevent  the  signals  being
>        handled according to their default dispositions.

Meh.  OK.

(I would've thought that a SIG_IGN'd signal would be dropped
immediately even if blocked; that's the behavior that dummy_handler
is designed to prevent, and I'm pretty sure that that code is there
because we saw it actually behaving that way on some platforms.
But apparently not on Linux?)

> ... All the calls to set the
> disposition to SIG_IGN explicitly are probably unnecessary since
> that's the default disposition, but I figured that was somehow useful
> as documentation, and a place to hang a comment.

Agreed, I would not suggest removing those.

            regards, tom lane

On Sat, Apr 17, 2021 at 8:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> (I would've thought that a SIG_IGN'd signal would be dropped
> immediately even if blocked; that's the behavior that dummy_handler
> is designed to prevent, and I'm pretty sure that that code is there
> because we saw it actually behaving that way on some platforms.
> But apparently not on Linux?)

Yeah, it's a point of variation between platforms.

POSIX: "If the action associated with a blocked signal is to ignore
the signal and if that signal is generated for the process, it is
unspecified whether the signal is discarded immediately upon
generation or remains pending."
Linux: "Blocked signals are never ignored, since the signal handler
may change by the time it is unblocked."
BSDs, Darwin: "If the signal is being ignored, then we forget about it
immediately."

On Sun, Apr 18, 2021 at 1:28 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> Linux: "Blocked signals are never ignored, since the signal handler
> may change by the time it is unblocked."

I should add, that's what the source says.  The man page for
sigpending(2) makes the opposite claim in its NOTES section on my
Debian system (it helpfully describes the BSD behaviour instead), but
the man page is demonstrably wrong.