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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
