We catch SIGFPE with FloatExceptionHandler() in regular backends,
turning it into an ereport(ERROR). But not in aux proceses. I think the
idea is that any process that might run user code prepares to handle
SIGFPE, and other processes do not. But that's not really stated explicitly.
BackgroundWorkerMain() installs the handler if the worker is registered
with BGWORKER_BACKEND_DATABASE_CONNECTION, and sets it to SIG_IGN
otherwise. Setting SIGFPE handler to SIG_IGN is actually undefined behavior.
I believe none of the user-callable functions are actually expected to
hit SIGFPE. The functions in adt/float.c all have explicit checks for
invalid operations. I think we relied on catching SIGFPE in the past,
but not anymore. I tried changing the ERROR into PANIC in
FloatExceptionHandler(), and all the regression tests still pass. So we
have no coverage for that currently.
Googling for the error, I found a few non-ancient examples where people
have hit it:
- https://github.com/apache/age/issues/2514. This came from the AGE
extension. They added an explicit check for zero divisor, so they
considered it non-intentional to hit SIGFPE.
-
https://www.postgresql.org/message-id/flat/19586-bb603bf5ad9934dd%40postgresql.org.
This was in the Postgres money type. It was fixed in commit b4dfae2ffa,
and David backpatched it all the way. That's a precedent that we don't
expect any core code to hit SIGFPE.
It's actually a little scary that a division by zero can be turned into
an ERROR at any point. You might not expect a code section to be
interrupted, but a division by zero would cause it to jump out with an
ERROR anyway. Hopefully any section where it matters is marked with
START/END_CRIT_SECTION(), but I don't think we always do that for short
sections that don't call any functions, for example. I haven't seen any
examples of that in the field, all the reports where a user backend hits
SIGFPE have been safe to ERROR out. It still makes me a little uneasy.
Meanwhile, if a SIGFPE happens in an aux process, it currently just
aborts. It'd be nice to make it a slightly more informative PANIC instead.
So, I propose to:
1. Change FloatExceptionHandler() to PANIC instead of ERROR
2. Install FloatExceptionHandler() in all processes.
There might be extensions that rely on the current SIGFPE handling. I
didn't find any in a quick search, but if they exist, IMO it'd be
reasonable to require them to adapt, as long as this is noted in the
release notes.
The downside I see is that a bug like what we had in the money type
would now become a PANIC instead of an ERROR. But it would be safer.
- Heikki
