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

On Thu, Dec 29, 2022 at 9:40 PM Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Tue, Jul 26, 2022 at 07:22:52PM -0400, Tom Lane wrote:
> > Thomas Munro <thomas(dot)munro(at)gmail(dot)com> writes:
> > > ... The regular expression machinery is capable of
> > > consuming a lot of CPU, and does CANCEL_REQUESTED(nfa->v->re)
> > > frequently to avoid getting stuck. With the patch as it stands, that
> > > would never be true.
> >
> > Surely that can't be too hard to fix. We might have to refactor
> > the code around QueryCancelPending a little bit so that callers
> > can ask "do we need a query cancel now?" without actually triggering
> > a longjmp ... but why would that be problematic?
>
> It could work. The problems are like those of making code safe to run in a
> signal handler. You can use e.g. snprintf in rcancelrequested(), but you
> still can't use palloc() or ereport(). I see at least these strategies:
>
> 1. Accept that recovery conflict checks run after a regex call completes.
> 2. Have rcancelrequested() return true unconditionally if we need a conflict
> check. If there's no actual conflict, restart the regex.
> 3. Have rcancelrequested() run the conflict check, including elog-using
> PostgreSQL code. On longjmp(), accept the leak of regex mallocs.
> 4. Have rcancelrequested() run the conflict check, including elog-using
> PostgreSQL code. On longjmp(), escalate to FATAL.
> 5. Write the conflict check code to dutifully avoid longjmp().
> 6. Convert src/backend/regex to use palloc, so longjmp() is fine.

Thanks! I appreciate the help getting unstuck here. I'd thought
about some of these but not all. I also considered a couple more:

7. Do a CFI() in a try/catch if INTERRUPTS_PENDING_CONDITION() is
true, and copy the error somewhere to be re-thrown later after the
regexp code exits with REG_CANCEL.
8. Do a CFI() in a try/catch if INTERRUPTS_PENDING_CONDITION() is
true, and call a new regexp function that will free everything before
re-throwing.

After Tom's response I spent some time trying to figure out how to
make a SOFT_CHECK_FOR_INTERRUPTS(), which would return a value to
indicate that it would like to throw. I think it would need to re-arm
various flags and introduce a programming rule for all interrupt
processing routines that if they fired once under a soft check they
must fire again later under a non-soft check. That all seems a bit
complicated, and a general mechanism like that seemed like overkill
for a single user, which led me to idea #7.

Idea #8 is a realisation that twisting oneself into a pretzel to avoid
having to change the regexp code or its REG_CANCEL control flow may be
a bit silly. If the only thing it really needs to do is free some
memory, maybe the regexp module should provide a function that frees
everything that is safe to call from our rcancelrequested callback, so
we can do so before we longjmp back to Kansas. Then the REG_CANCEL
code paths would be effectively unreachable in PostgreSQL. I don't
know if it's better or worse than your idea #6, "use palloc instead,
it already has garbage collection, duh", but it's a different take on
the same realisation that this is just about free().

I guess idea #6 must be pretty easy to try: just point that MALLOC()
macro to palloc(), and do a plain old CFI() in rcancelrequested().
Why do you suggest #3 as an interim measure? Do we fear that palloc()
might hurt regexp performance?

> I can reproduce that symptom reliably, on GNU/Linux, with the attached patch
> adding sleeps. The key log bit:
>
> 2022-09-16 11:50:37.338 CEST [15022:4] 031_recovery_conflict.pl LOG: statement: BEGIN;
> 2022-09-16 11:50:37.339 CEST [15022:5] 031_recovery_conflict.pl LOG: statement: LOCK TABLE test_recovery_conflict_table1 IN ACCESS SHARE MODE;
> 2022-09-16 11:50:37.341 CEST [15022:6] 031_recovery_conflict.pl LOG: statement: SELECT 1;
> 2022-09-16 11:50:38.076 CEST [14880:17] LOG: recovery still waiting after 11.482 ms: recovery conflict on lock
> 2022-09-16 11:50:38.076 CEST [14880:18] DETAIL: Conflicting process: 15022.
> 2022-09-16 11:50:38.076 CEST [14880:19] CONTEXT: WAL redo at 0/34243F0 for Standby/LOCK: xid 733 db 16385 rel 16386
> 2022-09-16 11:50:38.196 CEST [15022:7] 031_recovery_conflict.pl FATAL: terminating connection due to conflict with recovery
> 2022-09-16 11:50:38.196 CEST [15022:8] 031_recovery_conflict.pl DETAIL: User transaction caused buffer deadlock with recovery.
> 2022-09-16 11:50:38.196 CEST [15022:9] 031_recovery_conflict.pl HINT: In a moment you should be able to reconnect to the database and repeat your command.
> 2022-09-16 11:50:38.197 CEST [15022:10] 031_recovery_conflict.pl LOG: disconnection: session time: 0:00:01.041 user=nm database=test_db host=[local]
> 2022-09-16 11:50:38.198 CEST [14880:20] LOG: recovery finished waiting after 132.886 ms: recovery conflict on lock
>
> The second DETAIL should be "User was holding a relation lock for too long."
> The backend in question is idle in transaction. RecoveryConflictInterrupt()
> for PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK won't see IsWaitingForLock(),
> so it will find no conflict. However, RecoveryConflictReason remains
> clobbered, hence the wrong DETAIL message.

Aha. I'd speculated that RecoveryConflictReason must be capable of
reporting bogus errors like that up-thread.

> Incidentally, the affected test
> contains comment "# DROP TABLE containing block which standby has in a pinned
> buffer". The standby holds no pin at that moment; the LOCK TABLE pins system
> catalog pages, but it drops every pin it acquires.

Oh, I guess the comment is just wrong? There are earlier sections
concerned with buffer pins, but the section "RECOVERY CONFLICT 3" is
about locks.