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

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:

Reply via email to