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

Hi,

On Thu, Oct 25, 2018 at 10:38 PM, Amit Langote wrote:
> And here is the latest set of patches.  Sorry it took me a while.

Thanks for revising the patch!

> I didn't get time today to repeat the performance tests, but I'm planning
> to next week.  In the meantime, please feel free to test them and provide
> comments on the code changes.

Since it takes me a lot of time to see all of the patches, I will post comments
little by little from easy parts like typo check.


1.
0002:
+ * inheritance_make_rel_from_joinlist
+ *        Perform join planning for all non-dummy leaf inheritance chilren
+ *        in their role as query's target relation

"inheritance chilren" -> "inheritance children"


2.
0002:
+        /*
+         * Sub-partitions have to be processed recursively, because
+         * AppendRelInfoss link sub-partitions to their immediate parents, not
+         * the root partitioned table.
+         */

AppendRelInfoss -> AppendRelInfos (?)


3.
0002:
+        /* Reset interal join planning data structures. */

interal -> internal


4.
0004:
-static void expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte,
-                         Index rti);

Comments referring to expand_inherited_rtentry() is left.

backend/optimizer/plan/planner.c:1310:
           * Because of the way expand_inherited_rtentry works, that should be
backend/optimizer/plan/planner.c:1317:
           * Instead the duplicate child RTE created by expand_inherited_rtentry
backend/optimizer/util/plancat.c:118:
    * the rewriter or when expand_inherited_rtentry() added it to the query's
backend/optimizer/util/plancat.c:640:
    * the rewriter or when expand_inherited_rtentry() added it to the query's

About the last two comments in the above,
"the rewriter or when expand_inherited_rtentry() added it to the query's"
would be
"the rewriter or when add_inheritance_child_rel() added it to the query's".

I don't know how to correct the first two comments.


5.
0004:
-static void expand_partitioned_rtentry(PlannerInfo *root,
-                           RangeTblEntry *parentrte,
-                           Index parentRTindex, Relation parentrel,
-                           PlanRowMark *top_parentrc, LOCKMODE lockmode,
-                           List **appinfos);

Comments referring to expand_partitioned_rtentry() is also left.

backend/executor/execPartition.c:941:
 /*
  * get_partition_dispatch_recurse
  *      Recursively expand partition tree rooted at rel
  *
  * As the partition tree is expanded in a depth-first manner, we maintain two
  * global lists: of PartitionDispatch objects corresponding to partitioned
  * tables in *pds and of the leaf partition OIDs in *leaf_part_oids.
  *
  * Note that the order of OIDs of leaf partitions in leaf_part_oids matches
  * the order in which the planner's expand_partitioned_rtentry() processes
  * them.  It's not necessarily the case that the offsets match up exactly,
  * because constraint exclusion might prune away some partitions on the
  * planner side, whereas we'll always have the complete list; but unpruned
  * partitions will appear in the same order in the plan as they are returned
  * here.
  */

I think the second paragraph of the comments is no longer correct.
expand_partitioned_rtentry() expands the partition tree in a depth-first
manner, whereas expand_append_rel() doesn't neither in a depth-first manner
nor a breadth-first manner as below.

partitioned table tree image:
pt
  sub1
    sub1_1
      leaf0
    leaf1
  sub2
    leaf2
  leaf3

append_rel_list(expanded by expand_partitioned_rtentry):
  [sub1, sub1_1, leaf0, leaf1, sub2, leaf2, leaf3]

append_rel_list(expanded by expand_append_rel):
  [sub1, sub2, leaf3, sub1_1, leaf1, leaf0, leaf2]


6.
0004
+    /*
+     * A partitioned child table with 0 children is a dummy rel, so don't
+     * bother creating planner objects for it.
+     */
+    if (childrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+    {
+        PartitionDesc partdesc = RelationGetPartitionDesc(childrel);
+
+        Assert(!RELATION_IS_OTHER_TEMP(childrel));
+        if (partdesc->nparts == 0)
+        {
+            heap_close(childrel, NoLock);
+            return NULL;
+        }
+    }
+
+    /*
+     * If childrel doesn't belong to this session, skip it, also relinquishing
+     * the lock.
+     */
+    if (RELATION_IS_OTHER_TEMP(childrel))
+    {
+        heap_close(childrel, lockmode);
+        return NULL;
+    }

If we process the latter if block before the former one, Assert can be excluded
from the code.



It might be difficult to me to examine the codes whether there exists any logical
wrongness along with significant planner code changes, but I will try to look it.
Since it passes make check-world successfully, I think as of now there might be
not any wrong points.

If there's time, I will also do the performance tests.


--
Yoshikazu Imai