Re: 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,  <sveinn.sveinsson@gmail.com> wrote:
>>> (gdb) bt
>>> #0  0x000000000061ab1b in list_nth ()
>>> #1  0x00000000005e4081 in ExecLockNonLeafAppendTables ()
>>> #2  0x00000000005f4d52 in ExecInitMergeAppend ()
>>> #3  0x00000000005e0365 in ExecInitNode ()
>>> #4  0x00000000005f35a7 in ExecInitLimit ()
>>> #5  0x00000000005e00f3 in ExecInitNode ()
>>> #6  0x00000000005dd207 in standard_ExecutorStart ()
>>> #7  0x00000000006f96d2 in PortalStart ()
>>> #8  0x00000000006f5c7f in exec_simple_query ()
>>> #9  0x00000000006f6fac in PostgresMain ()
>>> #10 0x0000000000475cdc in ServerLoop ()
>>> #11 0x0000000000692ffa in PostmasterMain ()
>>> #12 0x0000000000476600 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