Re: non-bulk inserts and tuple routing

On 2018/02/05 19:43, Etsuro Fujita wrote:
> (2018/02/05 14:34), Amit Langote wrote:
>> On 2018/02/02 19:56, Etsuro Fujita wrote:
>>> * In ExecInitPartitionResultRelInfo:
>>> +       /*
>>> +        * Note that the entries in this list appear in no predetermined
>>> +        * order as result of initializing partition result rels as and
>>> when
>>> +        * they're needed.
>>> +        */
>>> +       estate->es_leaf_result_relations =
>>> + lappend(estate->es_leaf_result_relations,
>>> +                                           leaf_part_rri);
>>>
>>> Is it OK to put this within the "if (leaf_part_rri == NULL)" block?
>>
>> Good catch.  I moved it outside the block.  I was under the impression
>> that leaf result relations that were reused from the
>> mtstate->resultRelInfo arrary would have already been added to the list,
>> but it seems they are not.
>
> I commented this because the update-tuple-routing patch has added to the
> list ResultRelInfos reused from the mtstate->resultRelInfo arrary, but on
> reflection I noticed this would cause oddity in reporting execution stats
> for partitions' triggers in EXPLAIN ANALYZE.  Here is an example using the
> head:
>
> postgres=# create table trigger_test (a int, b text) partition by list (a);
> CREATE TABLE
> postgres=# create table trigger_test1 partition of trigger_test for values
> in (1);
> CREATE TABLE
> postgres=# create table trigger_test2 partition of trigger_test for values
> in (2);
> CREATE TABLE
> postgres=# create trigger before_upd_row_trig before update on
> trigger_test1 for
>  each row execute procedure trigger_data (23, 'skidoo');
> CREATE TRIGGER
> postgres=# create trigger before_del_row_trig before delete on
> trigger_test1 for
>  each row execute procedure trigger_data (23, 'skidoo');
> CREATE TRIGGER
> postgres=# insert into trigger_test values (1, 'foo');
> INSERT 0 1
> postgres=# explain analyze update trigger_test set a = 2 where a = 1;
> NOTICE:  before_upd_row_trig(23, skidoo) BEFORE ROW UPDATE ON trigger_test1
> NOTICE:  OLD: (1,foo),NEW: (2,foo)
> NOTICE:  before_del_row_trig(23, skidoo) BEFORE ROW DELETE ON trigger_test1
> NOTICE:  OLD: (1,foo)
>                                                   QUERY PLAN
>
> --------------------------------------------------------------------------------
>
> -------------------------------
>  Update on trigger_test  (cost=0.00..25.88 rows=6 width=42) (actual
> time=2.337..
> 2.337 rows=0 loops=1)
>    Update on trigger_test1
>    ->  Seq Scan on trigger_test1  (cost=0.00..25.88 rows=6 width=42)
> (actual tim
> e=0.009..0.011 rows=1 loops=1)
>          Filter: (a = 1)
>  Planning time: 0.186 ms
>  Trigger before_del_row_trig on trigger_test1: time=0.495 calls=1
>  Trigger before_upd_row_trig on trigger_test1: time=0.870 calls=1
>  Trigger before_del_row_trig on trigger_test1: time=0.495 calls=1
>  Trigger before_upd_row_trig on trigger_test1: time=0.870 calls=1
>  Execution time: 2.396 ms
> (10 rows)
>
> Both trigger stats for the on-update and on-delete triggers are doubly
> shown in the above output.  The reason would be that ExplainPrintTriggers
> called report_triggers twice for trigger_test1's ResultRelInfo: once for
> it from queryDesc->estate->es_result_relations and once for it from
> queryDesc->estate->es_leaf_result_relations.  I don't think this is
> intended behavior, so I think we should fix this.  I think we could
> probably address this by modifying ExecInitPartitionInfo in your patch so
> that it doesn't add to the es_leaf_result_relations list ResultRelInfos
> reused from the mtstate->resultRelInfo arrary, as your previous version of
> the patch.  (ExecGetTriggerResultRel looks at the list too, but it would
> probably work well for this change.)  It might be better to address this
> in another patch, though.

I see. Thanks for the analysis and the explanation.

Seeing as this bug exists in HEAD, as you also seem to be saying, we'd
need to fix it independently of the patches on this thread. I've posted a
patch in another thread titled "update tuple routing and triggers".

Thanks,
Amit