Re: [HACKERS] Almost there on column aliases

> 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.

Great. Thanks. And I should have committed my garbage earlier rather
than trying to make it work poorly ;)

> 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, ")");
>                   }
>               }
>           }

istm that the column aliases (rte->ref->attrs) should not be written out
if the table alias (rte->ref->relname) is not written. And the rules
regression test should be failing anyway, because I didn't update it
since I knew that there was something wrong with those plan strings and
I didn't want to hide that.

> 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?

Hmm. Because there are multiple places in the parser which needs to get
at a column name. When handling column aliases, I was having to look up
the actual column names anyway to verify that there were the correct
number of aliases specified (actually, I decided to allow any number of
aliases <= the number of actual columns, filling in with the underlying
column names if an alias was not specified) and so while I had the info
I cached it into the RTE structure for later use.

If I make the ref structure optional, then I have to start returning
lists of columns when working out the new join syntax, and I hated to
keep generating a bunch of temporary lists of things. Also, by making
the ref->refname non-optional in the structure, I could stop checking
for its existance before using either it *or* the true table name; this
cleaned up a bit of the code.
                     - Thomas

-- 
Thomas Lockhart
Caltech/JPL
Interferometry Systems and Technology