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