Re: Push down more full joins in postgres_fdw

On Thu, Nov 24, 2016 at 1:27 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2016/11/24 16:46, Ashutosh Bapat wrote:
>>>>
>>>> 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".
>
>
> I wrote:
>>>
>>> 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".)
>
>
>> table will be misleading as subquery can represent a join and
>> corresponding alias would represent the join. Relation is better term.
>
>
> But the documentation uses the word "table" for a join relation.  See
> 7.2.1.2. Table and Column Aliases:
> https://www.postgresql.org/docs/9.6/static/queries-table-expressions.html#QUERIES-TABLE-ALIASES

The definition there says "A temporary name can be given to tables and
complex table references to be used for references to the derived
table in the rest of the query. This is called a table alias.", please
note the usage "complex table references". Within the code, we do not
refer to a relation as table. So, the comment in this code should not
refer to a relation as table either.

>
>>> 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.
>
>
>> build_tlist_to_depase() calls pull_var_nodes() before creating the
>> tlist, whereas the code that searches does not do that. Code-wise
>> those are not the same things.
>
>
> You missed the point; the foreignrel->reltarget->exprs doesn't contain any
> PHVs, so the tlist created by build_tlist_to_depase will be guaranteed to be
> one-to-one with the foreignrel->reltarget->exprs.

It's guaranteed now, but can not be forever. This means anybody who
supports PHV tomorrow, will have to "remember" to update this code as
well. If s/he misses it, a bug will be introduced. That's the avenue
for bug, I am talking about.

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