Re: [HACKERS] Re: [BUGS] BUG #14657: Server process segmentation fault in v10, May 10th dev snapshot

2017-05-19 Thread Robert Haas
On Fri, May 19, 2017 at 6:07 AM, Ashutosh Bapat
 wrote:
> 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

2017-05-19 Thread Ashutosh Bapat
On Thu, May 18, 2017 at 7:23 AM, 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.

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

2017-05-18 Thread Sveinn Sveinsson
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

2017-05-17 Thread Amit Langote
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
@@