Pavel Stehule wrote:
> 2016-12-07 18:34 GMT+01:00 Alvaro Herrera <alvherre@2ndquadrant.com>:
> > Hmm. Now that I see how this works, by having the GetValue "guess" what
> > is going on and have a special case for it, I actually don't like it
> > very much. It seems way too magical. I think we should do away with
> > the "if column is NULL" case in GetValue, and instead inject a column
> > during transformTableExpr if columns is NIL. This has implications on
> > ExecInitExpr too, which currently checks for an empty column list -- it
> > would no longer have to do so.
>
> I prefer this way against second described. The implementation should be in
> table builder routines, not in executor.
Well, given the way you have implemented it, I would prefer the original
too. But your v23 is not what I meant. Essentially what you do in v23
is to communicate the lack of COLUMNS clause in a different way --
previously it was "ncolumns = 0", now it's "is_auto_col=true". It's
still "magic". It's not an improvement.
What I want to happen is that there is no magic at all; it's up to
transformExpr to make sure that when COLUMNS is empty, one column
appears and it must not be a magic column that makes the xml.c code act
differently, but rather to xml.c it should appear that this is just a
normal column that happens to return the entire row. If I say "COLUMNS
foo PATH '/'" I should be able to obtain a similar behavior (except that
in the current code, if I ask for "COLUMNS foo XML PATH '/'" I don't get
XML at all but rather weird text where all tags have been stripped out,
which is very strange. I would expect the tags to be preserved if the
output type is XML. Maybe the tag-stripping behavior should occur if
the output type is some type of text.)
I still have to figure out how to fix the tupledesc thing. What we have
now is not good.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services