(2018/02/06 11:38), Amit Langote wrote:
On 2018/02/06 10:48, Amit Langote wrote:
When working on this, I wondered if the es_leaf_result_relations should
actually be named something like es_tuple_routing_result_rels, to denote
the fact that they're created by tuple routing code. The current name
might lead to someone thinking that it contains *all* leaf result rels,
but that won't remain true after this patch. Thoughts?
While I'm waiting to hear some opinion on the renaming, I updated the
patch to clarify this in the comment about es_leaf_result_relations.
I'm not sure we really need to rename that. Wouldn't the updated
comment on that list in execnodes.h be enough?
Other comment:
@@ -178,6 +178,13 @@ ExecSetupPartitionTupleRouting(ModifyTableState
*mtstate,
resultRTindex,
rel,
estate->es_instrument);
+
+ /*
+ * Since we're newly creating this ResultRelInfo, add
it to
+ * someplace where explain.c could find them.
+ */
+ estate->es_leaf_result_relations =
+ lappend(estate->es_leaf_result_relations,
leaf_part_rri);
}
I think the above comment would need some more work because that list
will be searched by ExecGetTriggerResultRel also.
Other than that, the patch looks good to me.
Thanks for the patch!
Best regards,
Etsuro Fujita
