Re: Bug in ExecModifyTable function and trigger issues for foreign tables

On 2017/05/17 17:54, Ildus Kurbangaliev wrote:
> On Wed, 17 May 2017 15:28:24 +0900
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>
>> On Tue, May 16, 2017 at 11:26 PM, Ildus Kurbangaliev
>> <i(dot)kurbangaliev(at)postgrespro(dot)ru> wrote:
>>> On Tue, 16 May 2017 21:36:11 +0900
>>> Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>> On 2017/05/16 21:11, Ashutosh Bapat wrote:
>>>>> On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
>>>>> <i(dot)kurbangaliev(at)postgrespro(dot)ru> wrote:
>>>>
>>>>>> I agree. Maybe this issue should be added to Postgresql Open
>>>>>> Items? I think there should be some complex solution that fixes
>>>>>> not only triggers for foreign tables at table partitioning, but
>>>>>> covers other possible not working cases.
>>>>
>>>>> I doubt if this is an open item, since DMLs on foreign tables are
>>>>> supported since 9.3 and support to add foreign tables to
>>>>> inheritance was added back in 9.5.
>>>>
>>>> I think this issue was introduced by the latter, so that was my
>>>> fault.
>>>>
>>>> One approach I came up with to fix this issue is to rewrite the
>>>> targetList entries of an inherited UPDATE/DELETE properly using
>>>> rewriteTargetListUD, when generating a plan for each child table in
>>>> inheritance_planner. Attached is a WIP patch for that. Maybe I am
>>>> missing something, though.
>>
>> Could this patch include some regression tests to see at what extent
>> it has been tested? We surely don't want to see that broken again in
>> the future as well. (Nit: I did not look at the patch in details yet)

OK, I'll include regression tests in the next version of the patch.

>>> I tested the patch, looks good.
>>
>> What kind of tests did you do?
>
> I tested update triggers for foreign table when regular table is a
> parent and foreign table is a child. Case like this:
>
> explain verbose update parent set val = random();
> QUERY PLAN
> -------------------------------------------------------------------------------
> Update on public.parent (cost=0.00..258.63 rows=5535 width=10)
> Update on public.parent
> Update on public.child1
> Foreign Update on public.ftable // we have triggers on ftable here
>
>>
>> junkfilter = resultRelInfo->ri_junkFilter;
>> + tupleid = NULL;
>> estate->es_result_relation_info = resultRelInfo;
>> Er, what is that?
>
> That fixes the bug when tupleid from regular tuple is used to get
> oldtuple for triggers of foreign table.

That's right. Let me explain in more detail. Currently, tupleid is
only initialized at the top of ExecModifyTable, so if we just loop
within the for(;;) code in that function (without returning RETURNING to
caller), tupleid won't be initialized even when advancing to next
subplan in case of inherited UPDATE/DELETE. This would cause a problem.
Assume that the current subplan is for the parent, which is a plain
table, that the next subplan is for the child, which is a foreign table,
and that the foreign table has a BEFORE trigger, as tested by Ildus. In
that case the current subplan would set tupleid to ctid for each row
from the plain table, and after advancing to the next subplan, the
subplan would set oldtuple to wholerow for the first row from the
foreign table, *without initializing tupleid to NULL*, and then call
ExecBRUpdateTriggers/ExecBRDeleteTriggers during ExecUpdate/ExecDelete,
which would cause an assertion error for
Assert(HeapTupleIsValid(fdw_trigtuple) ^ ItemPointerIsValid(tupleid)) in
those trigger functions, because oldtuple and tupleid are both valid.
So, tupleid should be initialized at least when advancing to next
subplan. It might be better to initialize that at each iteration of the
for(;;) code, like oldtuple, though.

Best regards,
Etsuro Fujita