Bug in ExecModifyTable function and trigger issues for foreign tables
Hello hackers,
i was experimenting with fdw tables recently, and discovered two bugs
in postgres core code (tested on stable 9.6 and master).
Steps to reproduce:
1) create parent table
2) create child local table
3) create child foreign table
4) create 'before row update` trigger at foreign table
5) make update query on parent table.
I attached sql file with these steps. At the end postgres will show an
error like:
ERROR: could not open file "base/12410/33037": No such file or
directory
33037 is relid of the foreign table. Bug is related with the fact that
postgres will try use latest scanned tupleid from local table to try
get an old tuple for trigger of foreign table.
It should be fixed with the patch like:
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1324,7 +1324,6 @@ ExecModifyTable(ModifyTableState *node)
JunkFilter *junkfilter;
TupleTableSlot *slot;
TupleTableSlot *planSlot;
- ItemPointer tupleid = NULL;
ItemPointerData tuple_ctid;
HeapTupleData oldtupdata;
HeapTuple oldtuple;
@@ -1381,6 +1380,8 @@ ExecModifyTable(ModifyTableState *node)
*/
for (;;)
{
+ ItemPointer tupleid = NULL;
+After this patch the second bug will appear:
TRAP: FailedAssertion("!(((const void*)(fdw_trigtuple) != ((void *)0))
^ ((bool) (((const void*)(tupleid) != ((void *)0)) &&
((tupleid)->ip_posid != 0))))", File: "trigger.c", Line: 2428)
I'm not sure how it should be fixed, because as I see `oldtuple` will
be set only for AFTER ROW triggers by `wholerow` junk attribute.
Regards,
Ildus Kurbangaliev
Attachments:
test.sqlapplication/sqlDownload
On Sun, May 14, 2017 at 5:35 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:
TRAP: FailedAssertion("!(((const void*)(fdw_trigtuple) != ((void *)0))
^ ((bool) (((const void*)(tupleid) != ((void *)0)) &&
((tupleid)->ip_posid != 0))))", File: "trigger.c", Line: 2428)I'm not sure how it should be fixed, because as I see `oldtuple` will
be set only for AFTER ROW triggers by `wholerow` junk attribute.
There is comment on the ExecUpdate function
* When updating a foreign table,
* tupleid is invalid; the FDW has to figure out which row to
* update using data from the planSlot. oldtuple is passed to
* foreign table triggers; it is NULL when the foreign table has
* no relevant triggers.
After your fix, now tupleid is invalid which is expected, but seems
like we need to do something more. As per the comments seems like it
is expected to get the oldtuple from planSlot. But I don't see any
code for handling that part.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
After your fix, now tupleid is invalid which is expected, but seems
like we need to do something more. As per the comments seems like it
is expected to get the oldtuple from planSlot. But I don't see any
code for handling that part.
Maybe we should do something like attached patch.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
fix_fdw_trigger.patchapplication/octet-stream; name=fix_fdw_trigger.patchDownload+5-1
On Mon, May 15, 2017 at 2:04 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
After your fix, now tupleid is invalid which is expected, but seems
like we need to do something more. As per the comments seems like it
is expected to get the oldtuple from planSlot. But I don't see any
code for handling that part.Maybe we should do something like attached patch.
As a deficiency, shouldn't this try as well to improve regression test
coverage for FDW triggers with inheritance trees? Those tests are in
postgres_fdw. You may find other issues on the way..
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, 15 May 2017 10:34:58 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar <dilipbalaut@gmail.com>
wrote:After your fix, now tupleid is invalid which is expected, but seems
like we need to do something more. As per the comments seems like it
is expected to get the oldtuple from planSlot. But I don't see any
code for handling that part.Maybe we should do something like attached patch.
Hi,
planSlot contains already projected tuple, you can't use it as oldtuple.
I think problem is that `rewriteTargetListUD` called only for parent
relation, so there is no `wholerow` attribute for foreign tables.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:
Hi,
planSlot contains already projected tuple, you can't use it as oldtuple.
I think problem is that `rewriteTargetListUD` called only for parent
relation, so there is no `wholerow` attribute for foreign tables.
Oh, right!
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:
On Mon, 15 May 2017 10:34:58 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar <dilipbalaut@gmail.com>
wrote:After your fix, now tupleid is invalid which is expected, but seems
like we need to do something more. As per the comments seems like it
is expected to get the oldtuple from planSlot. But I don't see any
code for handling that part.Maybe we should do something like attached patch.
Hi,
planSlot contains already projected tuple, you can't use it as oldtuple.
I think problem is that `rewriteTargetListUD` called only for parent
relation, so there is no `wholerow` attribute for foreign tables.
Yes. postgresAddForeignUpdateTargets() which is called by
rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
for postgres_fdw but for other wrappers it might be a bad news. ctid,
whole row obtained from the remote postgres server will fit the tuple
descriptor of parent, but for other FDWs the column injected by
rewriteTargetListUD() may make the child tuple look different from
that of the parent, so we may not pass that column down to the child.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 15, 2017 at 5:43 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
Yes. postgresAddForeignUpdateTargets() which is called by
rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
for postgres_fdw but for other wrappers it might be a bad news. ctid,
whole row obtained from the remote postgres server will fit the tuple
descriptor of parent, but for other FDWs the column injected by
rewriteTargetListUD() may make the child tuple look different from
that of the parent, so we may not pass that column down to the child.
But, can't we call rewriteTargetListUD for all the inheritors if the
inheritor is a foreign table which will intern call the
postgresAddForeignUpdateTargets?
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 15, 2017 at 6:04 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Mon, May 15, 2017 at 5:43 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:Yes. postgresAddForeignUpdateTargets() which is called by
rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
for postgres_fdw but for other wrappers it might be a bad news. ctid,
whole row obtained from the remote postgres server will fit the tuple
descriptor of parent, but for other FDWs the column injected by
rewriteTargetListUD() may make the child tuple look different from
that of the parent, so we may not pass that column down to the child.But, can't we call rewriteTargetListUD for all the inheritors if the
inheritor is a foreign table which will intern call the
postgresAddForeignUpdateTargets?
Yes. But it's not necessary to have all children use same FDW.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, 15 May 2017 17:43:52 +0530
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote:
On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:On Mon, 15 May 2017 10:34:58 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar
<dilipbalaut@gmail.com> wrote:After your fix, now tupleid is invalid which is expected, but
seems like we need to do something more. As per the comments
seems like it is expected to get the oldtuple from planSlot.
But I don't see any code for handling that part.Maybe we should do something like attached patch.
Hi,
planSlot contains already projected tuple, you can't use it as
oldtuple. I think problem is that `rewriteTargetListUD` called only
for parent relation, so there is no `wholerow` attribute for
foreign tables.Yes. postgresAddForeignUpdateTargets() which is called by
rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
for postgres_fdw but for other wrappers it might be a bad news. ctid,
whole row obtained from the remote postgres server will fit the tuple
descriptor of parent, but for other FDWs the column injected by
rewriteTargetListUD() may make the child tuple look different from
that of the parent, so we may not pass that column down to the child.
I'm trying to say that when we have a regular table as parent, and
foreign table as child, in rewriteTargetListUD `wholerow` won't be
added, because rewriteTargetListUD will be called only for parent
relation. You can see that by running the script i provided in the first
message of this thread.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 15, 2017 at 7:24 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:
On Mon, 15 May 2017 17:43:52 +0530
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote:On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:On Mon, 15 May 2017 10:34:58 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar
<dilipbalaut@gmail.com> wrote:After your fix, now tupleid is invalid which is expected, but
seems like we need to do something more. As per the comments
seems like it is expected to get the oldtuple from planSlot.
But I don't see any code for handling that part.Maybe we should do something like attached patch.
Hi,
planSlot contains already projected tuple, you can't use it as
oldtuple. I think problem is that `rewriteTargetListUD` called only
for parent relation, so there is no `wholerow` attribute for
foreign tables.Yes. postgresAddForeignUpdateTargets() which is called by
rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
for postgres_fdw but for other wrappers it might be a bad news. ctid,
whole row obtained from the remote postgres server will fit the tuple
descriptor of parent, but for other FDWs the column injected by
rewriteTargetListUD() may make the child tuple look different from
that of the parent, so we may not pass that column down to the child.I'm trying to say that when we have a regular table as parent, and
foreign table as child, in rewriteTargetListUD `wholerow` won't be
added, because rewriteTargetListUD will be called only for parent
relation. You can see that by running the script i provided in the first
message of this thread.
You are right.
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
Remote SQL: UPDATE public.child1 SET val = $2 WHERE ctid = $1
-> Seq Scan on public.parent (cost=0.00..4.83 rows=255 width=10)
Output: random(), parent.ctid
-> Seq Scan on public.child1 (cost=0.00..48.25 rows=2550 width=10)
Output: random(), child1.ctid
-> Foreign Scan on public.ftable (cost=100.00..205.55 rows=2730 width=10)
Output: random(), ftable.ctid
Remote SQL: SELECT ctid FROM public.child1 FOR UPDATE
(12 rows)
explain verbose update ftable set val = random();
QUERY PLAN
-------------------------------------------------------------------------------
Update on public.ftable (cost=100.00..159.42 rows=1412 width=38)
Remote SQL: UPDATE public.child1 SET val = $2 WHERE ctid = $1
-> Foreign Scan on public.ftable (cost=100.00..159.42 rows=1412 width=38)
Output: random(), ctid, ftable.*
Remote SQL: SELECT val, ctid FROM public.child1 FOR UPDATE
(5 rows)
Anyway, the problem I am stating, i.e. we have a bigger problem to fix
when there are FDWs other than postgres_fdw involved seems to be still
valid.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, 16 May 2017 15:21:27 +0530
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote:
On Mon, May 15, 2017 at 7:24 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:On Mon, 15 May 2017 17:43:52 +0530
Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote:On Mon, May 15, 2017 at 2:46 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:On Mon, 15 May 2017 10:34:58 +0530
Dilip Kumar <dilipbalaut@gmail.com> wrote:On Sun, May 14, 2017 at 9:54 PM, Dilip Kumar
<dilipbalaut@gmail.com> wrote:After your fix, now tupleid is invalid which is expected, but
seems like we need to do something more. As per the comments
seems like it is expected to get the oldtuple from planSlot.
But I don't see any code for handling that part.Maybe we should do something like attached patch.
Hi,
planSlot contains already projected tuple, you can't use it as
oldtuple. I think problem is that `rewriteTargetListUD` called
only for parent relation, so there is no `wholerow` attribute for
foreign tables.Yes. postgresAddForeignUpdateTargets() which is called by
rewriteTargetListUD injects "ctid". "wholerow" is always there. Not
for postgres_fdw but for other wrappers it might be a bad news.
ctid, whole row obtained from the remote postgres server will fit
the tuple descriptor of parent, but for other FDWs the column
injected by rewriteTargetListUD() may make the child tuple look
different from that of the parent, so we may not pass that column
down to the child.I'm trying to say that when we have a regular table as parent, and
foreign table as child, in rewriteTargetListUD `wholerow` won't be
added, because rewriteTargetListUD will be called only for parent
relation. You can see that by running the script i provided in the
first message of this thread.You are right.
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
Remote SQL: UPDATE public.child1 SET val = $2 WHERE ctid = $1
-> Seq Scan on public.parent (cost=0.00..4.83 rows=255
width=10) Output: random(), parent.ctid
-> Seq Scan on public.child1 (cost=0.00..48.25 rows=2550
width=10) Output: random(), child1.ctid
-> Foreign Scan on public.ftable (cost=100.00..205.55 rows=2730
width=10) Output: random(), ftable.ctid
Remote SQL: SELECT ctid FROM public.child1 FOR UPDATE
(12 rows)explain verbose update ftable set val = random();
QUERY PLAN
-------------------------------------------------------------------------------
Update on public.ftable (cost=100.00..159.42 rows=1412 width=38)
Remote SQL: UPDATE public.child1 SET val = $2 WHERE ctid = $1
-> Foreign Scan on public.ftable (cost=100.00..159.42 rows=1412
width=38) Output: random(), ctid, ftable.*
Remote SQL: SELECT val, ctid FROM public.child1 FOR UPDATE
(5 rows)Anyway, the problem I am stating, i.e. we have a bigger problem to fix
when there are FDWs other than postgres_fdw involved seems to be still
valid.
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.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
<i.kurbangaliev@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.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/05/16 21:11, Ashutosh Bapat wrote:
On Tue, May 16, 2017 at 5:35 PM, Ildus Kurbangaliev
<i.kurbangaliev@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.
Best regards,
Etsuro Fujita
Attachments:
inherited-foreign-modify-WIP.patchapplication/x-patch; name=inherited-foreign-modify-WIP.patchDownload+57-4
On Tue, 16 May 2017 21:36:11 +0900
Etsuro Fujita <fujita.etsuro@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.kurbangaliev@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.Best regards,
Etsuro Fujita
I tested the patch, looks good.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, May 16, 2017 at 11:26 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:
On Tue, 16 May 2017 21:36:11 +0900
Etsuro Fujita <fujita.etsuro@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.kurbangaliev@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)
I tested the patch, looks good.
What kind of tests did you do?
junkfilter = resultRelInfo->ri_junkFilter;
+ tupleid = NULL;
estate->es_result_relation_info = resultRelInfo;
Er, what is that?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, 17 May 2017 15:28:24 +0900
Michael Paquier <michael.paquier@gmail.com> wrote:
On Tue, May 16, 2017 at 11:26 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:On Tue, 16 May 2017 21:36:11 +0900
Etsuro Fujita <fujita.etsuro@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.kurbangaliev@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)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.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/05/17 17:54, Ildus Kurbangaliev wrote:
On Wed, 17 May 2017 15:28:24 +0900
Michael Paquier <michael.paquier@gmail.com> wrote:On Tue, May 16, 2017 at 11:26 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:On Tue, 16 May 2017 21:36:11 +0900
Etsuro Fujita <fujita.etsuro@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.kurbangaliev@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 herejunkfilter = 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
On 2017/05/16 21:36, Etsuro Fujita wrote:
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.
While updating the patch, I noticed the patch rewrites the UPDATE
targetList incorrectly in some cases; rewrite_inherited_tlist I added to
adjust_appendrel_attrs (1) removes all junk items from the targetList
and (2) adds junk items for the child table using rewriteTargetListUD,
but it's wrong to drop all junk items in cases where there are junk
items for some other reasons than rewriteTargetListUD. Consider junk
items containing MULTIEXPR SubLink. One way I came up with to fix this
is to change (1) to only remove junk items with resname; since junk
items added by rewriteTargetListUD should have resname (note: we would
need resname to call ExecFindJunkAttributeInTlist at execution time!)
while other junk items wouldn't have resname (see
transformUpdateTargetList), we could correctly replace junk items added
by rewriteTargetListUD for the parent with ones for the child, by that
change. I might be missing something, though. Comments welcome.
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
On 2017/06/02 18:10, Etsuro Fujita wrote:
On 2017/05/16 21:36, Etsuro Fujita wrote:
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.While updating the patch, I noticed the patch rewrites the UPDATE
targetList incorrectly in some cases; rewrite_inherited_tlist I added to
adjust_appendrel_attrs (1) removes all junk items from the targetList
and (2) adds junk items for the child table using rewriteTargetListUD,
but it's wrong to drop all junk items in cases where there are junk
items for some other reasons than rewriteTargetListUD. Consider junk
items containing MULTIEXPR SubLink. One way I came up with to fix this
is to change (1) to only remove junk items with resname; since junk
items added by rewriteTargetListUD should have resname (note: we would
need resname to call ExecFindJunkAttributeInTlist at execution time!)
while other junk items wouldn't have resname (see
transformUpdateTargetList), we could correctly replace junk items added
by rewriteTargetListUD for the parent with ones for the child, by that
change. I might be missing something, though. Comments welcome.
I updated the patch that way. Please find attached an updated version.
Other changes:
* Moved the initialization for "tupleid" I added in ExecModifyTable as
discussed before, which I think is essentially the same as proposed by
Ildus in [1]/messages/by-id/20170514150525.0346ba72@postgrespro.ru, since I think that would be more consistent with "oldtuple".
* Added regression tests.
Anyway I'll add this to the next commitfest.
Best regards,
Etsuro Fujita
[1]: /messages/by-id/20170514150525.0346ba72@postgrespro.ru
/messages/by-id/20170514150525.0346ba72@postgrespro.ru
