From 2da74146aafbd9d505e9e0d9038138bc46f0cd08 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Wed, 8 Apr 2026 13:53:08 +0900 Subject: [PATCH v3 1/4] Modified test suite from Evan's patch The C function in the original test module was using the same resource owner as the parent SELECT, so the crash could not be reproduced. Added a dedicated resource owner around the SPI call to ensure the inner resource owner is released before the outer trigger-firing batch callback fires, which is necessary to trigger the crash this test is meant to cover. --- src/test/modules/Makefile | 1 + src/test/modules/meson.build | 1 + src/test/modules/test_spi_resowner/Makefile | 23 ++++ .../expected/ri_fastpath.out | 118 ++++++++++++++++++ .../modules/test_spi_resowner/meson.build | 31 +++++ .../test_spi_resowner/sql/ri_fastpath.sql | 107 ++++++++++++++++ .../test_spi_resowner--1.0.sql | 9 ++ .../test_spi_resowner/test_spi_resowner.c | 70 +++++++++++ .../test_spi_resowner.control | 4 + 9 files changed, 364 insertions(+) create mode 100644 src/test/modules/test_spi_resowner/Makefile create mode 100644 src/test/modules/test_spi_resowner/expected/ri_fastpath.out create mode 100644 src/test/modules/test_spi_resowner/meson.build create mode 100644 src/test/modules/test_spi_resowner/sql/ri_fastpath.sql create mode 100644 src/test/modules/test_spi_resowner/test_spi_resowner--1.0.sql create mode 100644 src/test/modules/test_spi_resowner/test_spi_resowner.c create mode 100644 src/test/modules/test_spi_resowner/test_spi_resowner.control diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 0a74ab5c86f..016b328c8c5 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -52,6 +52,7 @@ SUBDIRS = \ test_shmem \ test_shm_mq \ test_slru \ + test_spi_resowner \ test_tidstore \ unsafe_tests \ worker_spi \ diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build index 4bca42bb370..3ca454064d0 100644 --- a/src/test/modules/meson.build +++ b/src/test/modules/meson.build @@ -53,6 +53,7 @@ subdir('test_saslprep') subdir('test_shmem') subdir('test_shm_mq') subdir('test_slru') +subdir('test_spi_resowner') subdir('test_tidstore') subdir('typcache') subdir('unsafe_tests') diff --git a/src/test/modules/test_spi_resowner/Makefile b/src/test/modules/test_spi_resowner/Makefile new file mode 100644 index 00000000000..5a69e3a3c42 --- /dev/null +++ b/src/test/modules/test_spi_resowner/Makefile @@ -0,0 +1,23 @@ +# src/test/modules/test_spi_resowner/Makefile + +MODULE_big = test_spi_resowner +OBJS = \ + $(WIN32RES) \ + test_spi_resowner.o +PGFILEDESC = "test_spi_resowner - SQL-callable C SPI function under a dedicated ResourceOwner" + +EXTENSION = test_spi_resowner +DATA = test_spi_resowner--1.0.sql + +REGRESS = ri_fastpath + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/test_spi_resowner +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/test_spi_resowner/expected/ri_fastpath.out b/src/test/modules/test_spi_resowner/expected/ri_fastpath.out new file mode 100644 index 00000000000..ad6b0f7c9b3 --- /dev/null +++ b/src/test/modules/test_spi_resowner/expected/ri_fastpath.out @@ -0,0 +1,118 @@ +-- +-- Test RI fast-path FK check under C-level SPI. +-- +-- The RI fast-path caches PK relation references in ri_FastPathGetEntry() +-- under the current resource owner. When FK triggers fire inside a +-- C-level SPI context that creates a dedicated short-lived resource owner, +-- those references must be released before the inner resource owner is +-- released. The fix ensures batch callbacks fire at the same firing depth +-- at which they were registered, while the corresponding resource owner +-- is still alive. Without this, ri_FastPathTeardown would crash with +-- Assert(rel->rd_refcnt > 0) in index_close. +-- +-- Simple PL/pgSQL does not trigger this because its SPI connection spans +-- the entire function call, so its resource owner outlives the batch +-- callback. The critical test case requires a C function that creates a +-- dedicated short-lived resource owner around its SPI call. +-- +CREATE EXTENSION test_spi_resowner; +CREATE TABLE ri_fp_pk1 (id serial PRIMARY KEY); +CREATE TABLE ri_fp_pk2 (id serial PRIMARY KEY); +CREATE TABLE ri_fp_pk3 (id serial PRIMARY KEY); +INSERT INTO ri_fp_pk1 VALUES (1); +INSERT INTO ri_fp_pk2 VALUES (1); +INSERT INTO ri_fp_pk3 VALUES (1); +CREATE TABLE ri_fp_fk ( + id serial PRIMARY KEY, + a int REFERENCES ri_fp_pk1(id), + b int REFERENCES ri_fp_pk2(id), + c int REFERENCES ri_fp_pk3(id), + d int REFERENCES ri_fp_pk1(id), + e int REFERENCES ri_fp_pk2(id), + f int REFERENCES ri_fp_pk3(id) +); +-- C-level SPI INSERT: the critical test case. +-- Without the fix this crashes the server. +SELECT spi_exec_sql( + 'INSERT INTO ri_fp_fk (a, b, c, d, e, f) VALUES (1, 1, 1, 1, 1, 1)'); + spi_exec_sql +-------------- + +(1 row) + +-- Additional C-level SPI INSERTs to exercise batch reuse across calls. +-- Use different column orderings to ensure each is a distinct statement. +SELECT spi_exec_sql( + 'INSERT INTO ri_fp_fk (f, e, d, c, b, a) VALUES (1, 1, 1, 1, 1, 1)'); + spi_exec_sql +-------------- + +(1 row) + +SELECT spi_exec_sql( + 'INSERT INTO ri_fp_fk (a, c, e, b, d, f) VALUES (1, 1, 1, 1, 1, 1)'); + spi_exec_sql +-------------- + +(1 row) + +-- C-level SPI with FK violation: should error, not crash +SELECT spi_exec_sql( + 'INSERT INTO ri_fp_fk (a, b, c, d, e, f) VALUES (999, 1, 1, 1, 1, 1)'); +ERROR: insert or update on table "ri_fp_fk" violates foreign key constraint "ri_fp_fk_a_fkey" +DETAIL: Key (a)=(999) is not present in table "ri_fp_pk1". +CONTEXT: SQL statement "INSERT INTO ri_fp_fk (a, b, c, d, e, f) VALUES (999, 1, 1, 1, 1, 1)" +-- Nested: PL/pgSQL calling C SPI (mimics PostGIS toTopoGeom pattern) +CREATE FUNCTION plpgsql_calls_c_spi() RETURNS void AS $$ +DECLARE + ins_stmt text := 'INSERT INTO ri_fp_fk (a, b, c, d, e, f) VALUES (1, 1, 1, 1, 1, 1)'; +BEGIN + PERFORM spi_exec_sql(ins_stmt); +END; +$$ LANGUAGE plpgsql; +SELECT plpgsql_calls_c_spi(); + plpgsql_calls_c_spi +--------------------- + +(1 row) + +-- AFTER trigger that uses C-level SPI to insert into an FK-referencing table. +-- The FK batch callback is registered at the inner SPI's firing depth and +-- must fire before the inner resource owner is released. This exercises +-- the depth-matched callback firing introduced to fix that crash. +CREATE TABLE ri_fp_outer (id int PRIMARY KEY); +CREATE TABLE ri_fp_inner (id int REFERENCES ri_fp_pk1(id)); +CREATE FUNCTION outer_trigger_spi_ok() RETURNS trigger AS $$ +BEGIN + PERFORM spi_exec_sql('INSERT INTO ri_fp_inner VALUES (1)'); + RETURN NEW; +END $$ LANGUAGE plpgsql; +CREATE TRIGGER outer_tg AFTER INSERT ON ri_fp_outer + FOR EACH ROW EXECUTE FUNCTION outer_trigger_spi_ok(); +-- Fires outer_tg, whose PL/pgSQL body calls spi_exec_sql(). The C function +-- creates a dedicated resource owner; the FK batch callback fires at the +-- inner SPI's firing depth before that resource owner is released. +INSERT INTO ri_fp_outer VALUES (1); +CREATE FUNCTION outer_trigger_spi_fail() RETURNS trigger AS $$ +BEGIN + PERFORM spi_exec_sql('INSERT INTO ri_fp_inner VALUES (3)'); + RETURN NEW; +END $$ LANGUAGE plpgsql; +DROP TRIGGER outer_tg ON ri_fp_outer; +DROP FUNCTION outer_trigger_spi_ok(); +CREATE TRIGGER outer_tg AFTER INSERT ON ri_fp_outer + FOR EACH ROW EXECUTE FUNCTION outer_trigger_spi_fail(); +-- Like above but the inner insert fails. +INSERT INTO ri_fp_outer VALUES (2); +ERROR: insert or update on table "ri_fp_inner" violates foreign key constraint "ri_fp_inner_id_fkey" +DETAIL: Key (id)=(3) is not present in table "ri_fp_pk1". +CONTEXT: SQL statement "INSERT INTO ri_fp_inner VALUES (3)" +SQL statement "SELECT spi_exec_sql('INSERT INTO ri_fp_inner VALUES (3)')" +PL/pgSQL function outer_trigger_spi_fail() line 3 at PERFORM +DROP TRIGGER outer_tg ON ri_fp_outer; +DROP FUNCTION outer_trigger_spi_fail(); +DROP TABLE ri_fp_inner, ri_fp_outer; +-- Cleanup +DROP TABLE ri_fp_fk; +DROP TABLE ri_fp_pk3, ri_fp_pk2, ri_fp_pk1; +DROP EXTENSION test_spi_resowner; diff --git a/src/test/modules/test_spi_resowner/meson.build b/src/test/modules/test_spi_resowner/meson.build new file mode 100644 index 00000000000..fbb027e05c7 --- /dev/null +++ b/src/test/modules/test_spi_resowner/meson.build @@ -0,0 +1,31 @@ +test_spi_resowner_sources = files( + 'test_spi_resowner.c', +) + +if host_system == 'windows' + test_spi_resowner_sources += rc_lib_gen.process(win32ver_rc, extra_args: [ + '--NAME', 'test_spi_resowner', + '--FILEDESC', 'test_spi_resowner - SQL-callable C SPI function under a dedicated ResourceOwner',]) +endif + +test_spi_resowner = shared_module('test_spi_resowner', + test_spi_resowner_sources, + kwargs: pg_test_mod_args, +) +test_install_libs += test_spi_resowner + +test_install_data += files( + 'test_spi_resowner.control', + 'test_spi_resowner--1.0.sql', +) + +tests += { + 'name': 'test_spi_resowner', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'regress': { + 'sql': [ + 'ri_fastpath', + ], + }, +} diff --git a/src/test/modules/test_spi_resowner/sql/ri_fastpath.sql b/src/test/modules/test_spi_resowner/sql/ri_fastpath.sql new file mode 100644 index 00000000000..4517b2437c4 --- /dev/null +++ b/src/test/modules/test_spi_resowner/sql/ri_fastpath.sql @@ -0,0 +1,107 @@ +-- +-- Test RI fast-path FK check under C-level SPI. +-- +-- The RI fast-path caches PK relation references in ri_FastPathGetEntry() +-- under the current resource owner. When FK triggers fire inside a +-- C-level SPI context that creates a dedicated short-lived resource owner, +-- those references must be released before the inner resource owner is +-- released. The fix ensures batch callbacks fire at the same firing depth +-- at which they were registered, while the corresponding resource owner +-- is still alive. Without this, ri_FastPathTeardown would crash with +-- Assert(rel->rd_refcnt > 0) in index_close. +-- +-- Simple PL/pgSQL does not trigger this because its SPI connection spans +-- the entire function call, so its resource owner outlives the batch +-- callback. The critical test case requires a C function that creates a +-- dedicated short-lived resource owner around its SPI call. +-- +CREATE EXTENSION test_spi_resowner; + +CREATE TABLE ri_fp_pk1 (id serial PRIMARY KEY); +CREATE TABLE ri_fp_pk2 (id serial PRIMARY KEY); +CREATE TABLE ri_fp_pk3 (id serial PRIMARY KEY); +INSERT INTO ri_fp_pk1 VALUES (1); +INSERT INTO ri_fp_pk2 VALUES (1); +INSERT INTO ri_fp_pk3 VALUES (1); + +CREATE TABLE ri_fp_fk ( + id serial PRIMARY KEY, + a int REFERENCES ri_fp_pk1(id), + b int REFERENCES ri_fp_pk2(id), + c int REFERENCES ri_fp_pk3(id), + d int REFERENCES ri_fp_pk1(id), + e int REFERENCES ri_fp_pk2(id), + f int REFERENCES ri_fp_pk3(id) +); + +-- C-level SPI INSERT: the critical test case. +-- Without the fix this crashes the server. +SELECT spi_exec_sql( + 'INSERT INTO ri_fp_fk (a, b, c, d, e, f) VALUES (1, 1, 1, 1, 1, 1)'); + +-- Additional C-level SPI INSERTs to exercise batch reuse across calls. +-- Use different column orderings to ensure each is a distinct statement. +SELECT spi_exec_sql( + 'INSERT INTO ri_fp_fk (f, e, d, c, b, a) VALUES (1, 1, 1, 1, 1, 1)'); +SELECT spi_exec_sql( + 'INSERT INTO ri_fp_fk (a, c, e, b, d, f) VALUES (1, 1, 1, 1, 1, 1)'); + +-- C-level SPI with FK violation: should error, not crash +SELECT spi_exec_sql( + 'INSERT INTO ri_fp_fk (a, b, c, d, e, f) VALUES (999, 1, 1, 1, 1, 1)'); + +-- Nested: PL/pgSQL calling C SPI (mimics PostGIS toTopoGeom pattern) +CREATE FUNCTION plpgsql_calls_c_spi() RETURNS void AS $$ +DECLARE + ins_stmt text := 'INSERT INTO ri_fp_fk (a, b, c, d, e, f) VALUES (1, 1, 1, 1, 1, 1)'; +BEGIN + PERFORM spi_exec_sql(ins_stmt); +END; +$$ LANGUAGE plpgsql; + +SELECT plpgsql_calls_c_spi(); + +-- AFTER trigger that uses C-level SPI to insert into an FK-referencing table. +-- The FK batch callback is registered at the inner SPI's firing depth and +-- must fire before the inner resource owner is released. This exercises +-- the depth-matched callback firing introduced to fix that crash. +CREATE TABLE ri_fp_outer (id int PRIMARY KEY); +CREATE TABLE ri_fp_inner (id int REFERENCES ri_fp_pk1(id)); + +CREATE FUNCTION outer_trigger_spi_ok() RETURNS trigger AS $$ +BEGIN + PERFORM spi_exec_sql('INSERT INTO ri_fp_inner VALUES (1)'); + RETURN NEW; +END $$ LANGUAGE plpgsql; + +CREATE TRIGGER outer_tg AFTER INSERT ON ri_fp_outer + FOR EACH ROW EXECUTE FUNCTION outer_trigger_spi_ok(); + +-- Fires outer_tg, whose PL/pgSQL body calls spi_exec_sql(). The C function +-- creates a dedicated resource owner; the FK batch callback fires at the +-- inner SPI's firing depth before that resource owner is released. +INSERT INTO ri_fp_outer VALUES (1); + +CREATE FUNCTION outer_trigger_spi_fail() RETURNS trigger AS $$ +BEGIN + PERFORM spi_exec_sql('INSERT INTO ri_fp_inner VALUES (3)'); + RETURN NEW; +END $$ LANGUAGE plpgsql; + +DROP TRIGGER outer_tg ON ri_fp_outer; +DROP FUNCTION outer_trigger_spi_ok(); + +CREATE TRIGGER outer_tg AFTER INSERT ON ri_fp_outer + FOR EACH ROW EXECUTE FUNCTION outer_trigger_spi_fail(); + +-- Like above but the inner insert fails. +INSERT INTO ri_fp_outer VALUES (2); + +DROP TRIGGER outer_tg ON ri_fp_outer; +DROP FUNCTION outer_trigger_spi_fail(); +DROP TABLE ri_fp_inner, ri_fp_outer; + +-- Cleanup +DROP TABLE ri_fp_fk; +DROP TABLE ri_fp_pk3, ri_fp_pk2, ri_fp_pk1; +DROP EXTENSION test_spi_resowner; diff --git a/src/test/modules/test_spi_resowner/test_spi_resowner--1.0.sql b/src/test/modules/test_spi_resowner/test_spi_resowner--1.0.sql new file mode 100644 index 00000000000..29ef70ee0dc --- /dev/null +++ b/src/test/modules/test_spi_resowner/test_spi_resowner--1.0.sql @@ -0,0 +1,9 @@ +/* src/test/modules/test_spi_resowner/test_spi_resowner--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION test_spi_resowner" to load this file. \quit + +CREATE FUNCTION spi_exec_sql(query text) +RETURNS void +AS 'MODULE_PATHNAME', 'spi_exec_sql' +LANGUAGE C STRICT; diff --git a/src/test/modules/test_spi_resowner/test_spi_resowner.c b/src/test/modules/test_spi_resowner/test_spi_resowner.c new file mode 100644 index 00000000000..0306139b5c0 --- /dev/null +++ b/src/test/modules/test_spi_resowner/test_spi_resowner.c @@ -0,0 +1,70 @@ +/*------------------------------------------------------------------------- + * + * test_spi_resowner.c + * SQL-callable C function that uses SPI to execute a query. + * + * Useful for testing code paths that only trigger under C-level + * SPI (not PL/pgSQL), such as resource owner interactions with + * RI fast-path FK checks. + * + * Copyright (c) 2026, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/test/modules/test_spi_resowner/test_spi_resowner.c + * + *------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "executor/spi.h" +#include "utils/builtins.h" + +PG_MODULE_MAGIC; + +PG_FUNCTION_INFO_V1(spi_exec_sql); + +/* + * spi_exec_sql(query text) - execute a SQL query via SPI. + * + * Opens a fresh SPI connection, executes the query, and closes the + * connection. Creates a dedicated child resource owner around the + * SPI_execute call and releases it before returning, ensuring that + * any resources registered under it (such as relation references + * opened by RI fast-path FK checks) are released before the outer + * trigger-firing batch callback fires. This reproduces the resource + * owner mismatch that occurs with C-language extensions like PostGIS + * topology functions, which cannot be triggered from PL/pgSQL since + * PL/pgSQL's SPI connection spans the entire function call. + */ +Datum +spi_exec_sql(PG_FUNCTION_ARGS) +{ + const char *query = text_to_cstring(PG_GETARG_TEXT_PP(0)); + int ret; + ResourceOwner save = CurrentResourceOwner; + ResourceOwner childowner = ResourceOwnerCreate(save, "test_spi inner"); + + SPI_connect(); + + CurrentResourceOwner = childowner; + ret = SPI_execute(query, false, 0); + + if (ret < 0) + elog(ERROR, "SPI_execute failed: error code %d", ret); + + SPI_finish(); + + CurrentResourceOwner = save; + ResourceOwnerRelease(childowner, + RESOURCE_RELEASE_BEFORE_LOCKS, + true, false); + ResourceOwnerRelease(childowner, + RESOURCE_RELEASE_LOCKS, + true, false); + ResourceOwnerRelease(childowner, + RESOURCE_RELEASE_AFTER_LOCKS, + true, false); + ResourceOwnerDelete(childowner); + + PG_RETURN_VOID(); +} diff --git a/src/test/modules/test_spi_resowner/test_spi_resowner.control b/src/test/modules/test_spi_resowner/test_spi_resowner.control new file mode 100644 index 00000000000..2120ae9442f --- /dev/null +++ b/src/test/modules/test_spi_resowner/test_spi_resowner.control @@ -0,0 +1,4 @@ +comment = 'Test SQL-callable C function that uses SPI using dedicated ResourceOwner' +default_version = '1.0' +module_pathname = '$libdir/test_spi_resowner' +relocatable = true -- 2.47.3