On Sun, May 22, 2022 at 05:10:01PM -0700, Andres Freund wrote:
> On 2022-05-10 16:39:11 +1200, Thomas Munro wrote:
>> Ok, in this version there's two levels of flag:
>> RecoveryConflictPending, so we do nothing if that's not set, and then
>> the loop over RecoveryConflictPendingReasons is moved into
>> ProcessRecoveryConflictInterrupts(). Better?
>
> I think so.
>
> I don't particularly like the Handle/ProcessRecoveryConflictInterrupt() split,
> naming-wise. I don't think Handle vs Process indicates something meaningful?
> Maybe s/Interrupt/Signal/ for the signal handler one could help?
Handle is more consistent with the other types of interruptions in the
SIGUSR1 handler, so the name proposed in the patch in not that
confusing to me. And so does Process, in spirit with
ProcessProcSignalBarrier() and ProcessLogMemoryContextInterrupt().
While on it, is postgres.c the best home for
HandleRecoveryConflictInterrupt()? That's a very generic file, for
starters. Not related to the actual bug, just asking.
> It *might* look a tad cleaner to have the loop in a separate function from the
> existing code. I.e. a +ProcessRecoveryConflictInterrupts() that calls
> ProcessRecoveryConflictInterrupts().
Agreed that it would be a bit cleaner to keep the internals in a
different routine.
>> Yeah, in fact I'm exploring something like that in later bigger
>> redesign work[1] that gets rid of signal handlers. Here I'm looking
>> for something simple and potentially back-patchable and I don't want
>> to have to think about async signal safety of bit-level manipulations.
>
> Makes sense.
+1.
Also note that bufmgr.c mentions RecoveryConflictInterrupt() in the
top comment of HoldingBufferPinThatDelaysRecovery().
Should the processing of PROCSIG_RECOVERY_CONFLICT_DATABASE mention
that FATAL is used because we are never going to retry the conflict as
the database has been dropped? Getting rid of
RecoveryConflictRetryable makes the code easier to go through.
--
Michael