Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.06%. Comparing base (b09f4be) to head (484a082).

@@            Coverage Diff             @@
##             main    #4924      +/-   ##
==========================================
+ Coverage   76.83%   77.06%   +0.23%     
==========================================
  Files         200      199       -1     
  Lines       54468    54293     -175     
==========================================
- Hits        41852    41843       -9     
+ Misses      12616    12450     -166     

@philippemnoel I added the doc comment on the stack invariant and a regression guard in verify_bm25_index.sql to ensure the normal multi-row expression index path stays intact.

I tried to write a dedicated test to trigger the nested executor panic but ran into tooling walls. Standard PL/pgSQL or SPI_exec don't actually trigger the nested ExecutorRun_hook path—the nesting seems specific to how bingo.cansmiles evaluates expressions via its internal C++ APIs.

I also tried writing a Rust-level FFI test to mock PREV_EXECUTOR_RUN_HOOK and explicitly verify the depth logic. That failed on two fronts: the cargo pgrx test schema generator silently strips out test functions that mock unsafe static C-pointers, and falling back to a standard cargo test fails linking because the Postgres backend symbols (like ExceptionalCondition) aren't available in standalone targets.

Since we can't reliably mock the FFI pointers in the test harness, and standard SQL won't nest the executor, the regression guard is the safest bet. @Pandaaaa906 has the exact bingo setup required to trigger this, so they are in the best position to validate the fix on their end.

That does not sound ideal. Without a real test, how can we guarantee that this feature won't regress?

@philippemnoel I completely agree it's not ideal. It is never comfortable flying blind on a regression.

To put the long-term risk into perspective: this bug specifically lives in fake_aminsertcleanup.rs, which is a polyfill exclusively for PG15–PG17. PG18+ uses the native aminsertcleanup API and is completely immune to this issue. Because of this, the code we are patching has a limited lifespan and will eventually be deprecated.

If we must have an automated guarantee for this PG15-17 shim, the only reliable path forward is to build a custom #[pg_extern] function (compiled only under #[cfg(test)]) that explicitly triggers a nested SPI_execute call. We would then have to wire that dummy function into an expression index in our regression suite to trick the Postgres C-backend into natively nesting the ExecutorRun hooks.

It is absolutely doable, but it requires wiring up some heavy, test-specific FFI scaffolding just to validate a legacy polyfill.

Given this only affects the pre-PG18 shim, do you feel that test scaffolding is worth the maintenance overhead? Or are we okay relying on the happy-path regression guard + the user's manual validation for this specific edge case? I am fully happy to build out the SPI test if you prefer the strict guarantee!

Done , went ahead and built it out. The #[pg_extern] + Spi::run() approach works without the FFI scaffolding overhead I was worried about.

@philippemnoel Added the regression test in the latest commit. A #[pg_extern] function spi_identity calls Spi::run("SELECT 1") internally. When used as a BM25 expression index, a multi-row INSERT evaluates it per row, firing ExecutorRun_hook from inside expression evaluation - the exact nesting path the bug described. No external extension needed.

Without the depth-counter fix this panics. With the fix: 185 passed, 0 failed on pg16.

@philippemnoel Retracting my previous comment , the spi_identity test was a false green. I verified locally that it passes even against the original broken code. Standard SPI does not re-enter ExecutorRun_hook; it goes through the SPI manager which bypasses the hook chain entirely. The nesting is specific to extensions like bingo that call into the executor directly via internal C++ APIs. I've removed the test from the branch.

To summarize everything I tried:

• Spi::run() / SPI_execute - does not re-enter the hook
• PL/pgSQL functions - same, routed through SPI
• Rust FFI mocking of PREV_EXECUTOR_RUN_HOOK - stripped by the pgrx schema generator; fails to link in standalone cargo test due to missing Postgres backend symbols
• Custom #[pg_extern] as an expression index - the approach I claimed worked, but confirmed to be a false green

The only real path to an automated test is a custom C extension that calls ExecutorRun() directly outside SPI. That's meaningful infrastructure for a polyfill that only exists for PG15–17 and gets deleted when PG17 is EOL.

What the branch does have instead: the doc comment on EXECUTOR_RUN_STACK now specifies the exact stack invariant, the before/after execution trace, and explicitly documents why no automated test exists. Any future change to the hook logic has to reconcile with it. The regression guarantee for this specific nesting path comes from @Pandaaaa906 validating against their bingo setup.

If you want the C extension route I'll build it, just say the word. Otherwise I'd argue the documented invariant + reporter validation is the right tradeoff for a legacy shim with a known deprecation timeline.

That sounds reasonable but then we would be a confirmation that a manual test was run and that this is correct

That sounds reasonable but then we would be a confirmation that a manual test was run and that this is correct

@philippemnoel Agreed. I've tagged @Pandaaaa906 directly on the issue asking them to validate the fix against their bingo setup. Once they confirm it resolves the panic on their end, that serves as the manual test confirmation.

That sounds reasonable but then we would be a confirmation that a manual test was run and that this is correct

@philippemnoel Agreed. I've tagged @Pandaaaa906 directly on the issue asking them to validate the fix against their bingo setup. Once they confirm it resolves the panic on their end, that serves as the manual test confirmation.

It would be preferrable if you could come up with a script that tests with bingo.cansmile, even if we don't commit it as a test.

@philippemnoel Makes total sense. As requested, I won't commit this to the repo to avoid polluting the CI environments that don't have bingo installed, but here is the exact validation script that reproduces the environment and verifies the fix:

-- Validation script for issue #4843
-- Requirements: ParadeDB (with pg_search), bingo extension, PostgreSQL 16
-- Run via: psql -d <db_name> -f bingo_validation.sql

CREATE EXTENSION IF NOT EXISTS bingo;

CREATE TABLE bingo_nested_exec_test (
    id      SERIAL PRIMARY KEY,
    smiles  TEXT NOT NULL
);

-- The bingo.cansmiles expression evaluation triggers the internal SPI call
-- which forces the nested ExecutorRun_hook
CREATE INDEX bingo_nested_exec_test_bm25_idx
    ON bingo_nested_exec_test
    USING bm25 (id, (bingo.cansmiles(smiles)::pdb.simple))
    WITH (key_field = 'id');

-- Expected Before Fix: PANIC "entered unreachable code: get_insert_state"
-- Expected After Fix: INSERT 0 5
INSERT INTO bingo_nested_exec_test (smiles)
SELECT repeat('C', n)
FROM generate_series(1, 5) AS n;

-- Expected: 5
SELECT COUNT(*) AS row_count FROM bingo_nested_exec_test;

DROP TABLE bingo_nested_exec_test;

I have tagged the issue reporter so they can run this exact snippet against their local bingo dataset to confirm the panic is resolved on their end!

@philippemnoel Makes total sense. As requested, I won't commit this to the repo to avoid polluting the CI environments that don't have bingo installed, but here is the exact validation script that reproduces the environment and verifies the fix:

-- Validation script for issue #4843
-- Requirements: ParadeDB (with pg_search), bingo extension, PostgreSQL 16
-- Run via: psql -d <db_name> -f bingo_validation.sql

CREATE EXTENSION IF NOT EXISTS bingo;

CREATE TABLE bingo_nested_exec_test (
    id      SERIAL PRIMARY KEY,
    smiles  TEXT NOT NULL
);

-- The bingo.cansmiles expression evaluation triggers the internal SPI call
-- which forces the nested ExecutorRun_hook
CREATE INDEX bingo_nested_exec_test_bm25_idx
    ON bingo_nested_exec_test
    USING bm25 (id, (bingo.cansmiles(smiles)::pdb.simple))
    WITH (key_field = 'id');

-- Expected Before Fix: PANIC "entered unreachable code: get_insert_state"
-- Expected After Fix: INSERT 0 5
INSERT INTO bingo_nested_exec_test (smiles)
SELECT repeat('C', n)
FROM generate_series(1, 5) AS n;

-- Expected: 5
SELECT COUNT(*) AS row_count FROM bingo_nested_exec_test;

DROP TABLE bingo_nested_exec_test;

I have tagged the issue reporter so they can run this exact snippet against their local bingo dataset to confirm the panic is resolved on their end!

That's a pretty small test. I think we should install Bingo in CI on PG16 and then have this as a test as I suggested initially.

That's a pretty small test. I think we should install Bingo in CI on PG16 and then have this as a test as I suggested initially.

@philippemnoel I can definitely wire this up, but I want to flag the infrastructure cost before I modify the workflow files.

The bingo extension isn't available via standard apt or pgdg repositories. To install it in CI, we would have to add a step to the GitHub Actions workflow that curls the pre-compiled binary zip directly from the EPAM website, extracts it, and executes their custom shell script to copy the .so files into the Postgres lib directory before pg_regress runs.

If you are comfortable adding a manually downloaded, third-party binary step to the CI pipeline just to test this PG15-17 polyfill, I will go ahead and build it!

Let me know if you are good with that repo-hygiene tradeoff, and I'll push the workflow changes.

That's a pretty small test. I think we should install Bingo in CI on PG16 and then have this as a test as I suggested initially.

@philippemnoel I can definitely wire this up, but I want to flag the infrastructure cost before I modify the workflow files.

The bingo extension isn't available via standard apt or pgdg repositories. To install it in CI, we would have to add a step to the GitHub Actions workflow that curls the pre-compiled binary zip directly from the EPAM website, extracts it, and executes their custom shell script to copy the .so files into the Postgres lib directory before pg_regress runs.

If you are comfortable adding a manually downloaded, third-party binary step to the CI pipeline just to test this PG15-17 polyfill, I will go ahead and build it!

Let me know if you are good with that repo-hygiene tradeoff, and I'll push the workflow changes.

That's probably just a few seconds, no? Let's only run it on PG16 or whatever is the last version that requires this fix.

That's probably just a few seconds, no? Let's only run it on PG16 or whatever is the last version that requires this fix.

@philippemnoel Sounds like a plan. I will scope the bingo installation and the regression script strictly to the PG16 matrix run so we don't bloat the other test environments. I'll push the workflow updates shortly!

@philippemnoel Hit an architecture blocker on the CI integration. Bingo only ships prebuilt Linux binaries for x86_64, there is no aarch64 build. I verified this against the official download page and confirmed the URL returns empty.

The problem: all PG16 system matrix runs use ubicloud-standard-8-arm-ubuntu-2404 (arm64), so the bingo .so can't be loaded there. The only amd64 runner currently in the matrix is the PG18 system run.

That said, I did validate the full fix end-to-end locally on x86_64 PG16 (system Postgres, not pgrx). Here's exactly what I ran:

1. Downloaded bingo 1.40.0 for PG16 linux x86_64 from the EPAM download server
2. Copied libbingo-postgres.so to /usr/lib/postgresql/16/lib/
3. Ran bingo-pg-install.sh -y -pglibdir to generate bingo_install.sql
4. Installed bingo schema via sudo -u postgres psql -f bingo_install.sql
5. Installed pg_search via cargo pgrx install against the system PG16
6. Ran tests/pg_regress/sql/bingo_nested_executor_fix.sql -> the script creates a BM25 expression index using bingo.cansmiles, inserts 5 rows in a single multi-row INSERT, and confirms all 5 land cleanly

Output:

CREATE TABLE
CREATE INDEX
INSERT 0 5
 row_count
-----------
         5
(1 row)
DROP TABLE

No panic. The fix is confirmed working against real bingo on real PG16 x86_64.

To run this in CI we'd need to add a dedicated PG16 amd64 matrix entry (same pattern as the existing PG18 amd64 include). That adds one extra runner to the PR matrix. Is that acceptable, or would you prefer to keep the regression script as a manual validation artifact and merge as-is?

@philippemnoel The PG16 amd64 job is failing with an AWS EC2 fleet capacity error - We couldn't find any instance pools that match your instance requirements. This is a transient spot instance availability issue, not a code problem. Could you re-run it from your end or confirm you're seeing the same? All other checks are passing.

@mdashti Good point on the re-entrant trigger case. You're right that if a BEFORE/AFTER trigger on a BM25-indexed table fires a DML that inserts into the same table, the inner aminsert would try to push an InsertState for the same indexrelid into the already-active map and hit Entry::Occupied. The depth counter doesn't help there. I'll look at whether this path is actually reachable in practice - triggers on BM25-indexed tables doing DML back into the same table is an unusual pattern, but it's not impossible. A RAII guard would handle both cases cleanly. Should I go ahead and implement it?

Done, removed bingo_nested_executor_fix.sql, reverted the log fallback step, and restored the original AMD comment. The diff is now focused on the executor fix only.

@mdashti The trace is correct and I can reproduce the logic failure on paper. The depth-counter approach does break the recursive trigger / SPI-INSERT-into-same-table path. I'll implement the RAII guard, it's the cleanest fix since it restores independent frames per nesting level without the writer-sharing implications of reusing Entry::Occupied. I'll also tighten the doc comment to reflect the narrower invariant. Give me some time to get this right.

@mdashti Implemented the RAII approach. Each ExecutorRun/ProcessUtility hook invocation now gets its own independent InsertFrame via FrameGuard::new(). Nested DML into the same table always pushes a fresh frame so inner aminsert calls target an empty HashMap - no Entry::Occupied collision. FrameGuard::drop handles cleanup automatically including on unwind, with a std::thread::panicking() guard to prevent double-panic. Also corrected the doc comment overstating the stack invariant. 255 tests pass locally on pg16.

Hi, @Nihal-Pandey-2302
I confirm your fix is working

Hi, @Nihal-Pandey-2302 I confirm your fix is working

@Pandaaaa906 Thanks for testing man!!