Re: [HACKERS] Push down more full joins in postgres_fdw

On Fri, Jan 13, 2017 at 12:30 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> 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.

I don't think it's right to assume that the targetlist will be
available only when use_remote_estimate is true; for grouping it's
certainly not.

But I don't see this discussion getting anywhere. I will leave it to
the committer's judgement. I think we should pick up your patch on
27th December, update the comment per your mail on 5th Jan. I will
review it once and list down the things left to committer's judgement.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company