RE: speeding up planning with partitions

Hi Amit,

On Tue, Nov 20, 2018 at 10:24 PM, Amit Langote wrote:
> Attached v8 patches.

Thanks for the patch. I took a look 0003, 0005, 0006 of v8 patch.

1.
0003: line 267-268
+		 * Child relation may have be marked dummy if build_append_child_rel
+		 * found self-contradictory quals.

/s/have be marked/have been marked/

2.
0003: around line 1077
In append.c(or prepunion.c)
228      * Check that there's at least one descendant, else treat as no-child
229      * case.  This could happen despite above has_subclass() check, if table
230      * once had a child but no longer does.

has_subclass() check is moved to subquery_planner from above this code,
so the comments need to be modified like below.

s/above has_subclass() check/has_subclass() check in subquery_planner/

3.
0003: line 1241-1244
0006: line ?

In comments of expand_partitioned_rtentry:
+ * Note: even though only the unpruned partitions will be added to the
+ * resulting plan, this still locks *all* partitions via find_all_inheritors
+ * in order to avoid partitions being locked in a different order than other
+ * places in the backend that may lock partitions.

This comments is no more needed if 0006 patch is applied because
find_all_inheritors is removed in the 0006 patch.

4.
0003: line 1541-1544

+	 * Add the RelOptInfo.  Even though we may not really scan this relation
+	 * for reasons such as contradictory quals, we still need need to create
+	 * one, because for every RTE in the query's range table, there must be an
+	 * accompanying RelOptInfo.

s/need need/need/

5.
0003: line 1620-1621

+ * After creating the RelOptInfo for the given child RT index, it goes on to
+ * initialize some of its fields base on the parent RelOptInfo.

s/fields base on/fields based on/

6.
parsenodes.h
 906  *    inh is true for relation references that should be expanded to include
 907  *    inheritance children, if the rel has any.  This *must* be false for
 908  *    RTEs other than RTE_RELATION entries.

I think inh can become true now even if RTEKind equals RTE_SUBQUERY, so latter
sentence need to be modified.

7.
0005: line 109-115
+		/*
+		 * If partition is excluded by constraints, remove it from
+		 * live_parts, too.
+		 */
+		if (IS_DUMMY_REL(childrel))
+			parentrel->live_parts = bms_del_member(parentrel->live_parts, i);
+

When I read this comment, I imagined that relation_excluded_by_constraints()
would be called before this code. childrel is marked dummy if
build_append_child_rel found self-contradictory quals, so comments can be
modified more correctly like another comments in your patch as below.

In 0003: line 267-271
+		 * Child relation may have be marked dummy if build_append_child_rel
+		 * found self-contradictory quals.
+		 */
+		if (IS_DUMMY_REL(childrel))
+			continue;

8.
0003: line 705-711
+		 * Query planning may have added some columns to the top-level tlist,
+		 * which happens when there are row marks applied to inheritance
+		 * parent relations (additional junk columns needed for applying row
+		 * marks are added after expanding inheritance.)
+		 */
+		if (list_length(tlist) < list_length(root->processed_tlist))
+			tlist = root->processed_tlist;

In grouping_planner():

  if (planned_rel == NULL)
  {
      ...
      root->processed_tlist = tlist;
  }
  else
      tlist = root->processed_tlist;
  ...
  if (current_rel == NULL)
      current_rel = query_planner(root, tlist,
                                  standard_qp_callback, &qp_extra);
  ...
  /*
   * Query planning may have added some columns to the top-level tlist,
   * which happens when there are row marks applied to inheritance
   * parent relations (additional junk columns needed for applying row
   * marks are added after expanding inheritance.)
   */
  if (list_length(tlist) < list_length(root->processed_tlist))
      tlist = root->processed_tlist;


Are there any case tlist points to an address different from
root->processed_tlist after calling query_planner? Junk columns are possibly
added to root->processed_tlist after expanding inheritance, but that adding
process don't change the root->processed_tlist's pointer address.
I checked "Assert(tlist == root->processed_tlist)" after calling query_planner
passes "make check".

9.
0003: line 1722-1763
In build_append_child_rel():

+	/*
+	 * In addition to the quals inherited from the parent, we might
+	 * have securityQuals associated with this particular child node.
+	 * (Currently this can only happen in appendrels originating from
+	 * UNION ALL; inheritance child tables don't have their own
+	 * securityQuals.)	Pull any such securityQuals up into the
...
+		foreach(lc, childRTE->securityQuals)
+		{
...
+		}
+		Assert(security_level <= root->qual_security_level);
+	}

This foreach loop loops only once in the current regression tests. I checked
"Assert(childRTE->securityQuals->length == 1)" passes "make check".
I think there are no need to change codes, I state this fact only for sharing.


--
Yoshikazu Imai