Re: patch: function xmltable

2017-03-03 19:42 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2017-03-03 19:15 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
>
>> 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:
>
>
> The clause COLUMNS is optional on Oracle and DB2
>
> So I prefer a Oracle, DB2 design. If you are strongly against it, then we
> can remove it to be ANSI/SQL only.
>
> I am don't see an good idea to introduce third syntax.
>
>
>> 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.
>>
>
> I don't see a introduction own syntax as necessary solution here - use
> Oracle, DB2 compatible syntax, or ANSI.
>
> It is partially corner case - the benefit of this case is almost bigger
> compatibility with mentioned databases.
>
>
>>
>> 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).
>>
>
> You are commiter, and you should to decide - as first I prefer current
> state, as second a remove this part - it should be good for you too,
> because code that you don't like will be left.
>
> I dislike introduce new syntax - this case is not too important for this.
>
>
I am able to prepare reduced version if we do a agreement

Regards

Pavel

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