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

From: Andres Freund <andres(at)anarazel(dot)de>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
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-23 00:10:01
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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

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


Andres Freund

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2022-05-23 01:32:40 Re: Add --{no-,}bypassrls flags to createuser
Previous Message Michael Paquier 2022-05-22 23:53:24 Re: docs: mention "pg_read_all_stats" in "track_activities" description