On Wed, Apr 8, 2026 at 6:58 PM Amit Langote <[email protected]> wrote: > On Wed, Apr 8, 2026 at 10:23 AM Amit Langote <[email protected]> wrote: > > On Tue, Apr 7, 2026 at 10:00 PM Evan Montgomery-Recht > > <[email protected]> wrote: > > > The patch also adds a test module (test_spi_func) with a C function > > > that executes SQL via SPI_connect/SPI_execute/SPI_finish, since this > > > crash cannot be triggered from PL/pgSQL. The test exercises the > > > C-level SPI INSERT with multiple FK constraints, FK violations, and > > > nested PL/pgSQL-calls-C-SPI (matching the PostGIS call pattern). > > I applied only the test module changes and it passes (without > crashing) even without your proposed fix. It seems that's because the > C function in test_spi_func calling SPI is using the same resource > owner as the parent SELECT. I think you'd need to create a resource > owner manually in the spi_exec() C function to reproduce the crash, as > done in the attached 0001, which contains the src/test changes > extracted from your patch modified as described, including renaming > the C function to spi_exec_sql(). > > Also, the test cases that call spi_exec() (_sql()) directly from a > SELECT don't actually exercise the crash path because there is no > outer trigger-firing loop active. query_depth is 0 inside the inner > SPI's AfterTriggerEndQuery, so the old guard wouldn't suppress the > callback there anyway. The critical case requires spi_exec_sql() to be > called from inside an AFTER trigger, where query_depth > 0 causes the > guard to defer the callback past the inner resource owner's lifetime. > I've added that test case. I kept your original test cases as they > still provide useful coverage of C-level SPI FK behavior even if they > don't exercise the crash path specifically. Maybe your original > PostGIS test suite that hit the crash did have the right structure, > but that's not reflected in the patch as far as I can tell. > > I've also renamed the module to test_spi_resowner to better reflect > what it's about. > > For the fix, I have a different proposal. As you observed, the > query_depth > 0 early return in FireAfterTriggerBatchCallbacks() means > that the nested SPI's callbacks get called under the outer resource > owner, which may not be the same as the one that SPI used. I think it > was a mistake to have that early return in the first place. Instead we > could remember for each callback what firing level it should be called > at, so the nested SPI's callbacks fire before returning to the parent > level and parent-level callbacks fire when the parent level completes. > I have implemented that in the attached 0002 along with transaction > boundary cleanup of callbacks, which passes the check-world for me, > but I'll need to stare some more at it before committing. > > Let me know if this also fixes your own in-house test suite or if you > have any other suggestions or if you think I am missing something.
One more cleanup patch attached as 0003: afterTriggerFiringDepth was added by commit 5c54c3ed1 as a file-static variable, which in hindsight should have been a field in AfterTriggersData alongside the other per-transaction after-trigger state. This patch makes that correction. One alternative design worth considering for 0002: storing batch_callbacks per query level in AfterTriggersQueryData rather than as a single list in AfterTriggersData, so callbacks naturally live at the query level where they were registered and get cleaned up with AfterTriggerFreeQuery on abort. Deferred constraints still need a top-level list in AfterTriggersData since they fire outside any query level. FireAfterTriggerBatchCallbacks() takes a list parameter and the caller passes either the query-level or top-level list as appropriate. This eliminates the need for firing_depth-matched firing entirely. I did that in 0004. I think I like it over 0002. Will look more closely tomorrow morning. -- Thanks, Amit Langote
v3-0004-Store-batch-callbacks-at-the-appropriate-level-ra.patch
Description: Binary data
v3-0002-Fix-RI-fast-path-crash-under-nested-C-level-SPI.patch
Description: Binary data
v3-0003-Move-afterTriggerFiringDepth-into-AfterTriggersDa.patch
Description: Binary data
v3-0001-Modified-test-suite-from-Evan-s-patch.patch
Description: Binary data
