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

Hi, Amit,

On Fri, Dec 7, 2018 at 0:57 AM, Imai, Yoshikazu wrote:
> OK. I will continue the review of 0001 before/after your supplying of
> next patch with keeping those in mind.

Here's the continuation of the review. Almost all of below comments are
little fixes.

---
0001: line 76-77
In commit message:
  exclusion for target child relation, which is no longer
  is no longer needed.  Constraint exclusion runs during query_planner

s/which is no longer is no longer needed/which is no longer needed/

---
0001: line 464
+    if (IS_DUMMY_REL(find_base_rel(root, resultRelation )))

s/resultRelation )))/resultRelation)))/
(There is an extra space.)

---
0001: line 395-398
+     * Reset inh_target_child_roots to not be same as parent root's so that
+     * the subroots for this child's own children (if any) don't end up in
+     * root parent's list.  We'll eventually merge all entries into one list,
+     * but that's now now.

s/that's now now/that's not now/

---
0001: line 794
+     * are put into  a list that will be controlled by a single ModifyTable

s/are put into  a list/are put into a list/
(There are two spaces between "into" and "a".)

---
0001: line 241-242, 253-254, 291-294 (In set_append_rel_size())

+        if (appinfo->parent_relid == root->parse->resultRelation)
+            subroot = adjust_inherit_target_child(root, childrel, appinfo);

+            add_child_rel_equivalences(subroot, appinfo, rel, childrel,
+                                       root != subroot);

+        if (subroot != root)
+        {
+            root->inh_target_child_roots =
+                            lappend(root->inh_target_child_roots, subroot);

A boolean value of "appinfo->parent_relid == root->parse->resultRelation" is
same with "subroot != root"(because of line 241-242), so we can use bool
variable here like
child_is_target = (appinfo->parent_relid == root->parse->resultRelation).
The name of child_is_target is brought from arguments of
add_child_rel_equivalences() in your patch.

I attached a little diff as "v9-0001-delta.diff".

---
0001: line 313-431

adjust_inherit_target_child() is in allpaths.c in your patch, but it has the
code like below ones which are in master's(not patch applied) planner.c or
planmain.c, so could it be possible in planner.c(or planmain.c)?
This point is less important, but I was just wondering whether 
adjust_inherit_target_child() should be in allpaths.c, planner.c or planmain.c.

+    /* Translate the original query's expressions to this child. */
+    subroot = makeNode(PlannerInfo);
+    memcpy(subroot, root, sizeof(PlannerInfo));

+    root->parse->targetList = root->unexpanded_tlist;
+    subroot->parse = (Query *) adjust_appendrel_attrs(root,
+                                                      (Node *) root->parse,
+                                                      1, &appinfo);

+    tlist = preprocess_targetlist(subroot);
+    subroot->processed_tlist = tlist;
+    build_base_rel_tlists(subroot, tlist);

---
0001: line 57-70

In commit message:
  This removes some existing code in inheritance_planner that dealt
  with any subquery RTEs in the query.  The rationale of that code
  was that the subquery RTEs may change during each iteration of
  planning (that is, for different children), so different iterations
  better use different copies of those RTEs.  
  ...
  Since with the new code
  we perform planning just once, I think we don't need this special
  handling.

0001: line 772-782
-     * controlled by a single ModifyTable node.  All the instances share the
-     * same rangetable, but each instance must have its own set of subquery
-     * RTEs within the finished rangetable because (1) they are likely to get
-     * scribbled on during planning, and (2) it's not inconceivable that
-     * subqueries could get planned differently in different cases.  We need
-     * not create duplicate copies of other RTE kinds, in particular not the
-     * target relations, because they don't have either of those issues.  Not
-     * having to duplicate the target relations is important because doing so
-     * (1) would result in a rangetable of length O(N^2) for N targets, with
-     * at least O(N^3) work expended here; and (2) would greatly complicate
-     * management of the rowMarks list.

I used considerable time to confirm there are no needs copying subquery RTEs in
the new code, but so far I couldn't. If copied RTEs are only used when planning,
it might not need to copy RTEs in the new code because we perform planning just
once, so I tried to detect when copied RTEs are used or overwritten, but I gave
up.

Of course, there are comments about this,

-     * same rangetable, but each instance must have its own set of subquery
-     * RTEs within the finished rangetable because (1) they are likely to get
-     * scribbled on during planning, and (2) it's not inconceivable that

so copied RTEs might be used when planning, but I couldn't find the actual codes.
I also checked commits[1, 2] related to these codes. I'll check these for more
time but it would be better there are other's review and I also want a help here.

---
Maybe I checked all the way of the v9 patch excluding the codes about
EquivalenceClass codes(0001: line 567-638).
I'll consider whether there are any performance degration case, but I have
no idea for now. Do you have any points you concerns? If there, I'll check it.


[1] https://github.com/postgres/postgres/commit/b3aaf9081a1a95c245fd605dcf02c91b3a5c3a29
[2] https://github.com/postgres/postgres/commit/c03ad5602f529787968fa3201b35c119bbc6d782

--
Yoshikazu Imai