pg.ddx.io pgsql-hackers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
Back-branch bugs with fully-prunable UPDATEs 5+ messages / 4 participants [nested] [flat]
* Back-branch bugs with fully-prunable UPDATEs @ 2019-04-06 20:27 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2019-04-06 20:27 UTC (permalink / raw) To: pgsql-hackers@lists.postgresql.org; +Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> This test script works fine in HEAD: drop table if exists parttbl cascade; CREATE TABLE parttbl (a int, b int) PARTITION BY LIST (a); CREATE TABLE parttbl_1 PARTITION OF parttbl FOR VALUES IN (NULL,500,501,502); UPDATE parttbl SET a = NULL, b = NULL WHERE a = 1600 AND b = 999; In v11, it suffers an assertion failure in ExecSetupPartitionTupleRouting. In v10, it doesn't crash, but we do get WARNING: relcache reference leak: relation "parttbl" not closed which is surely a bug as well. (This is a boiled-down version of the script I mentioned in https://www.postgresql.org/message-id/13344.1554578481@sss.pgh.pa.us) This seems to be related to what Amit Langote complained of in https://www.postgresql.org/message-id/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp but since there's no foreign tables involved at all, either it's a different bug or he misdiagnosed what he was seeing. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Back-branch bugs with fully-prunable UPDATEs @ 2019-04-07 07:54 Amit Langote <amitlangote09@gmail.com> parent: Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 2 replies; 5+ messages in thread From: Amit Langote @ 2019-04-07 07:54 UTC (permalink / raw) To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> On Sun, Apr 7, 2019 at 5:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > This test script works fine in HEAD: > > drop table if exists parttbl cascade; > CREATE TABLE parttbl (a int, b int) PARTITION BY LIST (a); > CREATE TABLE parttbl_1 PARTITION OF parttbl FOR VALUES IN (NULL,500,501,502); > UPDATE parttbl SET a = NULL, b = NULL WHERE a = 1600 AND b = 999; > > In v11, it suffers an assertion failure in ExecSetupPartitionTupleRouting. > > In v10, it doesn't crash, but we do get > > WARNING: relcache reference leak: relation "parttbl" not closed > > which is surely a bug as well. > > (This is a boiled-down version of the script I mentioned in > https://www.postgresql.org/message-id/13344.1554578481@sss.pgh.pa.us) What we did in the following commit is behind this: commit 58947fbd56d1481a86a03087c81f728fdf0be866 Author: Tom Lane <tgl@sss.pgh.pa.us> Date: Fri Feb 22 12:23:00 2019 -0500 Fix plan created for inherited UPDATE/DELETE with all tables excluded. Before this commit, partitioning related code in the executor could always rely on the fact that ModifyTableState.resultRelInfo[] only contains *leaf* partitions. As of this commit, it may contain the root partitioned table in some cases, which breaks that assumption. I've attached fixes for PG 10 and 11, modifying ExecInitModifyTable() and inheritance_planner(), respectively. > This seems to be related to what Amit Langote complained of in > https://www.postgresql.org/message-id/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp > but since there's no foreign tables involved at all, either it's > a different bug or he misdiagnosed what he was seeing. I think that one is a different bug, but maybe I haven't looked closely enough. Thanks, Amit Attachments: [application/octet-stream] pg11-empty-ModifyTable-no-routing.patch (2.2K, ../../CA+HiwqExk2yo57aX2Mj99nR0SOP4s43J1+nccLm6h=3UzoX0qQ@mail.gmail.com/2-pg11-empty-ModifyTable-no-routing.patch) download | inline diff: diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 60edaa8b0a..0ae7b238e2 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1599,6 +1599,14 @@ inheritance_planner(PlannerInfo *root) withCheckOptionLists = list_make1(parse->withCheckOptions); if (parse->returningList) returningLists = list_make1(parse->returningList); + /* + * Since no tuples will be updated, don't require ModifyTable to + * create tuple-routing info that will be left unused. In fact + * it's necessary to do so, because we're cheating here by putting + * the root table into resultRelations list, which the tuple-routing + * code is not expecting to be there. + */ + root->partColsUpdated = false; } else { diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index f6d70e9f7a..b78a84e83e 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -665,6 +665,15 @@ select tableoid::regclass::text as relname, parted_tab.* from parted_tab order b parted_tab_part3 | 3 | a (3 rows) +-- modifies partition key, but no rows will actually be updated +explain update parted_tab set a = 2 where false; + QUERY PLAN +-------------------------------------------------------- + Update on parted_tab (cost=0.00..0.00 rows=0 width=0) + -> Result (cost=0.00..0.00 rows=0 width=0) + One-Time Filter: false +(3 rows) + drop table parted_tab; -- Check UPDATE with multi-level partitioned inherited target create table mlparted_tab (a int, b char, c text) partition by list (a); diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 30a45a20ae..f97d7e5e4d 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -168,6 +168,9 @@ from where parted_tab.a = ss.a; select tableoid::regclass::text as relname, parted_tab.* from parted_tab order by 1,2; +-- modifies partition key, but no rows will actually be updated +explain update parted_tab set a = 2 where false; + drop table parted_tab; -- Check UPDATE with multi-level partitioned inherited target [application/octet-stream] pg10-ExecInitModifyTable-root-table-fix.patch (2.5K, ../../CA+HiwqExk2yo57aX2Mj99nR0SOP4s43J1+nccLm6h=3UzoX0qQ@mail.gmail.com/3-pg10-ExecInitModifyTable-root-table-fix.patch) download | inline diff: diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 7de4568b03..fe29a81c1b 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1934,15 +1934,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) estate->es_result_relation_info = saved_resultRelInfo; /* The root table RT index is at the head of the partitioned_rels list */ - if (node->partitioned_rels) - { - Index root_rti; - Oid root_oid; - - root_rti = linitial_int(node->partitioned_rels); - root_oid = getrelid(root_rti, estate->es_range_table); - rel = heap_open(root_oid, NoLock); /* locked by InitPlan */ - } + if (mtstate->rootResultRelInfo) + rel = mtstate->rootResultRelInfo->ri_RelationDesc; else rel = mtstate->resultRelInfo->ri_RelationDesc; @@ -2134,10 +2127,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) mtstate->ps.ps_ExprContext = NULL; } - /* Close the root partitioned rel if we opened it above. */ - if (rel != mtstate->resultRelInfo->ri_RelationDesc) - heap_close(rel, NoLock); - /* * If needed, Initialize target list, projection and qual for ON CONFLICT * DO UPDATE. diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index da7608734c..79ea0c5774 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -665,6 +665,15 @@ select tableoid::regclass::text as relname, parted_tab.* from parted_tab order b parted_tab_part3 | 3 | a (3 rows) +-- modifies partition key, but no rows will actually be updated +explain update parted_tab set a = 2 where false; + QUERY PLAN +-------------------------------------------------------- + Update on parted_tab (cost=0.00..0.00 rows=0 width=0) + -> Result (cost=0.00..0.00 rows=0 width=0) + One-Time Filter: false +(3 rows) + drop table parted_tab; drop table some_tab cascade; NOTICE: drop cascades to table some_tab_child diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index bbe453cf26..101a0394f8 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -168,6 +168,9 @@ from where parted_tab.a = ss.a; select tableoid::regclass::text as relname, parted_tab.* from parted_tab order by 1,2; +-- modifies partition key, but no rows will actually be updated +explain update parted_tab set a = 2 where false; + drop table parted_tab; drop table some_tab cascade; ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Back-branch bugs with fully-prunable UPDATEs @ 2019-04-07 16:57 Tom Lane <tgl@sss.pgh.pa.us> parent: Amit Langote <amitlangote09@gmail.com> 1 sibling, 1 reply; 5+ messages in thread From: Tom Lane @ 2019-04-07 16:57 UTC (permalink / raw) To: Amit Langote <amitlangote09@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> Amit Langote <amitlangote09@gmail.com> writes: > On Sun, Apr 7, 2019 at 5:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> This test script works fine in HEAD: >> In v11, it suffers an assertion failure in ExecSetupPartitionTupleRouting. >> In v10, it doesn't crash, but we do get >> WARNING: relcache reference leak: relation "parttbl" not closed > What we did in the following commit is behind this: > commit 58947fbd56d1481a86a03087c81f728fdf0be866 > Before this commit, partitioning related code in the executor could > always rely on the fact that ModifyTableState.resultRelInfo[] only > contains *leaf* partitions. As of this commit, it may contain the > root partitioned table in some cases, which breaks that assumption. Ah. Thanks for the diagnosis and patches; pushed. I chose to patch HEAD similarly to v11, even though no bug manifests right now; it seems safer that way. We should certainly have the test case in HEAD, now that we realize there wasn't coverage for this. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Back-branch bugs with fully-prunable UPDATEs @ 2019-04-08 03:02 Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> parent: Amit Langote <amitlangote09@gmail.com> 1 sibling, 0 replies; 5+ messages in thread From: Etsuro Fujita @ 2019-04-08 03:02 UTC (permalink / raw) To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> (2019/04/07 16:54), Amit Langote wrote: > On Sun, Apr 7, 2019 at 5:28 AM Tom Lane<tgl@sss.pgh.pa.us> wrote: >> This seems to be related to what Amit Langote complained of in >> https://www.postgresql.org/message-id/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp >> but since there's no foreign tables involved at all, either it's >> a different bug or he misdiagnosed what he was seeing. > > I think that one is a different bug, but maybe I haven't looked closely enough. I started working on that from last Friday (though I didn't work on the weekend). I agree on Amit's reasoning stated in that post, and I think that that's my fault. Sorry for the delay. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Back-branch bugs with fully-prunable UPDATEs @ 2019-04-08 04:37 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> parent: Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 5+ messages in thread From: Amit Langote @ 2019-04-08 04:37 UTC (permalink / raw) To: Tom Lane <tgl@sss.pgh.pa.us>; Amit Langote <amitlangote09@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org> On 2019/04/08 1:57, Tom Lane wrote: > Amit Langote <amitlangote09@gmail.com> writes: >> On Sun, Apr 7, 2019 at 5:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> This test script works fine in HEAD: >>> In v11, it suffers an assertion failure in ExecSetupPartitionTupleRouting. >>> In v10, it doesn't crash, but we do get >>> WARNING: relcache reference leak: relation "parttbl" not closed > >> What we did in the following commit is behind this: >> commit 58947fbd56d1481a86a03087c81f728fdf0be866 >> Before this commit, partitioning related code in the executor could >> always rely on the fact that ModifyTableState.resultRelInfo[] only >> contains *leaf* partitions. As of this commit, it may contain the >> root partitioned table in some cases, which breaks that assumption. > > Ah. Thanks for the diagnosis and patches; pushed. Thank you. > I chose to patch HEAD similarly to v11, even though no bug manifests > right now; it seems safer that way. We should certainly have the > test case in HEAD, now that we realize there wasn't coverage for this. Agreed, thanks for taking care of that. Regards, Amit ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2019-04-08 04:37 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-04-06 20:27 Back-branch bugs with fully-prunable UPDATEs Tom Lane <tgl@sss.pgh.pa.us> 2019-04-07 07:54 ` Amit Langote <amitlangote09@gmail.com> 2019-04-07 16:57 ` Tom Lane <tgl@sss.pgh.pa.us> 2019-04-08 04:37 ` Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 2019-04-08 03:02 ` Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox