On 2016/11/23 0:30, Ashutosh Bapat wrote:

You wrote:
The comment below says "get the aliases", but what the function really
is the identifiers used for creating aliases. Please correct the comment.
+ * Get the relation and column alias for a given Var node, which belongs
+ * 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
+  * 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.
that the targetlist should be saved in fpinfo, the first time it gets
But the patch seems to be searching in foreignrel->reltarget->exprs and
deparsing from the tlist returned by add_to_flat_tlist(tlist,
+    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
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
tlist unless it's needed and also use the same tlist for all searches.
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:

Reply via email to