Thanks for the review.
On 2018/02/08 0:04, Robert Haas wrote:
> On Tue, Feb 6, 2018 at 12:52 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
>> About renaming es_leaf_result_relations to
>> es_tuple_routing_result_relations, I will defer that to committer. But on
>> second though, maybe we don't need to make this patch larger than it has
>> to be.
>
> +1 for renaming it. I like keeping the patch small, but not at the
> price of being misleading.
OK, done in the attached.
> + /*
> + * Since we're newly creating this ResultRelInfo, add it to
> + * someplace where others could find it.
> + */
>
> How about: "Since we've just initialized this ResultRelInfo, it's not
> in any list attached to the estate as yet. Add it, so that it can be
> found later."
Wrote that way.
Thanks,
Amit
From 146f4d64fc9c085d7bc26435613eddd96d972d0d Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 6 Feb 2018 10:40:45 +0900
Subject: [PATCH v4] Fix trigger behavior with update tuple routing
Trigger stats shown by EXPLAIN ANALYZE may contain duplicate
entries, because leaf partitions' ResultRelInfo is redundantly
present in multiple EState lists.
While at it, rename es_leaf_result_relations to something that
more accurately describes what it is.
Reported by: Etsuro Fujita
---
src/backend/commands/explain.c | 3 ++-
src/backend/executor/execMain.c | 7 +++++--
src/backend/executor/execPartition.c | 12 +++++++++---
src/backend/executor/execUtils.c | 2 +-
src/include/nodes/execnodes.h | 7 +++++--
5 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 41cd47e8bc..3339a8a0fc 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -652,7 +652,8 @@ ExplainPrintTriggers(ExplainState *es, QueryDesc *queryDesc)
bool show_relname;
int numrels =
queryDesc->estate->es_num_result_relations;
int numrootrels =
queryDesc->estate->es_num_root_result_relations;
- List *leafrels = queryDesc->estate->es_leaf_result_relations;
+ List *leafrels =
+
queryDesc->estate->es_tuple_routing_result_relations;
List *targrels = queryDesc->estate->es_trig_target_relations;
int nr;
ListCell *l;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 410921cc40..5d3e923cca 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1413,8 +1413,11 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
rInfo++;
nr--;
}
- /* Third, search through the leaf result relations, if any */
- foreach(l, estate->es_leaf_result_relations)
+ /*
+ * Third, search through the result relations that were created during
+ * tuple routing, if any.
+ */
+ foreach(l, estate->es_tuple_routing_result_relations)
{
rInfo = (ResultRelInfo *) lfirst(l);
if (RelationGetRelid(rInfo->ri_RelationDesc) == relid)
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index ba6b52c32c..4048c3ebc6 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -178,6 +178,15 @@ ExecSetupPartitionTupleRouting(ModifyTableState *mtstate,
resultRTindex,
rel,
estate->es_instrument);
+
+ /*
+ * Since we've just initialized this ResultRelInfo,
it's not in
+ * any list attached to the estate as yet. Add it, so
that it can
+ * be found later.
+ */
+ estate->es_tuple_routing_result_relations =
+
lappend(estate->es_tuple_routing_result_relations,
+ leaf_part_rri);
}
part_tupdesc = RelationGetDescr(partrel);
@@ -210,9 +219,6 @@ ExecSetupPartitionTupleRouting(ModifyTableState *mtstate,
mtstate != NULL &&
mtstate->mt_onconflict
!= ONCONFLICT_NONE);
- estate->es_leaf_result_relations =
- lappend(estate->es_leaf_result_relations,
leaf_part_rri);
-
proute->partitions[i] = leaf_part_rri;
i++;
}
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index e29f7aaf7b..50b6edce63 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -119,7 +119,7 @@ CreateExecutorState(void)
estate->es_root_result_relations = NULL;
estate->es_num_root_result_relations = 0;
- estate->es_leaf_result_relations = NIL;
+ estate->es_tuple_routing_result_relations = NIL;
estate->es_trig_target_relations = NIL;
estate->es_trig_tuple_slot = NULL;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 54ce63f147..c1c9c0c360 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -466,8 +466,11 @@ typedef struct EState
ResultRelInfo *es_root_result_relations; /* array of
ResultRelInfos */
int es_num_root_result_relations; /* length of
the array */
- /* Info about leaf partitions of partitioned table(s) for insert
queries: */
- List *es_leaf_result_relations; /* List of ResultRelInfos */
+ /*
+ * The following list contains ResultRelInfo's created by the tuple
+ * routing code for partitions that don't already have one.
+ */
+ List *es_tuple_routing_result_relations;
/* Stuff used for firing triggers: */
List *es_trig_target_relations; /* trigger-only ResultRelInfos
*/
--
2.11.0
