On 2016/11/23 0:30, Ashutosh Bapat wrote: You wrote:
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. + */
I wrote:
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".
OK, but one thing I'm concerned about is the term "relation alias" seems a bit confusing because we already used the term for the alias of the form "rN". To avoid that, how about saying "table alias", not "relation alias"? (in which case, the comment would be something like "Get the table 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 don't think so; in the current version, we essentially deparse from and search into the same object, ie, foreignrel->reltarget->exprs, since the tlist created by build_tlist_to_deparse is build from the foreignrel->reltarget->exprs. Also, since it is just created for deparsing the relation as a subquery in deparseRangeTblRef and isn't stored into fpinfo or anywhere alse, we would need no concern about creating such avenues. IMO I think adding comments would be enough for this. Anyway, I think this is an optional issue, so I'd like to leave this for the committer's judge.
I think I have explained this before.
My apologies for having misunderstood your words. 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
