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

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?

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

> It's pretty weird that we have all this stuff that we then just check a short
> while later in ProcessInterrupts() whether they've been set.
>
> Seems like it'd make more sense to throw the error in
> ProcessRecoveryConflictInterrupt(), now that it's not in a a signal handler
> anymore?

Yeah. The thing that was putting me off doing that (and caused me to
get kinda stuck in the valley of indecision for a while here, sorry
about that) aside from trying to keep the diff small, was the need to
replicate this self-loathing code in a second place:

if (QueryCancelPending && QueryCancelHoldoffCount != 0)
{
/*
* Re-arm InterruptPending so that we process the cancel request as
* soon as we're done reading the message. (XXX this is seriously
* ugly: it complicates INTERRUPTS_CAN_BE_PROCESSED(), and it means we
* can't use that macro directly as the initial test in this function,
* meaning that this code also creates opportunities for other bugs to
* appear.)
*/

But I have now tried doing that anyway, and I hope the simplification
in other ways makes it worth it. Thoughts, objections?

> > /*
> > @@ -3147,6 +3148,22 @@ ProcessInterrupts(void)
> > return;
> > InterruptPending = false;
> >
> > + /*
> > + * Have we been asked to check for a recovery conflict? Processing these
> > + * interrupts may result in RecoveryConflictPending and related variables
> > + * being set, to be handled further down.
> > + */
> > + for (int i = PROCSIG_RECOVERY_CONFLICT_FIRST;
> > + i <= PROCSIG_RECOVERY_CONFLICT_LAST;
> > + ++i)
> > + {
> > + if (RecoveryConflictInterruptPending[i])
> > + {
> > + RecoveryConflictInterruptPending[i] = false;
> > + ProcessRecoveryConflictInterrupt(i);
> > + }
> > + }
>
> Hm. This seems like it shouldn't be in ProcessInterrupts(). How about checking
> calling a wrapper doing all this if RecoveryConflictPending?

I moved the loop into ProcessRecoveryConflictInterrupt() and added an
"s" to the latter's name. It already had the right indentation level
to contain a loop, once I realised that the test of
proc_exit_inprogress must be redundant.

Better?

[1] https://www.postgresql.org/message-id/flat/CA%2BhUKG%2B3MkS21yK4jL4cgZywdnnGKiBg0jatoV6kzaniBmcqbQ%40mail.gmail.com