Re: postgres_fdw: Oddity in pushing down inherited UPDATE/DELETE joins to remote servers

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