Skip to content

Commit 12609fb

Browse files
Etsuro FujitaEtsuro Fujita
Etsuro Fujita
and
Etsuro Fujita
committed
Fix EvalPlanQual handling of foreign/custom joins in ExecScanFetch.
If inside an EPQ recheck, ExecScanFetch would run the recheck method function for foreign/custom joins even if they aren't descendant nodes in the EPQ recheck plan tree, which is problematic at least in the foreign-join case, because such a foreign join isn't guaranteed to have an alternative local-join plan required for running the recheck method function; in the postgres_fdw case this could lead to a segmentation fault or an assert failure in an assert-enabled build when running the recheck method function. Even if inside an EPQ recheck, any scan nodes that aren't descendant ones in the EPQ recheck plan tree should be normally processed by using the access method function; fix by modifying ExecScanFetch so that if inside an EPQ recheck, it runs the recheck method function for foreign/custom joins that are descendant nodes in the EPQ recheck plan tree as before and runs the access method function for foreign/custom joins that aren't. This fix also adds to postgres_fdw an isolation test for an EPQ recheck that caused issues stated above. Oversight in commit 385f337. Reported-by: Kristian Lejao <kristianlejao@gmail.com> Author: Masahiko Sawada <sawada.mshk@gmail.com> Co-authored-by: Etsuro Fujita <etsuro.fujita@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Etsuro Fujita <etsuro.fujita@gmail.com> Discussion: https://postgr.es/m/CAD21AoBpo6Gx55FBOW+9s5X=nUw3Xpq64v35fpDEKsTERnc4TQ@mail.gmail.com Backpatch-through: 13
1 parent 29dc7a6 commit 12609fb

File tree

6 files changed

+117
-7
lines changed

6 files changed

+117
-7
lines changed

‎contrib/postgres_fdw/.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
# Generated subdirectories
22
/log/
33
/results/
4+
/output_iso/
45
/tmp_check/
6+
/tmp_check_iso/

‎contrib/postgres_fdw/Makefile‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ EXTENSION = postgres_fdw
1717
DATA = postgres_fdw--1.0.sql postgres_fdw--1.0--1.1.sql postgres_fdw--1.1--1.2.sql
1818

1919
REGRESS = postgres_fdw query_cancel
20+
ISOLATION = eval_plan_qual
21+
ISOLATION_OPTS = --load-extension=postgres_fdw
2022
TAP_TESTS = 1
2123

2224
ifdef USE_PGXS

‎contrib/postgres_fdw/expected/eval_plan_qual.out‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
Parsed test spec with 2 sessions
2+
3+
starting permutation: s0_begin s0_update s1_begin s1_tuplock s0_commit s1_commit
4+
step s0_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
5+
step s0_update: UPDATE a SET i = i + 1;
6+
step s1_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
7+
step s1_tuplock:
8+
-- Verify if the sub-select has a foreign-join plan
9+
EXPLAIN (VERBOSE, COSTS OFF)
10+
SELECT a.i,
11+
(SELECT 1 FROM fb, fc WHERE a.i = fb.i AND fb.i = fc.i)
12+
FROM a FOR UPDATE;
13+
SELECT a.i,
14+
(SELECT 1 FROM fb, fc WHERE a.i = fb.i AND fb.i = fc.i)
15+
FROM a FOR UPDATE;
16+
<waiting ...>
17+
step s0_commit: COMMIT;
18+
step s1_tuplock: <... completed>
19+
QUERY PLAN
20+
----------------------------------------------------------------------------------------------------------------------------------------
21+
LockRows
22+
Output: a.i, ((SubPlan expr_1)), a.ctid
23+
-> Seq Scan on public.a
24+
Output: a.i, (SubPlan expr_1), a.ctid
25+
SubPlan expr_1
26+
-> Foreign Scan
27+
Output: 1
28+
Relations: (public.fb) INNER JOIN (public.fc)
29+
Remote SQL: SELECT NULL FROM (public.b r1 INNER JOIN public.c r2 ON (((r2.i = $1::integer)) AND ((r1.i = $1::integer))))
30+
(9 rows)
31+
32+
i|?column?
33+
-+--------
34+
2|
35+
(1 row)
36+
37+
step s1_commit: COMMIT;

‎contrib/postgres_fdw/meson.build‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,12 @@ tests += {
4141
],
4242
'regress_args': ['--dlpath', meson.project_build_root() / 'src/test/regress'],
4343
},
44+
'isolation': {
45+
'specs': [
46+
'eval_plan_qual',
47+
],
48+
'regress_args': ['--load-extension=postgres_fdw'],
49+
},
4450
'tap': {
4551
'tests': [
4652
't/001_auth_scram.pl',

‎contrib/postgres_fdw/specs/eval_plan_qual.spec‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
# Tests for the EvalPlanQual mechanism involving foreign tables
2+
3+
setup
4+
{
5+
DO $d$
6+
BEGIN
7+
EXECUTE $$CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
8+
OPTIONS (dbname '$$||current_database()||$$',
9+
port '$$||current_setting('port')||$$'
10+
)$$;
11+
END;
12+
$d$;
13+
CREATE USER MAPPING FOR PUBLIC SERVER loopback;
14+
15+
CREATE TABLE a (i int);
16+
CREATE TABLE b (i int);
17+
CREATE TABLE c (i int);
18+
CREATE FOREIGN TABLE fb (i int) SERVER loopback OPTIONS (table_name 'b');
19+
CREATE FOREIGN TABLE fc (i int) SERVER loopback OPTIONS (table_name 'c');
20+
21+
INSERT INTO a VALUES (1);
22+
INSERT INTO b VALUES (1);
23+
INSERT INTO c VALUES (1);
24+
}
25+
26+
teardown
27+
{
28+
DROP TABLE a;
29+
DROP TABLE b;
30+
DROP TABLE c;
31+
DROP SERVER loopback CASCADE;
32+
}
33+
34+
session s0
35+
step s0_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
36+
step s0_update { UPDATE a SET i = i + 1; }
37+
step s0_commit { COMMIT; }
38+
39+
session s1
40+
step s1_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
41+
step s1_tuplock {
42+
-- Verify if the sub-select has a foreign-join plan
43+
EXPLAIN (VERBOSE, COSTS OFF)
44+
SELECT a.i,
45+
(SELECT 1 FROM fb, fc WHERE a.i = fb.i AND fb.i = fc.i)
46+
FROM a FOR UPDATE;
47+
SELECT a.i,
48+
(SELECT 1 FROM fb, fc WHERE a.i = fb.i AND fb.i = fc.i)
49+
FROM a FOR UPDATE;
50+
}
51+
step s1_commit { COMMIT; }
52+
53+
# This test exercises EvalPlanQual with a SubLink sub-select (which should
54+
# be unaffected by any EPQ recheck behavior in the outer query).
55+
permutation s0_begin s0_update s1_begin s1_tuplock s0_commit s1_commit

‎src/include/executor/execScan.h‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,24 @@ ExecScanFetch(ScanState *node,
4949
{
5050
/*
5151
* This is a ForeignScan or CustomScan which has pushed down a
52-
* join to the remote side. The recheck method is responsible not
53-
* only for rechecking the scan/join quals but also for storing
54-
* the correct tuple in the slot.
52+
* join to the remote side. If it is a descendant node in the EPQ
53+
* recheck plan tree, run the recheck method function. Otherwise,
54+
* run the access method function below.
5555
*/
56+
if (bms_is_member(epqstate->epqParam, node->ps.plan->extParam))
57+
{
58+
/*
59+
* The recheck method is responsible not only for rechecking
60+
* the scan/join quals but also for storing the correct tuple
61+
* in the slot.
62+
*/
5663

57-
TupleTableSlot *slot = node->ss_ScanTupleSlot;
64+
TupleTableSlot *slot = node->ss_ScanTupleSlot;
5865

59-
if (!(*recheckMtd) (node, slot))
60-
ExecClearTuple(slot); /* would not be returned by scan */
61-
return slot;
66+
if (!(*recheckMtd) (node, slot))
67+
ExecClearTuple(slot); /* would not be returned by scan */
68+
return slot;
69+
}
6270
}
6371
else if (epqstate->relsubs_done[scanrelid - 1])
6472
{

0 commit comments

Comments
 (0)