Message-ID: <5C38910B.8010901@lab.ntt.co.jp>
Date: Fri, 11 Jan 2019 21:50:19 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: Query with high planning time at version 11.1 compared versions
 10.5 and 11.0
References: <8db4eca8-6e76-70ff-f934-e0228a1cc298@lab.ntt.co.jp>
 <20181206075726.pig32e6thltb56hr@alvherre.pgsql>
 <CAExHW5umxU5FOoT8FBcqp-GWKMtZhnuEy7U6bNH8QfzvYwUgEQ@mail.gmail.com>
 <CAExHW5uFwP_1N7RV6s8piQBvygGH+rppS60nFNHB9DKEM04ShA@mail.gmail.com>
 <5C34846E.9090600@lab.ntt.co.jp>
 <96720c99-ffa0-01ad-c594-0504c8eda708@lab.ntt.co.jp>
 <5C35D912.8040200@lab.ntt.co.jp>
 <7354b5bf-5932-4c0c-81ee-d9550de18995@lab.ntt.co.jp>
 <5C36E105.2020902@lab.ntt.co.jp>
 <dd1de042-dca5-cfb9-bc66-2e39020fbf6a@lab.ntt.co.jp>
In-Reply-To: <dd1de042-dca5-cfb9-bc66-2e39020fbf6a@lab.ntt.co.jp>
Content-Type: multipart/mixed;
 boundary="------------090907090604000601070601"
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, alvherre@2ndquadrant.com,
        pryzby@telsasoft.com, sanyo.moura@tatic.net,
        Jeff Janes <jeff.janes@gmail.com>, david.rowley@2ndquadrant.com,
        Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org
Precedence: bulk

This is a multi-part message in MIME format.
--------------090907090604000601070601
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

(2019/01/11 13:46), Amit Langote wrote:
> On 2019/01/10 15:07, 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?
>>
>> Will do.
>
> Thank you.

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.

Updated patch attached, which also updated other comments a little bit.

Best regards,
Etsuro Fujita

--------------090907090604000601070601
Content-Type: text/x-patch;
 name="0001-Add-child-EC-members-for-only-the-non-dummy-children-efujita-v2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0001-Add-child-EC-members-for-only-the-non-dummy-children-ef";
 filename*1="ujita-v2.patch"

*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***************
*** 1018,1059 **** 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.
  		 *
  		 * The child rel's targetlist might contain non-Var expressions, which
  		 * means that substitution into the quals could produce opportunities
--- 1018,1028 ----
  		Assert(childrel->reloptkind == RELOPT_OTHER_MEMBER_REL);
  
  		/*
! 		 * 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,1197 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  			continue;
  		}
  
! 		/* CE failed, so finish copying/modifying join quals. */
  		childrel->joininfo = (List *)
  			adjust_appendrel_attrs(root,
  								   (Node *) rel->joininfo,
  								   1, &appinfo);
  
  		/*
  		 * Note: we could compute appropriate attr_needed data for the child's
--- 1156,1191 ----
  			continue;
  		}
  
! 		/*
! 		 * 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,1212 **** set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		/*
  		 * If we consider partitionwise joins with the parent rel, do the same
  		 * for partitioned child rels.
  		 */
! 		if (rel->consider_partitionwise_join &&
! 			childRTE->relkind == RELKIND_PARTITIONED_TABLE)
  			childrel->consider_partitionwise_join = true;
  
  		/*
--- 1198,1209 ----
  		/*
  		 * 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;
  
  		/*
*** a/src/backend/optimizer/path/joinrels.c
--- b/src/backend/optimizer/path/joinrels.c
***************
*** 44,49 **** static void try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1,
--- 44,51 ----
  					   RelOptInfo *rel2, RelOptInfo *joinrel,
  					   SpecialJoinInfo *parent_sjinfo,
  					   List *parent_restrictlist);
+ static void update_child_rel_info(PlannerInfo *root,
+ 					  RelOptInfo *rel, RelOptInfo *childrel);
  static int match_expr_to_partition_keys(Expr *expr, RelOptInfo *rel,
  							 bool strict_op);
  
***************
*** 1312,1317 **** try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
--- 1314,1321 ----
  					   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;
  
***************
*** 1376,1381 **** try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
--- 1380,1406 ----
  		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);
***************
*** 1417,1422 **** try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
--- 1442,1469 ----
  	}
  }
  
+ /*
+  * 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;
+ }
+ 
  /*
   * Returns true if there exists an equi-join condition for each pair of
   * partition keys from given relations being joined.

--------------090907090604000601070601--