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

Hi, Amit!

On Thu, Sept 13, 2018 at 10:29 PM, Amit Langote wrote:
> Attached is what I have at the moment.

I also do the code review of the patch.
I could only see a v3-0001.patch so far, so below are all about v3-0001.patch.

I am new to inheritance/partitioning codes and code review, so my review below might have some mistakes. If there are
mistakes,please point out that kindly :)
 


v3-0001:
1. Is there any reason inheritance_make_rel_from_joinlist returns "parent" that is passed to it? I think we can refer
parentin the caller even if inheritance_make_rel_from_joinlist returns nothing.
 

+static RelOptInfo *
+inheritance_make_rel_from_joinlist(PlannerInfo *root,
+                                   RelOptInfo *parent,
+                                   List *joinlist)
+{
    ...
+    return parent;
+}

2. Is there the possible case that IS_DUMMY_REL(child_joinrel) is true in inheritance_adjust_scanjoin_target()?

+inheritance_adjust_scanjoin_target(PlannerInfo *root,
    ...
+{
    ...
+    foreach(lc, root->inh_target_child_rels)
+    {
        ...
+        /*
+         * If the child was pruned, its corresponding joinrel will be empty,
+         * which we can ignore safely.
+         */
+        if (IS_DUMMY_REL(child_joinrel))
+            continue;

I think inheritance_make_rel_from_joinlist() doesn't make dummy joinrel for the child that was pruned.

+inheritance_make_rel_from_joinlist(PlannerInfo *root,
...
+{
    ...
+    foreach(lc, root->append_rel_list)
+    {
+        RelOptInfo *childrel;
        ...
+        /* Ignore excluded/pruned children. */
+        if (IS_DUMMY_REL(childrel))
+            continue;
        ...
+        /*
+         * Save child joinrel to be processed later in
+         * inheritance_adjust_scanjoin_target, which adjusts its paths to
+         * be able to emit expressions in query's top-level target list.
+         */
+        root->inh_target_child_rels = lappend(root->inh_target_child_rels,
+                                              childrel);
+    }
+}

3.
Is the "root->parse->commandType != CMD_INSERT" required in:

@@ -2018,13 +1514,45 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
...
+        /*
+         * For UPDATE/DELETE on an inheritance parent, we must generate and
+         * apply scanjoin target based on targetlist computed using each
+         * of the child relations.
+         */
+        if (parse->commandType != CMD_INSERT &&
+            current_rel->reloptkind == RELOPT_BASEREL &&
+            current_rel->relid == root->parse->resultRelation &&
+            root->simple_rte_array[current_rel->relid]->inh)
...

@@ -2137,92 +1665,123 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
     final_rel->fdwroutine = current_rel->fdwroutine;
 
...
-    foreach(lc, current_rel->pathlist)
+    if (current_rel->reloptkind == RELOPT_BASEREL &&
+        current_rel->relid == root->parse->resultRelation &&
+        !IS_DUMMY_REL(current_rel) &&
+        root->simple_rte_array[current_rel->relid]->inh &&
+        parse->commandType != CMD_INSERT)


I think if a condition would be "current_rel->relid == root->parse->resultRelation && parse->commandType != CMD_INSERT"
atthe above if clause, elog() is called in the query_planner and planner don't reach above if clause.
 
Of course there is the case that query_planner returns the dummy joinrel and elog is not called, but in that case,
current_rel->relidmay be 0(current_rel is dummy joinrel) and root->parse->resultRelation may be not 0(a query is
INSERT).

4. Can't we use define value(IS_PARTITIONED or something like IS_INHERITANCE?) to identify inheritance and partitioned
tablein below code? It was little confusing to me that which code is related to inheritance/partitioned when looking
codes.

In make_one_rel():
+    if (root->parse->resultRelation &&
+        root->simple_rte_array[root->parse->resultRelation]->inh)
+    {
...
+        rel = inheritance_make_rel_from_joinlist(root, targetrel, joinlist);

In inheritance_make_rel_from_joinlist():
+        if (childrel->part_scheme != NULL)
+            childrel =
+                inheritance_make_rel_from_joinlist(root, childrel,
+                                                   translated_joinlist);


I can't review inheritance_adjust_scanjoin_target() deeply, because it is difficult to me to understand fully codes
aboutjoin processing.
 

--
Yoshikazu Imai