Re: Push down more full joins in postgres_fdw

On 2016/11/23 0:30, Ashutosh Bapat wrote:

You wrote:
>>> The comment below says "get the aliases", but what the function really
>>> returns
>>> is the identifiers used for creating aliases. Please correct the comment.
>>> +/*
>>> + * Get the relation and column alias for a given Var node, which belongs
>>> to
>>> + * input foreignrel. They are returned in *tabno and *colno respectively.
>>> + */

I wrote:
>> Actually, this was rewritten into the above by *you*.  The original comment
>> I added was:
>>
>> + /*
>> +  * Get info about the subselect alias to given expression.
>> +  *
>> +  * The subselect table and column numbers are returned to *tabno and
>> *colno,
>> +  * respectively.
>> +  */
>>
>> I'd like to change the comment into something like the original one.

> Sorry. I think the current version is better than previous one. The
> term "subselect alias" is confusing in the previous version. In the
> current version, "Get the relation and column alias for a given Var
> node," we need to add word "identifiers" like "Get the relation and
> column identifiers for a given Var node".

OK, but one thing I'm concerned about is the term "relation alias" seems 
a bit confusing because we already used the term for the alias of the 
form "rN".  To avoid that, how about saying "table alias", not "relation 
alias"?  (in which case, the comment would be something like "Get the 
table and column identifiers for a given Var node".)

>>> We discussed that we have to deparse and search from the same targetlist.
>>> And
>>> that the targetlist should be saved in fpinfo, the first time it gets
>>> created.
>>> But the patch seems to be searching in foreignrel->reltarget->exprs and
>>> deparsing from the tlist returned by add_to_flat_tlist(tlist,
>>> foreignrel->reltarget->exprs).
>>> +    foreach(lc, foreignrel->reltarget->exprs)
>>> +    {
>>> +        if (equal(lfirst(lc), (Node *) node))
>>> +        {
>>> +            *colno = i;
>>> +            return;
>>> +        }
>>> +        i++;
>>> +    }
>>> I guess, the reason why you are doing it this way, is SELECT clause for
>>> the
>>> outermost query gets deparsed before FROM clause. For later we call
>>> deparseRangeTblRef(), which builds the tlist. So, while deparsing SELECT
>>> clause, we do not have tlist to build from.

>> That's right.

>>> In that case, I guess, we have to
>>> build the tlist in get_subselect_alias_id() if it's not available and
>>> stick it
>>> in fpinfo. Subsequent calls to get_subselect_alias_id() should find tlist
>>> there. Then in deparseRangeTblRef() assert that there's a tlist in fpinfo
>>> and use it to build the SELECT clause of subquery. That way, we don't
>>> build
>>> tlist unless it's needed and also use the same tlist for all searches.
>>> Please
>>> use tlist_member() to search into the tlist.

>> I don't think that's a good idea because that would make the code
>> unnecessarily complicated and inefficient.  I think the direct search into
>> the foreignrel->reltarget->exprs shown above would be better because that's
>> more simple and efficient than what you proposed.  Note that since (1) the
>> foreignrel->reltarget->exprs doesn't contain any PHVs and (2) the
>> local_conds is empty, the tlist created for the subquery by
>> build_tlist_to_deparse is guaranteed to have one-to-one correspondence with
>> the foreignrel->reltarget->exprs, so the direct search works well.

> If we deparse from and search into different objects, that's not very
> maintainable code. Changes to any one of them require changes to the
> other, thus creating avenues for bugs.  Even if slighly expensive, we
> should search into and deparse from the same targetlist.

I don't think so; in the current version, we essentially deparse from 
and search into the same object, ie, foreignrel->reltarget->exprs, since 
the tlist created by build_tlist_to_deparse is build from the 
foreignrel->reltarget->exprs.  Also, since it is just created for 
deparsing the relation as a subquery in deparseRangeTblRef and isn't 
stored into fpinfo or anywhere alse, we would need no concern about 
creating such avenues.  IMO I think adding comments would be enough for 
this.  Anyway, I think this is an optional issue, so I'd like to leave 
this for the committer's judge.

> I think I
> have explained this before.

My apologies for having misunderstood your words.

Best regards,
Etsuro Fujita