Обсуждение: update tuple routing and triggers

Hi.

Fujita-san pointed out in a nearby thread [1] that EXPLAIN ANALYZE shows
duplicate stats for partitions' triggers.

Example:

create table p (a int) partition by list (a);
create table p1 partition of p for values in (1);
create table p2 partition of p for values in (2);
create table p3 partition of p for values in (3);

create trigger show_data before update on p1 for each row execute
procedure trig_notice_func();
create trigger show_data before update on p2 for each row execute
procedure trig_notice_func();

insert into p values (1), (2);

explain (analyze, costs off, timing off) update p set a = a + 1;
NOTICE:  OLD: (1); NEW: (2)
NOTICE:  OLD: (2); NEW: (3)
                  QUERY PLAN
----------------------------------------------
 Update on p (actual rows=0 loops=1)
   Update on p1
   Update on p2
   Update on p3
   ->  Seq Scan on p1 (actual rows=1 loops=1)
   ->  Seq Scan on p2 (actual rows=1 loops=1)
   ->  Seq Scan on p3 (actual rows=0 loops=1)
 Planning time: 2.000 ms
 Trigger show_data on p1: calls=1
 Trigger show_data on p2: calls=1
 Trigger show_data on p1: calls=1
 Trigger show_data on p2: calls=1
 Execution time: 4.228 ms
(13 rows)

See that the information about the trigger show_data is shown twice for
partitions p1 and p2.  That happens because ExplainPrintTriggers() goes
through both es_result_relations and es_leaf_result_relations to show the
trigger information.  As Fujita-san pointed out in the linked email,
ExecSetupPartitionTupleRouting() adds a partition ResultRelInfo to
es_leaf_result_relations even if it may already have been present in
es_result_relations, which happens if a ResultRelInfo is reused in the
case of update tuple routing.

Attached is a patch to fix that.  After the patch:

explain (analyze, costs off, timing off) update p set a = a + 1;
NOTICE:  OLD: (1); NEW: (2)
NOTICE:  OLD: (2); NEW: (3)
                  QUERY PLAN
----------------------------------------------
 Update on p (actual rows=0 loops=1)
   Update on p1
   Update on p2
   Update on p3
   ->  Seq Scan on p1 (actual rows=1 loops=1)
   ->  Seq Scan on p2 (actual rows=1 loops=1)
   ->  Seq Scan on p3 (actual rows=0 loops=1)
 Planning time: 0.627 ms
 Trigger show_data on p1: calls=1
 Trigger show_data on p2: calls=1
 Execution time: 1.443 ms
(11 rows)

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?

Thanks,
Amit

[1] https://www.postgresql.org/message-id/5A783549.4020409%40lab.ntt.co.jp

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

Thanks for the patch Amit.

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.

So what the patch does, looks good to me. One minor thing :

+ * Since we're newly creating this ResultRelInfo, add it to
+ * someplace where explain.c could find them.

I think above, instead of keeping the comment specifically for
explain.c, we should make it more general. The list is also used by
ExecGetTriggerResultRel(). (There, this duplicate relation issue does
not come up because it only uses the first one out of them).

I think the name es_tuple_routing_result_rels that you chose, sounds good.

Thanks
-Amit


On 6 February 2018 at 08:08, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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.
>
> Thanks,
> Amit



-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company

(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

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

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

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

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

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_f8@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

On Wed, Feb 7, 2018 at 8:26 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> OK, done in the attached.

Committed.  Thanks.

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

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

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

Regards,
Amit