Re: Push down more full joins in postgres_fdw

On 2016/11/22 18:28, Ashutosh Bapat wrote:
> 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));

Will revise.

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

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.

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

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

Fine with me.

Best regards,
Etsuro Fujita