Re: Push down more full joins in postgres_fdw

On 2016/11/19 0:16, Ashutosh Bapat wrote:
> Also another point
>
> I guess, this note doesn't add much value in the given context.
> Probably we should remove it.
> + * Note: the tlist would have one-to-one correspondence with the
> + * joining relation's reltarget->exprs because (1) the above test
> + * on PHVs guarantees that the reltarget->exprs doesn't contain
> + * any PHVs and (2) the joining relation's local_conds is NIL.
> + * This allows us to search the targetlist entry matching a given
> + * Var node from the tlist in get_subselect_alias_id.

OK, I removed.

> On Fri, Nov 18, 2016 at 5:10 PM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

I wrote:
>>> /*
>>> * For a join relation or an upper relation, use
>>> deparseExplicitTargetList.
>>> * Likewise, for a base relation that is being deparsed as a subquery,
>>> in
>>> * which case the caller would have passed tlist that is non-NIL, use
>>> that
>>> * function. Otherwise, use deparseTargetList.
>>> */

>> This looks correct. I have modified it to make it simple in the given
>> patch. But, I think we shouldn't refer to a function e.g.
>> deparseExplicitTargetlist() in the comment. Instead we should describe
>> the intent e.g. "deparse SELECT clause from the given targetlist" or
>> "deparse SELECT clause from attr_needed".

My taste would be to refer to those functions, because ISTM that makes
the explanation more simple and direct. So, I'd like to leave that for
the committer's judge.

I wrote:
>>>>> Done. I modified the patch as proposed; create the tlist by
>>>>> build_tlist_to_deparse in foreign_join_ok, if needed, and search the
>>>>> tlist
>>>>> by tlist_member. I also added a new member "tlist" to PgFdwRelationInfo
>>>>> to
>>>>> save the tlist created in foreign_join_ok.

You wrote:
>>>> Instead of adding a new member, you might want to reuse grouped_tlist
>>>> by renaming it.

>>> Done.

>> Right now, we are calculating tlist whether or not the ForeignPath
>> emerges as the cheapest path.

Yeah, I modified the patch so, as I thought that would be consistent
with the aggregate pushdown patch.

>> Instead we should calculate tlist, the
>> first time we need it and then add it to the fpinfo.

Having said that, I agree on that point. I'd like to propose (1) adding
a new member to fpinfo, to store a list of output Vars from the
subquery, and (2) creating a tlist from it in deparseRangeTblRef, then,
which would allow us to calculate the tlist only when we need it. The
member added to fpinfo would be also useful to address the comments on
the DML/UPDATE pushdown patch. See the attached patch in [1]. I named
the member a bit differently in that patch, though.

You modified the comments I added to deparseLockingClause into this:

/*
+ * Ignore relation if it appears in a lower subquery. Locking clause
+ * for such a relation is included in the subquery.
+ */

I don't think the second sentence is 100% correct because a locking
clause isn't always required for such a relation, so I modified the
sentence a bit.

Best regards,
Etsuro Fujita

[1]
https://www.postgresql.org/message-id/38245b84-fabf-0899-1b24-8f94cdc5900c%40lab.ntt.co.jp