Re: Push down more full joins in postgres_fdw

On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2017/01/05 21:38, Ashutosh Bapat wrote:
>>
>> On Thu, Jan 5, 2017 at 5:51 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>
>>> On 2017/01/05 21:11, Ashutosh Bapat wrote:
>
>
>>>>> On 2017/01/03 17:28, Ashutosh Bapat wrote:
>>>>>>
>>>>>> Also, in this function, if fpinfo->tlist is already set, why do we
>>>>>> want
>>>>>> to
>>>>>> build it again?
>
>
>>>> IIUC, for a relation with use_remote_estimates we will deparse the
>>>> query twice and will build the targetlist twice.
>
>
>>> That's right. We could avoid the duplicate work the way you proposed,
>>> but I
>>> was thinking to leave that for another patch. Should we do that in this
>>> patch?
>
>
>> If you are agree that the change is needed, it's better to do it in
>> this patch itself if we can, instead of a one liner patch.
>
>
> 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. This is just a
suggestion, for an additional check, you might want to check
rel->reltarget->exprs for NIL. But I think we don't need it.

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