Re: [HACKERS] patch: function xmltable - Mailing list pgsql-hackers

Pavel Stehule wrote:

> attached update with fixed tests

Heh, I noticed that you removed the libxml "context" lines that
differentiate xml.out from xml_2.out when doing this.  My implementation
emits those lines, so it was failing for me.  I restored them.

I also changed a few things to avoid copying into TableFuncScanState
things that come from the TableFunc itself, since the executor state
node can grab them from the plan node.  Let's do that.  So instead of
"evalcols" the code now checks that the column list is empty; and also,
read the ordinality column number from the plan node.

I have to bounce this back to you one more time, hopefully the last one
I hope.  Two things:

1. Please verify that pg_stat_statements behaves correctly.  The patch
doesn't have changes to contrib/ so without testing I'm guessing that it
doesn't work.  I think something very simple should do.

2. As I've complained many times, I find the way we manage an empty
COLUMNS clause pretty bad.  The standard doesn't require that syntax
(COLUMNS is required), and I don't like the implementation, so why not
provide the feature in a different way?  My proposal is to change the
column options in gram.y to be something like this:

xmltable_column_option_el:
                        IDENT b_expr
                                { $$ = makeDefElem($1, $2, @1); }
                        | DEFAULT b_expr
                                { $$ = makeDefElem("default", $2, @1); }
                        | FULL VALUE_P
                                { $$ = makeDefElem("full_value", NULL, @1); }
                        | NOT NULL_P
                                { $$ = makeDefElem("is_not_null", (Node *) makeInteger(true), @1); }
                        | NULL_P
                                { $$ = makeDefElem("is_not_null", (Node *) makeInteger(false), @1); }
                ;

Note the FULL VALUE.  Then we can process it like

                        else if (strcmp(defel->defname, "full_value") == 0)
                        {
                                if (fc->colexpr != NULL)
                                        ereport(ERROR,
                                                        (errcode(ERRCODE_SYNTAX_ERROR),
                                                         errmsg("FULL ROW may not be specified together with PATH"),
                                                         parser_errposition(defel->location)));
                                fc->full_row = true;
                        }

So if you want the full XML value of the row, you have to specify it,

 .. XMLTABLE ( ... COLUMNS ..., whole_row xml FULL VALUE, ... )

This has the extra feature that you can add, say, an ORDINALITY column
together with the XML value, something that you cannot do with the
current implementation.

It doesn't have to be FULL VALUE, but I couldn't think of anything
better.  (I didn't want to add any new keywords for this.)  If you have
a better idea, let's discuss.

Code-wise, this completely removes the "else" block in transformRangeTableFunc
which I marked with an XXX comment.  That's a good thing -- let's get
rid of that.  Also, it should remove the need for the separate "if
!columns" case in tfuncLoadRows.  All those cases would become part of
the normal code path instead of special cases.  I think
XmlTableSetColumnFilter doesn't need any change (we just don't call if
for the FULL VALUE row); and XmlTableGetValue needs a special case that
if the column filter is NULL (i.e. SetColumnFilter wasn't called for
that column) then return the whole row.


Of course, this opens an implementation issue: how do you annotate
things from parse analysis till execution?  The current TableFunc
structure doesn't help, because there are only lists of column names and
expressions; and we can't use the case of a NULL colexpr, because that
case is already used by the column filter being the column name (a
feature required by the standard).  A simple way would be to have a new
"colno" struct member, to store a column number for the column marked
FULL VALUE (just like ordinalitycol).  This means you can't have more
than one of those FULL VALUE columns, but that seems okay.


(Of course, this means that the two cases that have no COLUMNS in the
"xmltable" production in gram.y should go away).

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services