Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler?
Thomas Munro <thomas.munro@gmail.com>
From: Thomas Munro <thomas.munro@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>, Noah Misch <noah@leadboat.com>, Robert Haas <robertmhaas@gmail.com>, Michael Paquier <michael@paquier.xyz>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2023-01-14T02:23:11Z
Lists: pgsql-hackers
Attachments
- v5-0001-Use-MemoryContext-API-for-regex-memory-management.patch (text/x-patch) patch v5-0001
- v5-0002-Update-contrib-trgm_regexp-s-memory-management.patch (text/x-patch) patch v5-0002
- v5-0003-Redesign-interrupt-cancel-API-for-regex-engine.patch (text/x-patch) patch v5-0003
- v5-0004-Fix-recovery-conflict-SIGUSR1-handling.patch (text/x-patch) patch v5-0004
On Thu, Jan 5, 2023 at 2:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > The rcancelrequested API is something that I devised out of whole cloth > awhile ago. It's not in Tcl's copy of the code, which AFAIK is the > only other project using this regex engine. I do still have vague > hopes of someday seeing the engine as a standalone project, which is > why I'd prefer to keep a bright line between the engine and Postgres. > But there's no very strong reason to think that any hypothetical future > external users who need a cancel API would want this API as opposed to > one that requires exit() or longjmp() to get out of the engine. So if > we're changing the way we use it, I think it's perfectly reasonable to > redesign that API to make it simpler and less of an impedance mismatch. Thanks for that background. Alright then, here's a new iteration exploring this direction. It gets rid of CANCEL_REQUESTED() -> REG_CANCEL and the associated error and callback function, and instead has just "INTERRUPT(re);" at those cancellation points, which is a macro that defaults to nothing (for Tcl's benefit). Our regcustom.h defines it as CHECK_FOR_INTERRUPTS(). I dunno if it's worth passing the "re" argument... I was imagining that someone who wants to free memory explicitly and then longjmp would probably need it? (It might even be possible to expand to something that sets an error and returns, not investigated.) Better name or design very welcome. Another decision is to use the no-OOM version of palloc. (Not explored: could we use throwing palloc with attribute returns_nonnull to teach GCC and Clang to prune the failure handling from generated regex code?) (As for STACK_TOO_DEEP(): why follow a function pointer, when it could be macro-only too? But that's getting off track.) I split the patch in two: memory and interrupts. I also found a place in contrib/pg_trgm that did no-longer-needed try/finally.
Commits
-
Fix recovery conflict SIGUSR1 handling.
- 0da096d78e1e 17.0 landed
-
Redesign interrupt/cancel API for regex engine.
- db4f21e4a34b 16.0 landed
-
Update contrib/trgm_regexp's memory management.
- 6db75edb2ecb 16.0 landed
-
Update tsearch regex memory management.
- 4f51429dd7f1 16.0 landed
-
Use MemoryContext API for regex memory management.
- bea3d7e3831f 16.0 landed