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
