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