Re: Push down more full joins in postgres_fdw

On 2017/01/12 18:25, Ashutosh Bapat wrote:
> On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:

>>>> On 2017/01/05 21:11, Ashutosh Bapat wrote:
>>>>> IIUC, for a relation with use_remote_estimates we will deparse the
>>>>> query twice and will build the targetlist twice.

>> While working on this, I noticed a weird case.  Consider:
>>
>> postgres=# explain verbose select 1 from ft1 left join ft2 on (ft1.a =
>> ft2.a) inner join test on (true);
>>                                            QUERY PLAN
>> -------------------------------------------------------------------------------------------------
>>  Nested Loop  (cost=100.00..103.06 rows=1 width=4)
>>    Output: 1
>>    ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=0)
>>          Relations: (public.ft1) LEFT JOIN (public.ft2)
>>          Remote SQL: SELECT NULL FROM (public.t1 r1 LEFT JOIN public.t2 r2
>> ON (((r1.a = r2.a))))
>>    ->  Seq Scan on public.test  (cost=0.00..1.01 rows=1 width=0)
>>          Output: test.a, test.b
>> (7 rows)
>>
>> In this case the fpinfo->tlist of the foreign join is NIL, so whether or not
>> the tlist is already built cannot be discriminated by the fpinfo->tlist.  We
>> might need another flag to show that the tlist has been built already.
>> Since this is an existing issue and we would need to give careful thought to
>> this, so I'd like to leave this for another patch.

> I think in that case, relation's targetlist will also be NIL or
> contain no Var node. It wouldn't be expensive to build it again and
> again. That's better than maintaining a new flag.

I think that's ugly.  A more clean way I'm thinking is: (1) in 
postgresGetForeignJoinPaths(), create a tlist by 
build_tlist_to_deparse() and save it in fpinfo->tlist before 
estimate_path_cost_size() if use_remote_estimates=true, (2) in 
estimate_path_cost_size(), use the fpinfo->tlist if 
use_remote_estimates=true, and (3) in postgresGetForeignPlan(), use the 
fpinfo->tlist as the fdw_scan_tlist if use_remote_estimates=true, 
otherwise create a tlist as the fdw_scan_tlist by 
build_tlist_to_deparse(), like the attached.

What do you think about that?

Another change is: I simplified build_tlist_to_deparse() a bit and put 
that in postgres_fdw.c because that is used only in postgres_fdw.c.

I still think we should discuss this separately because this is an 
existing issue and that makes it easy to review the patch.  If the 
attached is the right way to go, I'll update the join-pushdown patch on 
top of the patch.

Best regards,
Etsuro Fujita