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

On Tue, Apr 12, 2022 at 10:50 AM Andres Freund <andres@anarazel.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