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