Re: Push down more full joins in postgres_fdw

The comments should explain why is the assertion true.
+ /* Shouldn't be NIL */
+ Assert(tlist != NIL);
+ /* Should be same length */
+ Assert(list_length(tlist) ==
list_length(foreignrel->reltarget->exprs));

>
> OK, I'd like to propose referencing to foreignrel->reltarget->exprs directly
> in deparseRangeTblRef and get_subselect_alias_id, then, which is the same as
> what I proposed in the first version of the patch, but I'd also like to
> change deparseRangeTblRef to use add_to_flat_tlist, not
> make_tlist_from_pathtarget, to create a tlist of the subquery, as you
> proposed.

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);

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.
+ */

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. 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.

The name get_subselect_alias_id() seems to suggest that the function returns
identifier for subselect alias, which isn't correct. It returns the alias
identifiers for deparsing a Var node. But I guess, we have left this to the
committer's judgement.

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