Re: [HACKERS] Almost there on column aliases

Thomas Lockhart <lockhart@alumni.caltech.edu> writes:
> The problem I recall is that paren also introduces a "plan", and if
> you call nodeRead() it sees the paren and then complains later because
> it expects a node label following the paren.

> I probably misdiagnosed the behavior, but in any case I'd be *really*
> happy if someone wants to put me out of my misery on this one ;)

Ah-hah, I see it.  nodeRead() expects that simple Value objects
(T_Integer, T_Float, T_String) will be output without any '{' ... '}'
wrapping.  _outNode() was putting them out with wrapping.  Apparently,
you're the first person in a long time (maybe forever) to try to dump
and reload node structures in which these node types appear outside
the context of a Const node.  (outConst calls outValue directly, without
going through outNode, so the bug didn't appear in that case.)

I've fixed _outNode() to suppress the unwanted wrapper for a Value
and removed the now-unnecessary special-case code for Attr lists.

BTW, the rule regress test is presently failing because I modified
ruleutils.c to dump the Attr list if it is not null, rather than
only if the refname is different from the relname:

*** 992,1008 ****                                  quote_identifier(rte->relname),
inherit_marker(rte));                if (strcmp(rte->relname, rte->ref->relname) != 0)
 
-                 {
-                     List *col;                     appendStringInfo(buf, " %s",
quote_identifier(rte->ref->relname));                    appendStringInfo(buf, " (");
 
!                     foreach (col, rte->ref->attrs)                     {
!                         if (col != lfirst(rte->ref->attrs))                             appendStringInfo(buf, ", ");
!                         appendStringInfo(buf, "%s", strVal(col));                     }                 }
}        }
 
--- 992,1012 ----                                  quote_identifier(rte->relname),
inherit_marker(rte));                if (strcmp(rte->relname, rte->ref->relname) != 0)
appendStringInfo(buf," %s",                                      quote_identifier(rte->ref->relname));
 
+                 if (rte->ref->attrs != NIL)
+                 {
+                     List *col;
+                      appendStringInfo(buf, " (");
!                     foreach(col, rte->ref->attrs)                     {
!                         if (col != rte->ref->attrs)                             appendStringInfo(buf, ", ");
!                         appendStringInfo(buf, "%s",
!                                          quote_identifier(strVal(lfirst(col))));                     }
+                     appendStringInfo(buf, ")");                 }             }         }

While this seems like appropriate logic, a bunch of the rule tests are
now showing long and perfectly content-free lists of attribute names in
reverse-listed FROM clauses.  This bothers me because it implies that
those names are being stored in the querytree that's dumped out to
pg_rewrite, which will be a further crimp in people's ability to write
complex rules.  I think we really don't want to store those nodes if we
don't have to.  Why are we building Attr lists when there's no actual
column aliasing being done?
        regards, tom lane