Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler?

Hi,

It'd be cool to commit and backpatch this - I'd like to re-enable the conflict
tests in the backbranches, and I don't think we want to do so with this issue
in place.

On 2022-05-10 16:39:11 +1200, Thomas Munro wrote:
> On Tue, Apr 12, 2022 at 10:50 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2022-04-12 10:33:28 +1200, Thomas Munro wrote:
> > > Instead of bothering to create N different XXXPending variables for
> > > the different conflict "reasons", I used an array. Other than that,
> > > it's much like existing examples.
> >
> > It kind of bothers me that each pending conflict has its own external function
> > call. It doesn't actually cost anything, because it's quite unlikely that
> > there's more than one pending conflict. Besides aesthetically displeasing,
> > it also leads to an unnecessarily large amount of code needed, because the
> > calls to RecoveryConflictInterrupt() can't be merged...
>
> 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?

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().

> > What might actually make more sense is to just have a bitmask or something?
>
> 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.

> /*
> @@ -3146,6 +3192,9 @@ ProcessInterrupts(void)
> return;
> InterruptPending = false;
>
> + if (RecoveryConflictPending)
> + ProcessRecoveryConflictInterrupts();
> +
> if (ProcDiePending)
> {
> ProcDiePending = false;

Should the ProcessRecoveryConflictInterrupts() call really be before the
ProcDiePending check?

Greetings,

Andres Freund