I had a look at recovery conflict signaling and a few things caught my
eye. No functional changes, but some cleanups and readability improvements:
Patch 0001: Remove useless errdetail_abort()
--------------------------------------------
The function is supposed to add DETAIL to errors when you are in an
aborted transaction, if the transaction was aborted by a recovery
conflict, like this:
ERROR: current transaction is aborted, commands ignored until end of
transaction block"
DETAIL: Abort reason: recovery conflict
But I don't see how to reach that. If a transaction is aborted by
recovery conflict, you get a different error like this:
ERROR: canceling statement due to conflict with recovery
DETAIL: User was holding a relation lock for too long.
The transaction abort clears the 'recoveryConflictPending' flag, so even
if that happens in a transaction block, you don't get that "DETAIL:
Abort reason: recovery conflict" in the subsequent errors.
errdetail_abort() was introduced in commit a8ce974cdd. I suppose it was
needed back then, but the signal handling has changed a lot since.
Looking at that commit now, though, I don't really understand how it was
reachable even back then. (Except with a race with an unrelated
transaction abort, see commit message)
Has anyone seen the "DETAIL: Abort reason: recovery conflict" in recent
years, or ever? If not, let's rip it out.
0002: Don't hint that you can reconnect when the database is dropped
--------------------------------------------------------------------
If you're connected to a database is being dropped, during recovery, you
get an error like this:
FATAL: terminating connection due to conflict with recovery
DETAIL: User was connected to a database that must be dropped.
HINT: In a moment you should be able to reconnect to the database and
repeat your command.
The hint seems misleading. The database is being dropped, you most
likely can *not* reconnect to it. Let's remove it.
0003-0004: Separate RecoveryConflictReasons from procsignals
-------------------------------------------------------------
We're currently using different PROCSIG_* flags to indicate different
kinds of recovery conflicts. We're also abusing the same flags in
functions like LogRecoveryConflict, which isn't related to inter-process
signaling. It seems better to have a separate enum for the recovery
conflict reasons. With this patch, there's just a single
PROCSIG_RECOVERY_CONFLICT to wake up a process on a recovery conflict,
and the reason is communicated by setting a flag in a bitmask in PGPROC.
I was inspired to do this in preparation of my project to replaces
latches with "interrupts". By having just a single PROCSIG flag, we
reduce the need for "interrupt bits" with that project. But it seems
nicer on its own merits too.
0005: Refactor ProcessRecoveryConflictInterrupt for readability
---------------------------------------------------------------
The function had a switch-statement with fallthrough through all the
cases. It took me a while to understand how it works. Once I finally
understood it, I refactored it to not rely on the fallthrough. I hope
this makes it easier for others too.
- Heikki
