RE: speeding up planning with partitions

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 parent in 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" at the 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->relid may 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 table in 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 about join processing.

--
Yoshikazu Imai