From 32c854736b90aacad38715ab6c02b73140c8c429 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Tue, 25 Apr 2023 16:10:42 +0800 Subject: [PATCH v1] Fix how we handle pseudoconstant clauses for scans of foreign joins When creating a scan plan, we need to extract all the pseudoconstant clauses which are supposed to be used as one-time quals in a gating Result node. Normally it suffices to check against baserestrictinfo of the parent relation of 'best_path', as well as the join clauses available from the outer relations if this is a parameterized scan. But for scans of foreign joins, we do not have any restriction clauses in these places, and that leads to gating clauses being lost in this case. To fix, store the restriction clauses for foreign joins in ForeignPath and check against them for gating clauses. --- .../postgres_fdw/expected/postgres_fdw.out | 21 +++++++++++++++++++ contrib/postgres_fdw/postgres_fdw.c | 13 +++++++----- contrib/postgres_fdw/sql/postgres_fdw.sql | 6 ++++++ src/backend/optimizer/plan/createplan.c | 9 +++++++- src/backend/optimizer/util/pathnode.c | 4 ++++ src/include/nodes/pathnodes.h | 5 +++++ src/include/optimizer/pathnode.h | 1 + 7 files changed, 53 insertions(+), 6 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index fd5752bd5b..c8b072c255 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -893,6 +893,27 @@ SELECT * FROM ft2 WHERE c1 = ANY (ARRAY(SELECT c1 FROM ft1 WHERE c1 < 5)); 4 | 4 | 00004 | Mon Jan 05 00:00:00 1970 PST | Mon Jan 05 00:00:00 1970 | 4 | 4 | foo (4 rows) +-- bug due to sloppy handling of pseudoconstant clauses for foreign joins +EXPLAIN (VERBOSE, COSTS OFF) + SELECT * FROM ft2 a, ft2 b + WHERE b.c1 = a.c1 AND now() < '25-April-2023'::timestamp; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ + Result + Output: a.c1, a.c2, a.c3, a.c4, a.c5, a.c6, a.c7, a.c8, b.c1, b.c2, b.c3, b.c4, b.c5, b.c6, b.c7, b.c8 + One-Time Filter: (now() < 'Tue Apr 25 00:00:00 2023'::timestamp without time zone) + -> Foreign Scan + Output: a.c1, a.c2, a.c3, a.c4, a.c5, a.c6, a.c7, a.c8, b.c1, b.c2, b.c3, b.c4, b.c5, b.c6, b.c7, b.c8 + Relations: (public.ft2 a) INNER JOIN (public.ft2 b) + Remote SQL: SELECT r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8, r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8 FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")))) +(7 rows) + +SELECT * FROM ft2 a, ft2 b +WHERE b.c1 = a.c1 AND now() < '25-April-2023'::timestamp; + c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8 | c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8 +----+----+----+----+----+----+----+----+----+----+----+----+----+----+----+---- +(0 rows) + -- we should not push order by clause with volatile expressions or unsafe -- collations EXPLAIN (VERBOSE, COSTS OFF) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 95dbe8b06c..2c79981289 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -524,7 +524,7 @@ static List *get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel); static List *get_useful_ecs_for_relation(PlannerInfo *root, RelOptInfo *rel); static void add_paths_with_pathkeys_for_rel(PlannerInfo *root, RelOptInfo *rel, - Path *epq_path); + Path *epq_path, List *joinrestrictinfo); static void add_foreign_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, RelOptInfo *grouped_rel, @@ -1038,7 +1038,7 @@ postgresGetForeignPaths(PlannerInfo *root, add_path(baserel, (Path *) path); /* Add paths with pathkeys */ - add_paths_with_pathkeys_for_rel(root, baserel, NULL); + add_paths_with_pathkeys_for_rel(root, baserel, NULL, NULL); /* * If we're not using remote estimates, stop here. We have no way to @@ -5992,7 +5992,7 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype, static void add_paths_with_pathkeys_for_rel(PlannerInfo *root, RelOptInfo *rel, - Path *epq_path) + Path *epq_path, List *joinrestrictinfo) { List *useful_pathkeys_list = NIL; /* List of all pathkeys */ ListCell *lc; @@ -6096,6 +6096,7 @@ add_paths_with_pathkeys_for_rel(PlannerInfo *root, RelOptInfo *rel, total_cost, useful_pathkeys, rel->lateral_relids, + joinrestrictinfo, sorted_epq_path, NIL)); } @@ -6348,6 +6349,7 @@ postgresGetForeignJoinPaths(PlannerInfo *root, total_cost, NIL, /* no pathkeys */ joinrel->lateral_relids, + extra->restrictlist, epq_path, NIL); /* no fdw_private */ @@ -6355,7 +6357,8 @@ postgresGetForeignJoinPaths(PlannerInfo *root, add_path(joinrel, (Path *) joinpath); /* Consider pathkeys for the join relation */ - add_paths_with_pathkeys_for_rel(root, joinrel, epq_path); + add_paths_with_pathkeys_for_rel(root, joinrel, epq_path, + extra->restrictlist); /* XXX Consider parameterized paths for the join relation */ } @@ -6981,7 +6984,7 @@ add_foreign_final_paths(PlannerInfo *root, RelOptInfo *input_rel, path->total_cost, path->pathkeys, NULL, /* no extra plan */ - NULL); /* no fdw_private */ + NIL); /* no fdw_private */ /* and add it to the final_rel */ add_path(final_rel, (Path *) final_path); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index c05046f867..246f4c15d8 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -355,6 +355,12 @@ WHERE a.c2 = 6 AND b.c1 = a.c1 AND a.c8 = 'foo' AND b.c7 = upper(a.c7); -- bug before 9.3.5 due to sloppy handling of remote-estimate parameters SELECT * FROM ft1 WHERE c1 = ANY (ARRAY(SELECT c1 FROM ft2 WHERE c1 < 5)); SELECT * FROM ft2 WHERE c1 = ANY (ARRAY(SELECT c1 FROM ft1 WHERE c1 < 5)); +-- bug due to sloppy handling of pseudoconstant clauses for foreign joins +EXPLAIN (VERBOSE, COSTS OFF) + SELECT * FROM ft2 a, ft2 b + WHERE b.c1 = a.c1 AND now() < '25-April-2023'::timestamp; +SELECT * FROM ft2 a, ft2 b +WHERE b.c1 = a.c1 AND now() < '25-April-2023'::timestamp; -- we should not push order by clause with volatile expressions or unsafe -- collations EXPLAIN (VERBOSE, COSTS OFF) diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 910ffbf1e1..7e20e89813 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -599,8 +599,15 @@ create_scan_plan(PlannerInfo *root, Path *best_path, int flags) * Detect whether we have any pseudoconstant quals to deal with. Then, if * we'll need a gating Result node, it will be able to project, so there * are no requirements on the child's tlist. + * + * Note that for scans of foreign joins, we do not have restriction clauses + * stored in baserestrictinfo and we do not consider parameterization. + * Instead we need to check against joinrestrictinfo stored in ForeignPath. */ - gating_clauses = get_gating_quals(root, scan_clauses); + if (best_path->pathtype == T_ForeignScan && IS_JOIN_REL(rel)) + gating_clauses = get_gating_quals(root, ((ForeignPath *) best_path)->joinrestrictinfo); + else + gating_clauses = get_gating_quals(root, scan_clauses); if (gating_clauses) flags = 0; diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 5f5596841c..92f18c6846 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -2249,6 +2249,7 @@ create_foreignscan_path(PlannerInfo *root, RelOptInfo *rel, pathnode->path.total_cost = total_cost; pathnode->path.pathkeys = pathkeys; + pathnode->joinrestrictinfo = NIL; pathnode->fdw_outerpath = fdw_outerpath; pathnode->fdw_private = fdw_private; @@ -2272,6 +2273,7 @@ create_foreign_join_path(PlannerInfo *root, RelOptInfo *rel, double rows, Cost startup_cost, Cost total_cost, List *pathkeys, Relids required_outer, + List *joinrestrictinfo, Path *fdw_outerpath, List *fdw_private) { @@ -2299,6 +2301,7 @@ create_foreign_join_path(PlannerInfo *root, RelOptInfo *rel, pathnode->path.total_cost = total_cost; pathnode->path.pathkeys = pathkeys; + pathnode->joinrestrictinfo = joinrestrictinfo; pathnode->fdw_outerpath = fdw_outerpath; pathnode->fdw_private = fdw_private; @@ -2344,6 +2347,7 @@ create_foreign_upper_path(PlannerInfo *root, RelOptInfo *rel, pathnode->path.total_cost = total_cost; pathnode->path.pathkeys = pathkeys; + pathnode->joinrestrictinfo = NIL; pathnode->fdw_outerpath = fdw_outerpath; pathnode->fdw_private = fdw_private; diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index cf28416da8..5adf35ac1d 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -1828,6 +1828,10 @@ typedef struct SubqueryScanPath * ForeignPath represents a potential scan of a foreign table, foreign join * or foreign upper-relation. * + * In the case of foreign join, joinrestrictinfo stores the RestrictInfos to + * apply to this join. It is needed because we need to extract gating clauses + * from it. + * * fdw_private stores FDW private data about the scan. While fdw_private is * not actually touched by the core code during normal operations, it's * generally a good idea to use a representation that can be dumped by @@ -1837,6 +1841,7 @@ typedef struct SubqueryScanPath typedef struct ForeignPath { Path path; + List *joinrestrictinfo; /* RestrictInfos to apply to join */ Path *fdw_outerpath; List *fdw_private; } ForeignPath; diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h index 69be701b16..9f8f1b2798 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -134,6 +134,7 @@ extern ForeignPath *create_foreign_join_path(PlannerInfo *root, RelOptInfo *rel, double rows, Cost startup_cost, Cost total_cost, List *pathkeys, Relids required_outer, + List *joinrestrictinfo, Path *fdw_outerpath, List *fdw_private); extern ForeignPath *create_foreign_upper_path(PlannerInfo *root, RelOptInfo *rel, -- 2.31.0