From: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler? |
Date: | 2022-05-10 04:39:11 |
Message-ID: | CA+hUKGL5FLBP5rV-axSZ-0Y5aZNe9aS-JDW8pGZctvkeXM2-ww@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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?
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Fix-recovery-conflict-SIGUSR1-handling.patch | text/x-patch | 14.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Masahiko Sawada | 2022-05-10 05:04:42 | Re: Perform streaming logical transactions by background workers and parallel apply |
Previous Message | Perumal Raj | 2022-05-10 04:35:28 | pglogical setup in cascade replication architecture |