On Thu, Jan 21, 2016 at 8:36 AM, Ashutosh Bapat
>> What this patch does to the naming and calling conventions in
>> deparse.c does not good. Previously, we had deparseTargetList().
>> Now, we sometimes use that and sometimes deparseAttrsUsed() for almost
>> the same thing.
> Previously deparseTargetList deparsed the SELECT or RETURNING clause by
> including list of name of attributes provided by attrs_used. That's now done
> by deparseAttrsUsed(). In current path deparseTargetList() deparses the
> targetlist i.e. list of TargetEntry nodes (right now only Vars). Although
> these two functions return comma separated string of column names, their
> inputs are different. deparseAttrsUsed() can never be called for more than
> one base relation. deparseTargetList() on the other hand can deparse a
> targetlist with Var nodes from multiple relations. We need those two
> functionalities separately. We might convert attrs_used into a list of
> TargetEntry nodes using build_tlist_to_deparse() and use deparseTargetList
> everywhere. A side effect of that would be separating retrieved_attrs
> collection from deparsing code. I didn't do that change in this version to
> avoid large code changes. But I am fine doing that, if that makes code
> If we have to keep old deparseTargetList as is (and don't rename it as
> deparseAttrsUsed), we can rename the new deparseTargetList as something
> different may be deparseSelectList. I am fine with that too. But we need the
> later functionality, whatever its name be.
I'm not arguing that we don't need the functionality. I'm arguing
that if we've got a set of existing functions that are named one way,
we shouldn't get a whole bunch of new functions that invent an
entirely new naming convention. I'm not sure exactly how to clean
this up, but I think we need to find a way.
>> Previously, we had deparseColumnRef(); now we have
>> both that and deparseJoinVar() doing very similar things. But in each
>> case, the function names are not parallel and the calling conventions
>> are totally different. Like here:
>> + if (context->foreignrel->reloptkind == RELOPT_JOINREL)
>> + deparseJoinVar(node, context);
>> + else
>> + deparseColumnRef(buf, node->varno,
>> node->varattno, context->root);
>> We pass the buf separately to deparseColumnRef(), but for some reason
>> not to deparseJoinVar().I wonder if these functions need to be two
>> separate things or if the work done by deparseJoinVar() should
>> actually be part of deparseColumnRef(). But even if it needs to be
>> separate, I wonder why we can't arrange things so that they get the
>> same arguments, more or less.
> deparseVar() is called for any Var node that's encountered. deparseJoinVar
> is called to deparse a Var from join relation, which is supposed to output
> INNER or OUTER var's alias as used in INNER or OUTER subqueries. It does not
> output the real column names. deparseColumnRef() however is the same old
> thing; it deparses column of given base relation. They are not similar
deparseColumnRef() emits things like "foo" meaning column foo, or
"foo.bar" meaning column bar of table foo. deparseJoinVar() emits
things like "r.a7", referring to a column called "a7" in a relation
called "r". I feel that those *are* similar things.
I also wonder whether they couldn't be made more similar. It seems to
me this patch is going to realias things potentially multiple times
for its own convenience. That's not a catastrophe, but it's not
great, either, because it produces queries that are not necessarily
very human readable. It would be nicer to get
actual_table_name.actual_column_name in more places and r.a7 in fewer.
> I agree that the code is complex for a reader. One of the reasons is
> recursive nature of deparsing. I will try to make it more cleaner and easier
> to understand. Would adding a call tree for deparsing routines help here? Or
> improving function prologues of even the existing functions?
I don't think so. A README might help, but honestly I think some of
these APIs really just need to be revised.
The Enterprise PostgreSQL Company
Sent via pgsql-hackers mailing list (firstname.lastname@example.org)
To make changes to your subscription: