From c2367bfb3e2b6e3b592b2bda08d5825cf2b95bd8 Mon Sep 17 00:00:00 2001 From: Masahiko Sawada Date: Thu, 7 Aug 2025 23:37:31 -0700 Subject: [PATCH v2] Fix assertion failure in postgresGetForeignJoinPaths(). --- contrib/postgres_fdw/Makefile | 8 +++ .../postgres_fdw/expected/foreign_recheck.out | 27 ++++++++++ contrib/postgres_fdw/meson.build | 14 +++++- contrib/postgres_fdw/postgres_fdw.c | 16 ++++-- .../postgres_fdw/specs/foreign_recheck.spec | 50 +++++++++++++++++++ src/backend/access/heap/heapam_handler.c | 3 ++ 6 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 contrib/postgres_fdw/expected/foreign_recheck.out create mode 100644 contrib/postgres_fdw/specs/foreign_recheck.spec diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile index adfbd2ef758..fd1133f00dd 100644 --- a/contrib/postgres_fdw/Makefile +++ b/contrib/postgres_fdw/Makefile @@ -28,5 +28,13 @@ SHLIB_PREREQS = submake-libpq subdir = contrib/postgres_fdw top_builddir = ../.. include $(top_builddir)/src/Makefile.global + +# We have the sole isolation test case that requires injection points, +# so invoke the isolation test if injection points are enabled. +ifeq ($(enable_injection_points),yes) +EXTRA_INSTALL=src/test/modules/injection_points +ISOLATION = foreign_recheck +endif + include $(top_srcdir)/contrib/contrib-global.mk endif diff --git a/contrib/postgres_fdw/expected/foreign_recheck.out b/contrib/postgres_fdw/expected/foreign_recheck.out new file mode 100644 index 00000000000..bab3b61c0c2 --- /dev/null +++ b/contrib/postgres_fdw/expected/foreign_recheck.out @@ -0,0 +1,27 @@ +Parsed test spec with 2 sessions + +starting permutation: s0_lock s1_update s1_wakeup +injection_points_attach +----------------------- + +(1 row) + +step s0_lock: + SELECT + (SELECT 1 FROM sch1.b AS b, sch1.c AS c WHERE a.i = b.i AND b.i = c.i) + FROM sch1.a as a + FOR UPDATE; + +step s1_update: UPDATE sch1.a SET i = i + 100; +step s1_wakeup: SELECT injection_points_wakeup('heapam_lock_tuple-before-lock'); +injection_points_wakeup +----------------------- + +(1 row) + +step s0_lock: <... completed> +?column? +-------- + +(1 row) + diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.build index 5c11bc6496f..6dc40d7f1af 100644 --- a/contrib/postgres_fdw/meson.build +++ b/contrib/postgres_fdw/meson.build @@ -30,7 +30,7 @@ install_data( kwargs: contrib_data_args, ) -tests += { +postgres_fdw_test = { 'name': 'postgres_fdw', 'sd': meson.current_source_dir(), 'bd': meson.current_build_dir(), @@ -47,3 +47,15 @@ tests += { ], }, } + +if get_option('injection_points') + postgres_fdw_test += { + 'isolation': { + 'specs': [ + 'foreign_recheck', + ] + } + } +endif + +tests += postgres_fdw_test diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 456b267f70b..23c1b956048 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -6276,6 +6276,7 @@ postgresGetForeignJoinPaths(PlannerInfo *root, Cost total_cost; Path *epq_path; /* Path to create plan to be executed when * EvalPlanQual gets triggered. */ + bool need_epq = false; /* * Skip if this join combination has been considered already. @@ -6313,9 +6314,18 @@ postgresGetForeignJoinPaths(PlannerInfo *root, * calling foreign_join_ok(), since that function updates fpinfo and marks * it as pushable if the join is found to be pushable. */ - if (root->parse->commandType == CMD_DELETE || - root->parse->commandType == CMD_UPDATE || - root->rowMarks) + for (PlannerInfo *proot = root; proot != NULL; proot = proot->parent_root) + { + if (proot->parse->commandType == CMD_DELETE || + proot->parse->commandType == CMD_UPDATE || + proot->rowMarks) + { + need_epq = true; + break; + } + } + + if (need_epq) { epq_path = GetExistingLocalJoinPath(joinrel); if (!epq_path) diff --git a/contrib/postgres_fdw/specs/foreign_recheck.spec b/contrib/postgres_fdw/specs/foreign_recheck.spec new file mode 100644 index 00000000000..9cc48e6a48c --- /dev/null +++ b/contrib/postgres_fdw/specs/foreign_recheck.spec @@ -0,0 +1,50 @@ +setup +{ + CREATE EXTENSION injection_points; + CREATE EXTENSION postgres_fdw; + DO $d$ + BEGIN + EXECUTE $$CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw + OPTIONS (dbname '$$||current_database()||$$', + port '$$||current_setting('port')||$$' + )$$; + END; + $d$; + CREATE USER MAPPING FOR public SERVER loopback; + + CREATE SCHEMA sch1; + CREATE TABLE sch1.a (i int); + CREATE FOREIGN TABLE sch1.b (i int) SERVER loopback OPTIONS (schema_name 'sch2', table_name 'b'); + CREATE FOREIGN TABLE sch1.c (i int) SERVER loopback OPTIONS (schema_name 'sch2', table_name 'c'); + + CREATE SCHEMA sch2; + CREATE TABLE sch2.b (i int); + CREATE TABLE sch2.c (i int); + + INSERT INTO sch1.a VALUES (1); + INSERT INTO sch2.b VALUES (1); + INSERT INTO sch2.c VALUES (1); +} + +session "s0" +setup { + SELECT injection_points_set_local(); + SELECT injection_points_attach('heapam_lock_tuple-before-lock', 'wait'); +} +step "s0_lock" { + SELECT + (SELECT 1 FROM sch1.b AS b, sch1.c AS c WHERE a.i = b.i AND b.i = c.i) + FROM sch1.a as a + FOR UPDATE; +} + +session "s1" +step "s1_update" { UPDATE sch1.a SET i = i + 100; } +step "s1_wakeup" { SELECT injection_points_wakeup('heapam_lock_tuple-before-lock'); } + +# "s0_lock" execute a FOR UPDATE query but it stops before locking the result +# tuple because of the injection point. "s1_update" updates the same tuple +# concurrently and we wake up the session "s0", resulting that the FOR UPDATE +# query rechecks the tuple via EPQ. Verify that postgresRecheckForeignScan() +# function can check the tuple properly. +permutation "s0_lock" "s1_update" "s1_wakeup" diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index cb4bc35c93e..45b1cafc01d 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -44,6 +44,7 @@ #include "storage/procarray.h" #include "storage/smgr.h" #include "utils/builtins.h" +#include "utils/injection_point.h" #include "utils/rel.h" static void reform_and_rewrite_tuple(HeapTuple tuple, @@ -375,6 +376,8 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot, Assert(TTS_IS_BUFFERTUPLE(slot)); + INJECTION_POINT("heapam_lock_tuple-before-lock", NULL); + tuple_lock_retry: tuple->t_self = *tid; result = heap_lock_tuple(relation, tuple, cid, mode, wait_policy, -- 2.39.5 (Apple Git-154)