Re: [HACKERS] Re: [BUGS] BUG #14657: Server process segmentation fault in v10, May 10th dev snapshot
On Fri, May 19, 2017 at 6:07 AM, Ashutosh Bapatwrote: > We still have the same copy shared across multiple append paths and > set_plan_refs would change change it underneath those. May not be a > problem right now but may be a problem in the future. I agree. I think it's better for the path-creation functions to copy the list, so that there is no surprising sharing of substructure. set_plan_refs() obviously expects this data to be unshared, and this seems like the best way to ensure that's true in all cases. Committed that way. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14657: Server process segmentation fault in v10, May 10th dev snapshot
On Thu, May 18, 2017 at 7:23 AM, Amit Langotewrote: > On 2017/05/18 10:49, Amit Langote wrote: >> On 2017/05/18 2:14, Dilip Kumar wrote: >>> On Wed, May 17, 2017 at 7:41 PM, wrote: (gdb) bt #0 0x0061ab1b in list_nth () #1 0x005e4081 in ExecLockNonLeafAppendTables () #2 0x005f4d52 in ExecInitMergeAppend () #3 0x005e0365 in ExecInitNode () #4 0x005f35a7 in ExecInitLimit () #5 0x005e00f3 in ExecInitNode () #6 0x005dd207 in standard_ExecutorStart () #7 0x006f96d2 in PortalStart () #8 0x006f5c7f in exec_simple_query () #9 0x006f6fac in PostgresMain () #10 0x00475cdc in ServerLoop () #11 0x00692ffa in PostmasterMain () #12 0x00476600 in main () >> >> Thanks for the test case Sveinn and thanks Dilip for analyzing. >> >>> Seems like the issue is that the plans under multiple subroots are >>> pointing to the same partitioned_rels. >> >> That's correct. >> >>> If I am not getting it wrong "set_plan_refs(PlannerInfo *root, Plan >>> *plan, int rtoffset)" the rtoffset is specific to the subroot. Now, >>> problem is that set_plan_refs called for different subroot is updating >>> the same partition_rel info and make this value completely wrong which >>> will ultimately make ExecLockNonLeafAppendTables to access the out of >>> bound "rte" index. >> >> Yes. >> >>> set_plan_refs >>> { >>> [clipped] >>> case T_MergeAppend: >>> { >>> [clipped] >>> >>> foreach(l, splan->partitioned_rels) >>> { >>> lfirst_int(l) += rtoffset; >>> >>> >>> I think the solution should be that create_merge_append_path make the >>> copy of partitioned_rels list? >> >> Yes, partitioned_rels should be copied. >> >>> Attached patch fixes the problem but I am not completely sure about the fix. >> >> Thanks for creating the patch, although I think a better fix would be to >> make get_partitioned_child_rels() do the list_copy. That way, any other >> users of partitioned_rels will not suffer the same issue. Attached patch >> implements that, along with a regression test. >> >> Added to the open items. > > Oops, forgot to cc -hackers. Patch attached again. May be we should add a comment as to why the copy is needed. We still have the same copy shared across multiple append paths and set_plan_refs would change change it underneath those. May not be a problem right now but may be a problem in the future. Another option, which consumes a bit less memory is to make a copy at the time of planning if the path gets selected as the cheapest path. -- 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
[HACKERS] Re: [BUGS] BUG #14657: Server process segmentation fault in v10, May 10th dev snapshot
The patch fixed the problem, thanks a lot. Regards, Sveinn. On fim 18.maĆ 2017 01:53, Amit Langote wrote: > On 2017/05/18 10:49, Amit Langote wrote: >> On 2017/05/18 2:14, Dilip Kumar wrote: >>> On Wed, May 17, 2017 at 7:41 PM,wrote: (gdb) bt #0 0x0061ab1b in list_nth () #1 0x005e4081 in ExecLockNonLeafAppendTables () #2 0x005f4d52 in ExecInitMergeAppend () #3 0x005e0365 in ExecInitNode () #4 0x005f35a7 in ExecInitLimit () #5 0x005e00f3 in ExecInitNode () #6 0x005dd207 in standard_ExecutorStart () #7 0x006f96d2 in PortalStart () #8 0x006f5c7f in exec_simple_query () #9 0x006f6fac in PostgresMain () #10 0x00475cdc in ServerLoop () #11 0x00692ffa in PostmasterMain () #12 0x00476600 in main () >> Thanks for the test case Sveinn and thanks Dilip for analyzing. >> >>> Seems like the issue is that the plans under multiple subroots are >>> pointing to the same partitioned_rels. >> That's correct. >> >>> If I am not getting it wrong "set_plan_refs(PlannerInfo *root, Plan >>> *plan, int rtoffset)" the rtoffset is specific to the subroot. Now, >>> problem is that set_plan_refs called for different subroot is updating >>> the same partition_rel info and make this value completely wrong which >>> will ultimately make ExecLockNonLeafAppendTables to access the out of >>> bound "rte" index. >> Yes. >> >>> set_plan_refs >>> { >>> [clipped] >>> case T_MergeAppend: >>> { >>> [clipped] >>> >>> foreach(l, splan->partitioned_rels) >>> { >>> lfirst_int(l) += rtoffset; >>> >>> >>> I think the solution should be that create_merge_append_path make the >>> copy of partitioned_rels list? >> Yes, partitioned_rels should be copied. >> >>> Attached patch fixes the problem but I am not completely sure about the fix. >> Thanks for creating the patch, although I think a better fix would be to >> make get_partitioned_child_rels() do the list_copy. That way, any other >> users of partitioned_rels will not suffer the same issue. Attached patch >> implements that, along with a regression test. >> >> Added to the open items. > Oops, forgot to cc -hackers. Patch attached again. > > Thanks, > Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #14657: Server process segmentation fault in v10, May 10th dev snapshot
On 2017/05/18 10:49, Amit Langote wrote: > On 2017/05/18 2:14, Dilip Kumar wrote: >> On Wed, May 17, 2017 at 7:41 PM,wrote: >>> (gdb) bt >>> #0 0x0061ab1b in list_nth () >>> #1 0x005e4081 in ExecLockNonLeafAppendTables () >>> #2 0x005f4d52 in ExecInitMergeAppend () >>> #3 0x005e0365 in ExecInitNode () >>> #4 0x005f35a7 in ExecInitLimit () >>> #5 0x005e00f3 in ExecInitNode () >>> #6 0x005dd207 in standard_ExecutorStart () >>> #7 0x006f96d2 in PortalStart () >>> #8 0x006f5c7f in exec_simple_query () >>> #9 0x006f6fac in PostgresMain () >>> #10 0x00475cdc in ServerLoop () >>> #11 0x00692ffa in PostmasterMain () >>> #12 0x00476600 in main () > > Thanks for the test case Sveinn and thanks Dilip for analyzing. > >> Seems like the issue is that the plans under multiple subroots are >> pointing to the same partitioned_rels. > > That's correct. > >> If I am not getting it wrong "set_plan_refs(PlannerInfo *root, Plan >> *plan, int rtoffset)" the rtoffset is specific to the subroot. Now, >> problem is that set_plan_refs called for different subroot is updating >> the same partition_rel info and make this value completely wrong which >> will ultimately make ExecLockNonLeafAppendTables to access the out of >> bound "rte" index. > > Yes. > >> set_plan_refs >> { >> [clipped] >> case T_MergeAppend: >> { >> [clipped] >> >> foreach(l, splan->partitioned_rels) >> { >> lfirst_int(l) += rtoffset; >> >> >> I think the solution should be that create_merge_append_path make the >> copy of partitioned_rels list? > > Yes, partitioned_rels should be copied. > >> Attached patch fixes the problem but I am not completely sure about the fix. > > Thanks for creating the patch, although I think a better fix would be to > make get_partitioned_child_rels() do the list_copy. That way, any other > users of partitioned_rels will not suffer the same issue. Attached patch > implements that, along with a regression test. > > Added to the open items. Oops, forgot to cc -hackers. Patch attached again. Thanks, Amit >From 73754e24178087cf8aa53904acfb032343be25df Mon Sep 17 00:00:00 2001 From: amit Date: Thu, 18 May 2017 10:45:44 +0900 Subject: [PATCH] Fix crash when partitioned table Append referenced in subplans --- src/backend/optimizer/plan/planner.c | 2 +- src/test/regress/expected/inherit.out | 31 +++ src/test/regress/sql/inherit.sql | 10 ++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index c4a5651abd..bc2797e42c 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -6076,7 +6076,7 @@ get_partitioned_child_rels(PlannerInfo *root, Index rti) if (pc->parent_relid == rti) { - result = pc->child_rels; + result = list_copy(pc->child_rels); break; } } diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index af7090ba0d..35d182d599 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1918,3 +1918,34 @@ explain (costs off) select * from mcrparted where a = 20 and c > 20; -- scans mc (7 rows) drop table mcrparted; +-- check that partitioned table Appends cope with being referenced in +-- subplans +create table parted_minmax (a int, b varchar(16)) partition by range (a); +create table parted_minmax1 partition of parted_minmax for values from (1) to (10); +create index parted_minmax1i on parted_minmax1 (a, b); +insert into parted_minmax values (1,'12345'); +explain (costs off) select min(a), max(a) from parted_minmax where b = '12345'; + QUERY PLAN +--- + Result + InitPlan 1 (returns $0) + -> Limit + -> Merge Append + Sort Key: parted_minmax1.a + -> Index Only Scan using parted_minmax1i on parted_minmax1 + Index Cond: ((a IS NOT NULL) AND (b = '12345'::text)) + InitPlan 2 (returns $1) + -> Limit + -> Merge Append + Sort Key: parted_minmax1_1.a DESC + -> Index Only Scan Backward using parted_minmax1i on parted_minmax1 parted_minmax1_1 + Index Cond: ((a IS NOT NULL) AND (b = '12345'::text)) +(13 rows) + +select min(a), max(a) from parted_minmax where b = '12345'; + min | max +-+- + 1 | 1 +(1 row) + +drop table parted_minmax; diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 7f34f43ec0..70fe971d51 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@