Re: postgres_fdw: Oddity in pushing down inherited UPDATE/DELETEjoins to remote servers - Mailing list pgsql-hackers

From Amit Langote
Subject Re: postgres_fdw: Oddity in pushing down inherited UPDATE/DELETEjoins to remote servers
Date
Msg-id 5f8521d1-26e1-207d-7580-0f5a24b0592e@lab.ntt.co.jp
Whole thread Raw
In response to Re: postgres_fdw: Oddity in pushing down inherited UPDATE/DELETEjoins to remote servers  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
Responses Re: postgres_fdw: Oddity in pushing down inherited UPDATE/DELETEjoins to remote servers  (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>)
Re: postgres_fdw: Oddity in pushing down inherited UPDATE/DELETEjoins to remote servers  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
List pgsql-hackers
On 2018/05/17 12:30, Etsuro Fujita wrote:
> (2018/05/17 1:16), Robert Haas wrote:
>> Was it just good luck that this ever worked at all?  I mean:
>>
>> -        if (rti<  root->simple_rel_array_size&&
>> -            root->simple_rel_array[rti] != NULL)
>> +        if (rti<  subroot->simple_rel_array_size&&
>> +            subroot->simple_rel_array[rti] != NULL)
>>           {
>> -            RelOptInfo *resultRel = root->simple_rel_array[rti];
>> +            RelOptInfo *resultRel = subroot->simple_rel_array[rti];
>>
>>               fdwroutine = resultRel->fdwroutine;
>>           }
>>           else
>>           {
>> -            RangeTblEntry *rte = planner_rt_fetch(rti, root);
>> +            RangeTblEntry *rte = planner_rt_fetch(rti, subroot);
>>
>> ...an RTI is only meaningful relative to a particular PlannerInfo's
>> range table.  It can't be right to just take an RTI for one
>> PlannerInfo and look it up in some other PlannerInfo's range table.
> 
> I think that can be right; because inheritance_planner generates the
> simple_rel_array and simple_rte_array for the parent PlannerInfo so that
> it allows us to do that.  This is the logic that that function creates the
> simple_rel_array for the parent PlannerInfo:
> 
>         /*
>          * We need to collect all the RelOptInfos from all child plans into
>          * the main PlannerInfo, since setrefs.c will need them.  We use the
>          * last child's simple_rel_array (previous ones are too short), so we
>          * have to propagate forward the RelOptInfos that were already built
>          * in previous children.
>          */
>         Assert(subroot->simple_rel_array_size >= save_rel_array_size);
>         for (rti = 1; rti < save_rel_array_size; rti++)
>         {
>             RelOptInfo *brel = save_rel_array[rti];
> 
>             if (brel)
>                 subroot->simple_rel_array[rti] = brel;
>         }

Looking at this for a bit, I wondered if this crash wouldn't have occurred
if the "propagation" had also considered join relations in addition to
simple relations.  For example, if I changed inheritance_planner like the
attached (not proposing that we consider committing it), reported crash
doesn't occur.  The fact that it's not currently that way means that
somebody thought that there is no point in keeping all of those joinrels
around until plan creation time.  If that is so, is it a bit worrying that
a FDW function invoked from createplan.c may try to look for one?

Thanks,
Amit



pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: Needless additional partition check in INSERT?
Next
From: Amit Langote
Date:
Subject: Re: Needless additional partition check in INSERT?