RE: speeding up planning with partitions - Mailing list pgsql-hackers

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