Re: Push down more full joins in postgres_fdw

>
>
>> There is a already a function to build targetlist for a given relation
>> build_tlist_to_deparse(), which does the same thing as this code for a
>> join or
>> base relation and when there are no local conditions. Why don't we use
>> that
>> function instead of duplicating that logic? If tomorrow, the logic to
>> build the
>> targetlist changes, we will need to duplicate those changes. We should
>> avoid
>> that.
>> + /* Build a tlist from the subquery. */
>> + tlist = add_to_flat_tlist(tlist, foreignrel->reltarget->exprs);
>
>
> Will modify the patch to use build_tlist_to_deparse. Actually, the early
> versions of the patch used that function, but it looks like I changed not to
> use that function, as I misunderstood about your comments on this part at
> some point. Sorry for that.
>
>> 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.
>> + */
>
>
> 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".

>
>> 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 think I
have explained this before.

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