public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: Tom Lane <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Oversight in reparameterize_path_by_child leading to executor crash
Date: Wed, 2 Aug 2023 19:02:38 +0800
Message-ID: <CAMbWs4_vPd1yvxhiwD67o5cBsmgWc-py+B8gFFDDTrTQY5EXeg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAMbWs496+N=UAjOc=rcD3P7B6oJe4rZw08e_TZRUsWbPxZW3Tw@mail.gmail.com>
<[email protected]>
On Tue, Aug 1, 2023 at 9:20 PM Tom Lane <[email protected]> wrote:
> Richard Guo <[email protected]> writes:
> > So what I'm thinking is that maybe we can add a new type of path, named
> > SampleScanPath, to have the TableSampleClause per path. Then we can
> > safely reparameterize the TableSampleClause as needed for each
> > SampleScanPath. That's what the attached patch does.
>
> Alternatively, could we postpone the reparameterization until
> createplan.c? Having to build reparameterized expressions we might
> not use seems expensive, and it would also contribute to the memory
> bloat being complained of in nearby threads.
I did think about this option but figured out that it seems beyond the
scope of just fixing SampleScan. But if we want to optimize the
reparameterization mechanism besides fixing this crash, I think this
option is much better. I drafted a patch as attached.
Thanks
Richard
Attachments:
[application/octet-stream] v2-0001-Postpone-reparameterization-of-paths-until-when-creating-plans.patch (14.4K, ../CAMbWs4_vPd1yvxhiwD67o5cBsmgWc-py+B8gFFDDTrTQY5EXeg@mail.gmail.com/3-v2-0001-Postpone-reparameterization-of-paths-until-when-creating-plans.patch)
download | inline diff:
From 1e195c405014140090b2be49738a017eabb59de4 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Wed, 2 Aug 2023 16:09:55 +0800
Subject: [PATCH v2] Postpone reparameterization of paths until when creating
plans
---
src/backend/optimizer/path/joinpath.c | 52 +++----------
src/backend/optimizer/plan/createplan.c | 17 +++++
src/backend/optimizer/util/pathnode.c | 77 ++++++++++++++++++--
src/include/nodes/pathnodes.h | 13 ++++
src/include/optimizer/pathnode.h | 1 +
src/test/regress/expected/partition_join.out | 60 +++++++++++++++
src/test/regress/sql/partition_join.sql | 12 +++
7 files changed, 185 insertions(+), 47 deletions(-)
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 4b58936fa4..b5e208c761 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -30,19 +30,6 @@
/* Hook for plugins to get control in add_paths_to_joinrel() */
set_join_pathlist_hook_type set_join_pathlist_hook = NULL;
-/*
- * Paths parameterized by the parent can be considered to be parameterized by
- * any of its child.
- */
-#define PATH_PARAM_BY_PARENT(path, rel) \
- ((path)->param_info && bms_overlap(PATH_REQ_OUTER(path), \
- (rel)->top_parent_relids))
-#define PATH_PARAM_BY_REL_SELF(path, rel) \
- ((path)->param_info && bms_overlap(PATH_REQ_OUTER(path), (rel)->relids))
-
-#define PATH_PARAM_BY_REL(path, rel) \
- (PATH_PARAM_BY_REL_SELF(path, rel) || PATH_PARAM_BY_PARENT(path, rel))
-
static void try_partial_mergejoin_path(PlannerInfo *root,
RelOptInfo *joinrel,
Path *outer_path,
@@ -778,23 +765,15 @@ try_nestloop_path(PlannerInfo *root,
{
/*
* If the inner path is parameterized, it is parameterized by the
- * topmost parent of the outer rel, not the outer rel itself. Fix
- * that.
+ * topmost parent of the outer rel, not the outer rel itself. We will
+ * fix that in createplan.c. For now we need to check whether we can
+ * translate the inner path, and if not avoid creating nestloop path.
*/
- if (PATH_PARAM_BY_PARENT(inner_path, outer_path->parent))
+ if (PATH_PARAM_BY_PARENT(inner_path, outer_path->parent) &&
+ !path_is_reparameterizable_by_child(inner_path))
{
- inner_path = reparameterize_path_by_child(root, inner_path,
- outer_path->parent);
-
- /*
- * If we could not translate the path, we can't create nest loop
- * path.
- */
- if (!inner_path)
- {
- bms_free(required_outer);
- return;
- }
+ bms_free(required_outer);
+ return;
}
add_path(joinrel, (Path *)
@@ -869,20 +848,11 @@ try_partial_nestloop_path(PlannerInfo *root,
return;
/*
- * If the inner path is parameterized, it is parameterized by the topmost
- * parent of the outer rel, not the outer rel itself. Fix that.
+ * See the comments in try_nestloop_path.
*/
- if (PATH_PARAM_BY_PARENT(inner_path, outer_path->parent))
- {
- inner_path = reparameterize_path_by_child(root, inner_path,
- outer_path->parent);
-
- /*
- * If we could not translate the path, we can't create nest loop path.
- */
- if (!inner_path)
- return;
- }
+ if (PATH_PARAM_BY_PARENT(inner_path, outer_path->parent) &&
+ !path_is_reparameterizable_by_child(inner_path))
+ return;
/* Might be good enough to be worth trying, so let's try it. */
add_partial_path(joinrel, (Path *)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index af48109058..4cb37d4187 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -29,6 +29,7 @@
#include "optimizer/cost.h"
#include "optimizer/optimizer.h"
#include "optimizer/paramassign.h"
+#include "optimizer/pathnode.h"
#include "optimizer/paths.h"
#include "optimizer/placeholder.h"
#include "optimizer/plancat.h"
@@ -4327,6 +4328,22 @@ create_nestloop_plan(PlannerInfo *root,
List *nestParams;
Relids saveOuterRels = root->curOuterRels;
+ /*
+ * If the inner path is parameterized, it is parameterized by the
+ * topmost parent of the outer rel, not the outer rel itself. Fix
+ * that.
+ */
+ if (PATH_PARAM_BY_PARENT(best_path->jpath.innerjoinpath,
+ best_path->jpath.outerjoinpath->parent))
+ {
+ best_path->jpath.innerjoinpath =
+ reparameterize_path_by_child(root,
+ best_path->jpath.innerjoinpath,
+ best_path->jpath.outerjoinpath->parent);
+
+ Assert(best_path->jpath.innerjoinpath != NULL);
+ }
+
/* NestLoop can project, so no need to be picky about child tlists */
outer_plan = create_plan_recurse(root, best_path->jpath.outerjoinpath, 0);
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index f123fcb41e..1977ef1409 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2430,6 +2430,16 @@ create_nestloop_path(PlannerInfo *root,
{
NestPath *pathnode = makeNode(NestPath);
Relids inner_req_outer = PATH_REQ_OUTER(inner_path);
+ Relids outerrelids;
+
+ /*
+ * Paths are parameterized by top-level parents, so run parameterization
+ * tests on the parent relids.
+ */
+ if (outer_path->parent->top_parent_relids)
+ outerrelids = outer_path->parent->top_parent_relids;
+ else
+ outerrelids = outer_path->parent->relids;
/*
* If the inner path is parameterized by the outer, we must drop any
@@ -2439,7 +2449,7 @@ create_nestloop_path(PlannerInfo *root,
* estimates for this path. We detect such clauses by checking for serial
* number match to clauses already enforced in the inner path.
*/
- if (bms_overlap(inner_req_outer, outer_path->parent->relids))
+ if (bms_overlap(inner_req_outer, outerrelids))
{
Bitmapset *enforced_serials = get_param_path_clause_serials(inner_path);
List *jclauses = NIL;
@@ -4019,6 +4029,43 @@ reparameterize_path(PlannerInfo *root, Path *path,
return NULL;
}
+/*
+ * path_is_reparameterizable_by_child
+ * Given a path parameterized by the parent of a child relation, check to
+ * see if it can be translated to be parameterized by child relation.
+ *
+ * Currently, only a few path types are supported here, though more could be
+ * added at need.
+ */
+bool
+path_is_reparameterizable_by_child(Path *path)
+{
+ switch (nodeTag(path))
+ {
+ case T_Path:
+ case T_IndexPath:
+ case T_BitmapHeapPath:
+ case T_BitmapAndPath:
+ case T_BitmapOrPath:
+ case T_ForeignPath:
+ case T_CustomPath:
+ case T_NestPath:
+ case T_MergePath:
+ case T_HashPath:
+ case T_AppendPath:
+ case T_MaterialPath:
+ case T_MemoizePath:
+ case T_GatherPath:
+ return true;
+ default:
+
+ /* We don't know how to reparameterize this path. */
+ return false;
+ }
+
+ return false;
+}
+
/*
* reparameterize_path_by_child
* Given a path parameterized by the parent of the given child relation,
@@ -4047,11 +4094,13 @@ reparameterize_path_by_child(PlannerInfo *root, Path *path,
( (newnode) = makeNode(nodetype), \
memcpy((newnode), (node), sizeof(nodetype)) )
-#define ADJUST_CHILD_ATTRS(node) \
+#define ADJUST_CHILD_EXPRS(node, fieldtype) \
((node) = \
- (List *) adjust_appendrel_attrs_multilevel(root, (Node *) (node), \
- child_rel, \
- child_rel->top_parent))
+ (fieldtype) adjust_appendrel_attrs_multilevel(root, (Node *) (node), \
+ child_rel, \
+ child_rel->top_parent))
+
+#define ADJUST_CHILD_ATTRS(node) ADJUST_CHILD_EXPRS(node, List *)
#define REPARAMETERIZE_CHILD_PATH(path) \
do { \
@@ -4099,7 +4148,23 @@ do { \
switch (nodeTag(path))
{
case T_Path:
- FLAT_COPY_PATH(new_path, path, Path);
+ {
+ FLAT_COPY_PATH(new_path, path, Path);
+
+ if (path->pathtype == T_SampleScan)
+ {
+ Index scan_relid = path->parent->relid;
+ RangeTblEntry *rte;
+
+ /* it should be a base rel with a tablesample clause... */
+ Assert(scan_relid > 0);
+ rte = planner_rt_fetch(scan_relid, root);
+ Assert(rte->rtekind == RTE_RELATION);
+ Assert(rte->tablesample != NULL);
+
+ ADJUST_CHILD_EXPRS(rte->tablesample, TableSampleClause *);
+ }
+ }
break;
case T_IndexPath:
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index c17b53f7ad..c6c6c9eba8 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -1637,6 +1637,19 @@ typedef struct Path
#define PATH_REQ_OUTER(path) \
((path)->param_info ? (path)->param_info->ppi_req_outer : (Relids) NULL)
+/*
+ * Paths parameterized by the parent can be considered to be parameterized by
+ * any of its child.
+ */
+#define PATH_PARAM_BY_PARENT(path, rel) \
+ ((path)->param_info && bms_overlap(PATH_REQ_OUTER(path), \
+ (rel)->top_parent_relids))
+#define PATH_PARAM_BY_REL_SELF(path, rel) \
+ ((path)->param_info && bms_overlap(PATH_REQ_OUTER(path), (rel)->relids))
+
+#define PATH_PARAM_BY_REL(path, rel) \
+ (PATH_PARAM_BY_REL_SELF(path, rel) || PATH_PARAM_BY_PARENT(path, rel))
+
/*----------
* IndexPath represents an index scan over a single index.
*
diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h
index 001e75b5b7..85f5562033 100644
--- a/src/include/optimizer/pathnode.h
+++ b/src/include/optimizer/pathnode.h
@@ -295,6 +295,7 @@ extern Path *reparameterize_path(PlannerInfo *root, Path *path,
double loop_count);
extern Path *reparameterize_path_by_child(PlannerInfo *root, Path *path,
RelOptInfo *child_rel);
+extern bool path_is_reparameterizable_by_child(Path *path);
/*
* prototypes for relnode.c
diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.out
index 6560fe2416..a11f738411 100644
--- a/src/test/regress/expected/partition_join.out
+++ b/src/test/regress/expected/partition_join.out
@@ -505,6 +505,31 @@ SELECT t1.a, ss.t2a, ss.t2c FROM prt1 t1 LEFT JOIN LATERAL
550 | |
(12 rows)
+-- lateral reference in sample scan
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt1 t1 JOIN LATERAL
+ (SELECT * FROM prt1 t2 TABLESAMPLE SYSTEM (t1.a) REPEATABLE(t1.b)) s
+ ON t1.a = s.a;
+ QUERY PLAN
+-------------------------------------------------------------
+ Append
+ -> Nested Loop
+ -> Seq Scan on prt1_p1 t1_1
+ -> Sample Scan on prt1_p1 t2_1
+ Sampling: system (t1_1.a) REPEATABLE (t1_1.b)
+ Filter: (t1_1.a = a)
+ -> Nested Loop
+ -> Seq Scan on prt1_p2 t1_2
+ -> Sample Scan on prt1_p2 t2_2
+ Sampling: system (t1_2.a) REPEATABLE (t1_2.b)
+ Filter: (t1_2.a = a)
+ -> Nested Loop
+ -> Seq Scan on prt1_p3 t1_3
+ -> Sample Scan on prt1_p3 t2_3
+ Sampling: system (t1_3.a) REPEATABLE (t1_3.b)
+ Filter: (t1_3.a = a)
+(16 rows)
+
-- bug with inadequate sort key representation
SET enable_partitionwise_aggregate TO true;
SET enable_hashjoin TO false;
@@ -1944,6 +1969,41 @@ SELECT * FROM prt1_l t1 LEFT JOIN LATERAL
550 | 0 | 0002 | | | | |
(12 rows)
+-- partitionwise join with lateral reference in sample scan
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt1_l t1 JOIN LATERAL
+ (SELECT * FROM prt1_l t2 TABLESAMPLE SYSTEM (t1.a) REPEATABLE(t1.b)) s ON
+ t1.a = s.a AND t1.b = s.b AND t1.c = s.c;
+ QUERY PLAN
+----------------------------------------------------------------------------------------
+ Append
+ -> Nested Loop
+ -> Seq Scan on prt1_l_p1 t1_1
+ -> Sample Scan on prt1_l_p1 t2_1
+ Sampling: system (t1_1.a) REPEATABLE (t1_1.b)
+ Filter: ((t1_1.a = a) AND (t1_1.b = b) AND ((t1_1.c)::text = (c)::text))
+ -> Nested Loop
+ -> Seq Scan on prt1_l_p2_p1 t1_2
+ -> Sample Scan on prt1_l_p2_p1 t2_2
+ Sampling: system (t1_2.a) REPEATABLE (t1_2.b)
+ Filter: ((t1_2.a = a) AND (t1_2.b = b) AND ((t1_2.c)::text = (c)::text))
+ -> Nested Loop
+ -> Seq Scan on prt1_l_p2_p2 t1_3
+ -> Sample Scan on prt1_l_p2_p2 t2_3
+ Sampling: system (t1_3.a) REPEATABLE (t1_3.b)
+ Filter: ((t1_3.a = a) AND (t1_3.b = b) AND ((t1_3.c)::text = (c)::text))
+ -> Nested Loop
+ -> Seq Scan on prt1_l_p3_p1 t1_4
+ -> Sample Scan on prt1_l_p3_p1 t2_4
+ Sampling: system (t1_4.a) REPEATABLE (t1_4.b)
+ Filter: ((t1_4.a = a) AND (t1_4.b = b) AND ((t1_4.c)::text = (c)::text))
+ -> Nested Loop
+ -> Seq Scan on prt1_l_p3_p2 t1_5
+ -> Sample Scan on prt1_l_p3_p2 t2_5
+ Sampling: system (t1_5.a) REPEATABLE (t1_5.b)
+ Filter: ((t1_5.a = a) AND (t1_5.b = b) AND ((t1_5.c)::text = (c)::text))
+(26 rows)
+
-- join with one side empty
EXPLAIN (COSTS OFF)
SELECT t1.a, t1.c, t2.b, t2.c FROM (SELECT * FROM prt1_l WHERE a = 1 AND a = 2) t1 RIGHT JOIN prt2_l t2 ON t1.a = t2.b AND t1.b = t2.a AND t1.c = t2.c;
diff --git a/src/test/regress/sql/partition_join.sql b/src/test/regress/sql/partition_join.sql
index 48daf3aee3..e2daab03fb 100644
--- a/src/test/regress/sql/partition_join.sql
+++ b/src/test/regress/sql/partition_join.sql
@@ -100,6 +100,12 @@ SELECT t1.a, ss.t2a, ss.t2c FROM prt1 t1 LEFT JOIN LATERAL
(SELECT t2.a AS t2a, t3.a AS t3a, t2.b t2b, t2.c t2c, least(t1.a,t2.a,t3.a) FROM prt1 t2 JOIN prt2 t3 ON (t2.a = t3.b)) ss
ON t1.c = ss.t2c WHERE (t1.b + coalesce(ss.t2b, 0)) = 0 ORDER BY t1.a;
+-- lateral reference in sample scan
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt1 t1 JOIN LATERAL
+ (SELECT * FROM prt1 t2 TABLESAMPLE SYSTEM (t1.a) REPEATABLE(t1.b)) s
+ ON t1.a = s.a;
+
-- bug with inadequate sort key representation
SET enable_partitionwise_aggregate TO true;
SET enable_hashjoin TO false;
@@ -387,6 +393,12 @@ SELECT * FROM prt1_l t1 LEFT JOIN LATERAL
(SELECT t2.a AS t2a, t2.c AS t2c, t2.b AS t2b, t3.b AS t3b, least(t1.a,t2.a,t3.b) FROM prt1_l t2 JOIN prt2_l t3 ON (t2.a = t3.b AND t2.c = t3.c)) ss
ON t1.a = ss.t2a AND t1.c = ss.t2c WHERE t1.b = 0 ORDER BY t1.a;
+-- partitionwise join with lateral reference in sample scan
+EXPLAIN (COSTS OFF)
+SELECT * FROM prt1_l t1 JOIN LATERAL
+ (SELECT * FROM prt1_l t2 TABLESAMPLE SYSTEM (t1.a) REPEATABLE(t1.b)) s ON
+ t1.a = s.a AND t1.b = s.b AND t1.c = s.c;
+
-- join with one side empty
EXPLAIN (COSTS OFF)
SELECT t1.a, t1.c, t2.b, t2.c FROM (SELECT * FROM prt1_l WHERE a = 1 AND a = 2) t1 RIGHT JOIN prt2_l t2 ON t1.a = t2.b AND t1.b = t2.a AND t1.c = t2.c;
--
2.31.0
view thread (3+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Oversight in reparameterize_path_by_child leading to executor crash
In-Reply-To: <CAMbWs4_vPd1yvxhiwD67o5cBsmgWc-py+B8gFFDDTrTQY5EXeg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox