(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
