Skip to content

Commit 0da096d

Browse files
macdicemacdice
committed
Fix recovery conflict SIGUSR1 handling.
We shouldn't be doing non-trivial work in signal handlers in general, and in this case the handler could reach unsafe code and corrupt state. It also clobbered its own "reason" code. Move all recovery conflict decision logic into the next CHECK_FOR_INTERRUPTS(), and have the signal handler just set flags and the latch, following the standard pattern. Since there are several different "reasons", use a separate flag for each. With this refactoring, the recovery conflict system no longer piggy-backs on top of the regular query cancelation mechanism, but instead raises an error directly if it decides that is necessary. It still needs to respect QueryCancelHoldoffCount, because otherwise the FEBE protocol might get out of sync (see commit 2b3a8b2). This fixes one class of intermittent failure in the new 031_recovery_conflict.pl test added by commit 9f8a050, though the buggy coding is much older. Failures outside contrived testing seem to be very rare (or perhaps incorrectly attributed) in the field, based on lack of reports. No back-patch for now due to complexity and release schedule. We have the option to back-patch into 16 later, as 16 has prerequisite commit bea3d7e. Reviewed-by: Andres Freund <andres@anarazel.de> (earlier version) Reviewed-by: Michael Paquier <michael@paquier.xyz> (earlier version) Reviewed-by: Robert Haas <robertmhaas@gmail.com> (earlier version) Tested-by: Christoph Berg <myon@debian.org> Discussion: https://postgr.es/m/CA%2BhUKGK3PGKwcKqzoosamn36YW-fsuTdOPPF1i_rtEO%3DnEYKSg%40mail.gmail.com Discussion: https://postgr.es/m/CALj2ACVr8au2J_9D88UfRCi0JdWhyQDDxAcSVav0B0irx9nXEg%40mail.gmail.com
1 parent 8c16ad3 commit 0da096d

File tree

5 files changed

+186
-170
lines changed

5 files changed

+186
-170
lines changed

‎src/backend/storage/buffer/bufmgr.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -4923,8 +4923,8 @@ LockBufferForCleanup(Buffer buffer)
49234923
}
49244924

49254925
/*
4926-
* Check called from RecoveryConflictInterrupt handler when Startup
4927-
* process requests cancellation of all pin holders that are blocking it.
4926+
* Check called from ProcessRecoveryConflictInterrupts() when Startup process
4927+
* requests cancellation of all pin holders that are blocking it.
49284928
*/
49294929
bool
49304930
HoldingBufferPinThatDelaysRecovery(void)

‎src/backend/storage/ipc/procsignal.c

+7-7
Original file line numberDiff line numberDiff line change
@@ -662,25 +662,25 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
662662
HandleParallelApplyMessageInterrupt();
663663

664664
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_DATABASE))
665-
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_DATABASE);
665+
HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_DATABASE);
666666

667667
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_TABLESPACE))
668-
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_TABLESPACE);
668+
HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_TABLESPACE);
669669

670670
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_LOCK))
671-
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_LOCK);
671+
HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_LOCK);
672672

673673
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_SNAPSHOT))
674-
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_SNAPSHOT);
674+
HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_SNAPSHOT);
675675

676676
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_LOGICALSLOT))
677-
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_LOGICALSLOT);
677+
HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_LOGICALSLOT);
678678

679679
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK))
680-
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK);
680+
HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK);
681681

682682
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
683-
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
683+
HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
684684

685685
SetLatch(MyLatch);
686686

0 commit comments

Comments
 (0)