On 2017/05/17 17:54, Ildus Kurbangaliev wrote:
On Wed, 17 May 2017 15:28:24 +0900
Michael Paquier <michael.paqu...@gmail.com> wrote:
On Tue, May 16, 2017 at 11:26 PM, Ildus Kurbangaliev
<i.kurbangal...@postgrespro.ru> wrote:
On Tue, 16 May 2017 21:36:11 +0900
Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote:
On 2017/05/16 21:11, Ashutosh Bapat wrote:
On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
<i.kurbangal...@postgrespro.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
