(2019/01/16 14:45), Etsuro Fujita wrote:
(2019/01/15 11:42), Amit Langote wrote:
On 2019/01/11 21:50, Etsuro Fujita wrote:
(2019/01/10 10:41), Amit Langote wrote:
That's a loaded meaning and abusing it to mean something else can be
challenged, but we can live with that if properly documented.
Speaking of
which:
/* used by partitionwise joins: */
bool consider_partitionwise_join; /* consider
partitionwise join
* paths? (if
partitioned
rel) */
Maybe, mention here how it will be abused in back-branches for
non-partitioned relations?
I know we don't yet reach a consensus on what to do in details to
address
this issue, but for the above, how about adding comments like this to
set_append_rel_size(), instead of the header file:
/*
* If we consider partitionwise joins with the parent rel, do the
same
* for partitioned child rels.
*
* Note: here we abuse the consider_partitionwise_join flag for child
* rels that are not partitioned, to tell try_partitionwise_join()
* that their targetlists and EC entries have been generated.
*/
if (rel->consider_partitionwise_join)
childrel->consider_partitionwise_join = true;
ISTM that that would be more clearer than the header file.
Thanks for updating the patch. I tend to agree that it might be better to
add such details here than in the header as it's better to keep the
latter
more stable.
About the comment you added, I think we could clarify the note further
as:
Note: here we abuse the consider_partitionwise_join flag by setting it
*even* for child rels that are not partitioned. In that case, we set it
to tell try_partitionwise_join() that it doesn't need to generate their
targetlists and EC entries as they have already been generated here, as
opposed to the dummy child rels for which the flag is left set to
false so
that it will generate them.
Maybe it's a bit wordy, but it helps get the intention across more
clearly.
I think that is well-worded, so +1 from me.
I updated the patch as such and rebased it to the latest HEAD. I also
added the commit message. Attached is an updated patch. Does that make
sense? If there are no objections, I'll push that patch early next week.
Best regards,
Etsuro Fujita
>From 207e3dfdf6770a043c14d512956fd5ddc5421daa Mon Sep 17 00:00:00 2001
From: Etsuro Fujita <efuj...@postgresql.org>
Date: Fri, 18 Jan 2019 21:49:31 +0900
Subject: [PATCH] Postpone generating tlists and EC members for inheritance
dummy children.
Previously, in set_append_rel_size(), we generated tlists and EC members
for dummy children for possible use by partition-wise join, even if
partition-wise join was disabled, but adding such EC members causes
noticeable planning speed degradation in cases where there are lots of
dummy children, as the EC members lists grow huge, which makes the
search for their parent EC members in add_child_rel_equivalences()
much time-consuming. Postpone the work until such children are actually
involved in a partition-wise join.
Reported-by: Sanyo Capobiango
Analyzed-by: Justin Pryzby and Alvaro Herrera
Author: Amit Langote, with a few additional changes by me
Reviewed-by: Ashutosh Bapat
Backpatch-through: v11 where partition-wise join was added
Discussion: https://postgr.es/m/CAO698qZnrxoZu7MEtfiJmpmUtz3AVYFVnwzR%2BpqjF%3DrmKBTgpw%40mail.gmail.com
---
src/backend/optimizer/path/allpaths.c | 78 +++++++++++++--------------
src/backend/optimizer/path/joinrels.c | 47 ++++++++++++++++
2 files changed, 86 insertions(+), 39 deletions(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index bc389b52e4..e1ecfd6052 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1018,42 +1018,11 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
/*
- * Copy/Modify targetlist. Even if this child is deemed empty, we need
- * its targetlist in case it falls on nullable side in a child-join
- * because of partitionwise join.
- *
- * NB: the resulting childrel->reltarget->exprs may contain arbitrary
- * expressions, which otherwise would not occur in a rel's targetlist.
- * Code that might be looking at an appendrel child must cope with
- * such. (Normally, a rel's targetlist would only include Vars and
- * PlaceHolderVars.) XXX we do not bother to update the cost or width
- * fields of childrel->reltarget; not clear if that would be useful.
- */
- childrel->reltarget->exprs = (List *)
- adjust_appendrel_attrs(root,
- (Node *) rel->reltarget->exprs,
- 1, &appinfo);
-
- /*
- * We have to make child entries in the EquivalenceClass data
- * structures as well. This is needed either if the parent
- * participates in some eclass joins (because we will want to consider
- * inner-indexscan joins on the individual children) or if the parent
- * has useful pathkeys (because we should try to build MergeAppend
- * paths that produce those sort orderings). Even if this child is
- * deemed dummy, it may fall on nullable side in a child-join, which
- * in turn may participate in a MergeAppend, where we will need the
- * EquivalenceClass data structures.
- */
- if (rel->has_eclass_joins || has_useful_pathkeys(root, rel))
- add_child_rel_equivalences(root, appinfo, rel, childrel);
- childrel->has_eclass_joins = rel->has_eclass_joins;
-
- /*
- * We have to copy the parent's quals to the child, with appropriate
- * substitution of variables. However, only the baserestrictinfo
- * quals are needed before we can check for constraint exclusion; so
- * do that first and then check to see if we can disregard this child.
+ * We have to copy the parent's targetlist and quals to the child,
+ * with appropriate substitution of variables. However, only the
+ * baserestrictinfo quals are needed before we can check for
+ * constraint exclusion; so do that first and then check to see if we
+ * can disregard this child.
*
* The child rel's targetlist might contain non-Var expressions, which
* means that substitution into the quals could produce opportunities
@@ -1187,11 +1156,36 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
continue;
}
- /* CE failed, so finish copying/modifying join quals. */
+ /*
+ * CE failed, so finish copying/modifying targetlist and join quals.
+ *
+ * NB: the resulting childrel->reltarget->exprs may contain arbitrary
+ * expressions, which otherwise would not occur in a rel's targetlist.
+ * Code that might be looking at an appendrel child must cope with
+ * such. (Normally, a rel's targetlist would only include Vars and
+ * PlaceHolderVars.) XXX we do not bother to update the cost or width
+ * fields of childrel->reltarget; not clear if that would be useful.
+ */
childrel->joininfo = (List *)
adjust_appendrel_attrs(root,
(Node *) rel->joininfo,
1, &appinfo);
+ childrel->reltarget->exprs = (List *)
+ adjust_appendrel_attrs(root,
+ (Node *) rel->reltarget->exprs,
+ 1, &appinfo);
+
+ /*
+ * We have to make child entries in the EquivalenceClass data
+ * structures as well. This is needed either if the parent
+ * participates in some eclass joins (because we will want to consider
+ * inner-indexscan joins on the individual children) or if the parent
+ * has useful pathkeys (because we should try to build MergeAppend
+ * paths that produce those sort orderings).
+ */
+ if (rel->has_eclass_joins || has_useful_pathkeys(root, rel))
+ add_child_rel_equivalences(root, appinfo, rel, childrel);
+ childrel->has_eclass_joins = rel->has_eclass_joins;
/*
* Note: we could compute appropriate attr_needed data for the child's
@@ -1204,9 +1198,15 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
/*
* If we consider partitionwise joins with the parent rel, do the same
* for partitioned child rels.
+ *
+ * Note: here we abuse the consider_partitionwise_join flag by setting
+ * it *even* for child rels that are not partitioned. In that case,
+ * we set it to tell try_partitionwise_join() that it doesn't need to
+ * generate their targetlists and EC entries as they have already been
+ * generated here, as opposed to the dummy child rels for which the
+ * flag is left set to false so that it will generate them.
*/
- if (rel->consider_partitionwise_join &&
- childRTE->relkind == RELKIND_PARTITIONED_TABLE)
+ if (rel->consider_partitionwise_join)
childrel->consider_partitionwise_join = true;
/*
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 00611a5e40..8bfe9c3ff7 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -44,6 +44,8 @@ static void try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1,
RelOptInfo *rel2, RelOptInfo *joinrel,
SpecialJoinInfo *parent_sjinfo,
List *parent_restrictlist);
+static void update_child_rel_info(PlannerInfo *root,
+ RelOptInfo *rel, RelOptInfo *childrel);
static SpecialJoinInfo *build_child_join_sjinfo(PlannerInfo *root,
SpecialJoinInfo *parent_sjinfo,
Relids left_relids, Relids right_relids);
@@ -1315,6 +1317,8 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
RelOptInfo *joinrel, SpecialJoinInfo *parent_sjinfo,
List *parent_restrictlist)
{
+ bool rel1_is_simple = IS_SIMPLE_REL(rel1);
+ bool rel2_is_simple = IS_SIMPLE_REL(rel2);
int nparts;
int cnt_parts;
@@ -1379,6 +1383,27 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
AppendRelInfo **appinfos;
int nappinfos;
+ /*
+ * If a child table has consider_partitionwise_join=false, it means
+ * that it's a dummy relation for which we skipped setting up tlist
+ * expressions and adding EC members in set_append_rel_size(), so do
+ * that now for use later.
+ */
+ if (rel1_is_simple && !child_rel1->consider_partitionwise_join)
+ {
+ Assert(child_rel1->reloptkind == RELOPT_OTHER_MEMBER_REL);
+ Assert(IS_DUMMY_REL(child_rel1));
+ update_child_rel_info(root, rel1, child_rel1);
+ child_rel1->consider_partitionwise_join = true;
+ }
+ if (rel2_is_simple && !child_rel2->consider_partitionwise_join)
+ {
+ Assert(child_rel2->reloptkind == RELOPT_OTHER_MEMBER_REL);
+ Assert(IS_DUMMY_REL(child_rel2));
+ update_child_rel_info(root, rel2, child_rel2);
+ child_rel2->consider_partitionwise_join = true;
+ }
+
/* We should never try to join two overlapping sets of rels. */
Assert(!bms_overlap(child_rel1->relids, child_rel2->relids));
child_joinrelids = bms_union(child_rel1->relids, child_rel2->relids);
@@ -1420,6 +1445,28 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
}
}
+/*
+ * Set up tlist expressions for the childrel, and add EC members referencing
+ * the childrel.
+ */
+static void
+update_child_rel_info(PlannerInfo *root,
+ RelOptInfo *rel, RelOptInfo *childrel)
+{
+ AppendRelInfo *appinfo = root->append_rel_array[childrel->relid];
+
+ /* Make child tlist expressions */
+ childrel->reltarget->exprs = (List *)
+ adjust_appendrel_attrs(root,
+ (Node *) rel->reltarget->exprs,
+ 1, &appinfo);
+
+ /* Make child entries in the EquivalenceClass as well */
+ if (rel->has_eclass_joins || has_useful_pathkeys(root, rel))
+ add_child_rel_equivalences(root, appinfo, rel, childrel);
+ childrel->has_eclass_joins = rel->has_eclass_joins;
+}
+
/*
* Construct the SpecialJoinInfo for a child-join by translating
* SpecialJoinInfo for the join between parents. left_relids and right_relids
--
2.19.2
