Re: Eliminating SPI / SQL from some RI triggers - take 3

Hi Amit,

First time contributing to this project, let me know if I missed
something or need to adjust what I put together.

I found a crash in the RI fast-path FK check code introduced by
2da86c1ef9b and extended by b7b27eb41a5. C-language extensions that
use SPI to INSERT into tables with multiple FK constraints hit an
assertion failure (or, without assertions, a server crash) when the
batch callback fires. I discovered this via PostGIS's topology CI --
toTopoGeom() uses SPI to insert into edge_data, which has 4 immediate
FK constraints referencing node and face. PG 18 passes the same test;
the master crashes.

This appears to be a separate issue from Chao Li's deferred-trigger
batching bug; the patches touch different files and don't conflict. I
did do a regression test on the merge referenced both PostgreSQL and
PostGIS (to validate that this works.)

The problem: ri_FastPathGetEntry() opens pk_rel/idx_rel and creates
TupleTableSlots, registering them with the current resource owner --
the SPI portal's. The batch callback ri_FastPathTeardown() only fires
when query_depth == 0 (via FireAfterTriggerBatchCallbacks), but by
that time the inner portal has finished and its resource owner has
released the relation references and TupleDesc pins. The teardown then
tries to close relations whose refcounts are already zero:

TRAP: failed Assert("rel->rd_refcnt > 0"), File: "relcache.c"

RelationDecrementReferenceCount
-> RelationClose -> index_close -> ri_FastPathTeardown
-> ri_FastPathEndBatch -> FireAfterTriggerBatchCallbacks
-> AfterTriggerEndQuery -> standard_ExecutorFinish

and TupleDesc pins that are no longer tracked by any resource owner
cause "tupdesc reference not owned by resource owner" errors.

Note that simple PL/pgSQL functions don't trigger this because
PL/pgSQL's SPI connection spans the entire function call, so the
portal's resource owner outlives the batch callback. The crash
requires nested SPI from a C extension, which creates a shorter-lived
portal.

The attached patch (against master, for application) fixes this by
transferring both relation references and TupleDesc pins from the
current resource owner to CurTransactionResourceOwner immediately
after creating them in ri_FastPathGetEntry(). The transfer uses
RelationIncrementReferenceCount / PinTupleDesc under the target owner
followed by RelationDecrementReferenceCount / ReleaseTupleDesc under
the original. I chose this over switching CurrentResourceOwner around
the table_open/index_open calls because the latter also affects
transient buffer pins acquired during catalog lookups inside those
functions.

ri_FastPathTeardown is updated to clear any buffered tuples (whose
buffer pins belong to the current resource owner) before switching to
CurTransactionResourceOwner for the close/drop operations.

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).

This is purely a correctness fix with no performance or backward
compatibility impact. No documentation changes are needed since this
is an internal bug fix.

The patch compiles cleanly and passes pgindent, clang-tidy, and
cppcheck. All 247 regression subtests pass, along with the full meson
test suite (370 ok, 0 fail, 21 skipped) (skipped due to hardware
availability on my side this week). I also verified the PostGIS
topology test (toTopoGeom) passes clean with no warnings, and tested
abort paths (FK violation, transaction rollback, subtransaction abort
via EXCEPTION blocks) (not in scope of PostgreSQL but more for my own
verification that things work). Code coverage on the new lines is
100%. Tested on macOS (aarch64) and Linux (aarch64, via Docker).

Unrelated to my patch, SonarCloud flagged a potential issue in
recheck_matched_pk_tuple() (line 3370): the function loops over
ii_NumIndexKeyAttrs elements of the skeys array, but the caller in
ri_FastPathFlushArray passes recheck_skey[1] -- an array of exactly
one element. This is safe because ri_FastPathFlushArray is the

single-column FK path, so ii_NumIndexKeyAttrs is always 1 there.
However, the function signature doesn't communicate this constraint,
which flags as CWE-125 (out-of-bounds read) / CERT C ARR30-C. Adding
an nkeys parameter (like ri_FastPathProbeOne already has) would make
the contract explicit.

Unrelated to PostgreSQL directly, I currently have a workaround for
the changes I'm making to PostGIS to test out some performance
enhancements. I left comments in the code so that if this gets
accepted, I can revert to a cleaner approach, as this appears to only
affect pg19 based on the testing I've done so far.

If there's a cleaner approach or a larger underlying issue, I'm
definitely willing to keep testing to find a better solution.

--
thanks, Evan Montgomery-Recht

On Mon, Apr 6, 2026 at 10:12 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Tue, Apr 7, 2026 at 10:46 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> > > On Apr 6, 2026, at 17:45, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > On Fri, Apr 3, 2026 at 6:39 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > >> On Fri, Apr 3, 2026 at 5:58 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> > >>> I spent several hours debugging this patch today, and I found a problem where the batch mode doesn't seem to handle deferred RI triggers, although the commit message suggests that it should.
> > >>>
> > >>> I traced this scenario:
> > >>> ```
> > >>> CREATE TABLE pk (a int primary key);
> > >>> CREATE TABLE fk (a int references pk(a) DEFERRABLE INITIALLY DEFERRED);
> > >>> BEGIN;
> > >>> INSERT INTO fk VALUES (1);
> > >>> INSERT INTO pk VALUES (1);
> > >>> COMMIT;
> > >>> ```
> > >>>
> > >>> When COMMIT is executed, it reaches RI_FKey_check(), where AfterTriggerIsActive() checks whether afterTriggers.query_depth >= 0. But in the deferred case, afterTriggers.query_depth is -1.
> > >>>
> > >>> From the code:
> > >>> ```
> > >>> if (ri_fastpath_is_applicable(riinfo))
> > >>> {
> > >>> if (AfterTriggerIsActive())
> > >>> {
> > >>> /* Batched path: buffer and probe in groups */
> > >>> ri_FastPathBatchAdd(riinfo, fk_rel, newslot);
> > >>> }
> > >>> else
> > >>> {
> > >>> /* ALTER TABLE validation: per-row, no cache */
> > >>> ri_FastPathCheck(riinfo, fk_rel, newslot);
> > >>> }
> > >>> return PointerGetDatum(NULL);
> > >>> }
> > >>> ```
> > >>>
> > >>> So this ends up falling back to the per-row path for deferred RI checks at COMMIT, even though the intent here seems to be only to bypass the ALTER TABLE validation case, where batch callbacks would never fire, and MyTriggerDepth is 0. So, maybe we can just check MyTriggerDepth>0 in AfterTriggerIsActive().
> > >>>
> > >>> I tried the attached fix. With it, deferred triggers go through the batch mode, and all existing tests still pass.
> > >>
> > >> I think you might be right. Thanks for the patch. It looks correct
> > >> to me at a glance, but I will need to check it a bit more closely
> > >> before committing.
> > >
> > > Thinking about this some more, your fix is on the right track but
> > > needs a bit more work -- MyTriggerDepth > 0 is too broad since it
> > > fires for BEFORE triggers too. I have a revised version using a new
> > > afterTriggerFiringDepth counter that I'll push shortly.
> > >
> > > Added an open item for tracking in the meantime:
> > > https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items#Open_Issues
> >
> > V2 looks good to me. Besides the normal cases, I also traced an abnormal case to verify that afterTriggerFiringDepth is always reset to 0:
> > ```
> > evantest=# begin;
> > BEGIN
> > evantest=*# INSERT INTO fk VALUES (2);
> > INSERT 0 1
> > evantest=*# commit;
> > ERROR: insert or update on table "fk" violates foreign key constraint "fk_a_fkey"
> > DETAIL: Key (a)=(2) is not present in table "pk".
> > ```
>
> Thanks for checking. Pushed.
>
> --
> Thanks, Amit Langote
>
>