Re: update tuple routing and triggers

[Amit Langote]

On 2018/02/09 4:31, Robert Haas wrote:
> On Wed, Feb 7, 2018 at 8:26 PM, Amit Langote
>  wrote:
>> OK, done in the attached.
> 
> Committed.  Thanks.

Thank you.  Sorry, missed renaming leafrel that you did yourself.

Regards,
Amit

Re: update tuple routing and triggers

[Robert Haas]

On Wed, Feb 7, 2018 at 8:26 PM, Amit Langote
 wrote:
> OK, done in the attached.

Committed.  Thanks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: update tuple routing and triggers

[Amit Langote]

Thanks for the review.

On 2018/02/08 0:04, Robert Haas wrote:
> On Tue, Feb 6, 2018 at 12:52 AM, Amit Langote
>  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 
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)
boolshow_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
--- 

Re: update tuple routing and triggers

[Robert Haas]

On Tue, Feb 6, 2018 at 12:52 AM, Amit Langote
 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.

+/*
+ * 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."

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: update tuple routing and triggers

[Amit Langote]

On 2018/02/06 13:56, Amit Khandekar wrote:
> I was wondering whether the same duplicate result rels issue can arise
> for es_root_result_relations and es_result_relations. But I think for
> inserts, es_root_result_relations is NULL, and for updates, these two
> lists always have distinct set of relations. So this issue might not
> be there for these two lists.

Yeah, we don't need to worry about es_root_result_relations.

Thanks,
Amit

Re: update tuple routing and triggers

[Amit Langote]

Thank you both for the review.

I updated the comment in ExecSetupPartitionTupleRouting considering the
point both of you raised.

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.

Thanks,
Amit
From d001de96ab452e48b011db7f34840a3e6b8999c9 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 6 Feb 2018 10:40:45 +0900
Subject: [PATCH v3] 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.

Reported by: Etsuro Fujita
---
 src/backend/executor/execPartition.c | 10 +++---
 src/include/nodes/execnodes.h|  5 -
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/backend/executor/execPartition.c 
b/src/backend/executor/execPartition.c
index 106a96d910..8094dbc614 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -178,6 +178,13 @@ ExecSetupPartitionTupleRouting(ModifyTableState *mtstate,
  resultRTindex,
  rel,
  
estate->es_instrument);
+
+   /*
+* Since we're newly creating this ResultRelInfo, add 
it to
+* someplace where others could find it.
+*/
+   estate->es_leaf_result_relations =
+   lappend(estate->es_leaf_result_relations, 
leaf_part_rri);
}
 
part_tupdesc = RelationGetDescr(partrel);
@@ -210,9 +217,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/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index a2a2a9f3d4..1915b53b2f 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -466,7 +466,10 @@ 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: */
+   /*
+* The following list contains ResultRelInfo's created by the tuple
+* routing code for partitions that don't already have one.
+*/
List   *es_leaf_result_relations;   /* List of ResultRelInfos */
 
/* Stuff used for firing triggers: */
-- 
2.11.0

Re: update tuple routing and triggers

[Etsuro Fujita]

(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

Re: update tuple routing and triggers

[Amit Langote]

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.

Thanks,
Amit
From 2dca4abcf584f8239b8c47a9a3b96be5585bce06 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 6 Feb 2018 10:40:45 +0900
Subject: [PATCH v2] 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.

Reported by: Etsuro Fujita
---
 src/backend/executor/execPartition.c | 10 +++---
 src/include/nodes/execnodes.h|  5 -
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/backend/executor/execPartition.c 
b/src/backend/executor/execPartition.c
index 106a96d910..619a8d7a99 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -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);
}
 
part_tupdesc = RelationGetDescr(partrel);
@@ -210,9 +217,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/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index a2a2a9f3d4..1915b53b2f 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -466,7 +466,10 @@ 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: */
+   /*
+* The following list contains ResultRelInfo's created by the tuple
+* routing code for partitions that don't already have one.
+*/
List   *es_leaf_result_relations;   /* List of ResultRelInfos */
 
/* Stuff used for firing triggers: */
-- 
2.11.0