Re: patch: function xmltable

2016-11-19 0:42 GMT+01:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> The SQL standard seems to require a comma after the XMLNAMESPACES
> clause:
>
> <XML table> ::=
> XMLTABLE <left paren>
> [ <XML namespace declaration> <comma> ]
> <XML table row pattern>
> [ <XML table argument list> ]
> COLUMNS <XML table column definitions> <right paren>
>
> I don't understand the reason for that, but I have added it:
>
> | XMLTABLE '(' XMLNAMESPACES '(' XmlNamespaceList
> ')' ',' c_expr xmlexists_argument ')'
> {
> TableExpr *n = makeNode(TableExpr);
> n->row_path = $8;
> n->expr = $9;
> n->cols = NIL;
> n->namespaces = $5;
> n->location = @1;
> $$ = (Node *)n;
> }
> | XMLTABLE '(' XMLNAMESPACES '(' XmlNamespaceList
> ')' ',' c_expr xmlexists_argument COLUMNS TableExprColList ')'
> {
> TableExpr *n = makeNode(TableExpr);
> n->row_path = $8;
> n->expr = $9;
> n->cols = $11;
> n->namespaces = $5;
> n->location = @1;
> $$ = (Node *)n;
> }
> ;
>
>
yes, looks my oversight - it is better

> Another thing I did was remove the TableExprColOptionsOpt production; in
> its place I added a third rule in TableExprCol for "ColId Typename
> IsNotNull" (i.e. no options). This seems to reduce the size of the
> generated gram.c a good dozen kB.
>

If I remember well - this was required by better compatibility with Oracle

ANSI SQL: colname type DEFAULT PATH
Oracle: colname PATH DEFAULT

My implementation allows both combinations - there are two reasons: 1. one
less issue when people does port from Oracle, 2. almost all examples of
XMLTABLE on a net are from Oracle - it can be unfriendly, when these
examples would not work on PG - there was discussion about this issue in
this mailing list

>
>
> I didn't like much the use of c_expr in all these productions. As I
> understand it, c_expr is mostly an implementation artifact and we should
> be using a_expr or b_expr almost everywhere. I see that XMLEXISTS
> already expanded the very limited use of c_expr there was; I would
> prefer to fix that one too rather than replicate it here. TBH I'm not
> sure I like that XMLTABLE is re-using xmlexists_argument.
>

There are two situations: c_expr as document content, and c_expr after
DEFAULT and PATH keywords. First probably can be fixed, second not, because
"PATH" is unreserved keyword only.

>
> Actually, is the existing XMLEXISTS production correct? What I see in
> the standard is
>
> <XML table row pattern> ::= <character string literal>
>
> <XML table argument list> ::=
> PASSING <XML table argument passing mechanism> <XML query argument>
> [ { <comma> <XML query argument> }... ]
>
> <XML table argument passing mechanism> ::= <XML passing mechanism>
>
> <XML table column definitions> ::= <XML table column definition> [ {
> <comma> <XML table column definition> }... ]
>
> <XML table column definition> ::=
> <XML table ordinality column definition>
> | <XML table regular column definition>
>
> <XML table ordinality column definition> ::= <column name> FOR ORDINALITY
>
> <XML table regular column definition> ::=
> <column name> <data type> [ <XML passing mechanism> ]
> [ <default clause> ]
> [ PATH <XML table column pattern> ]
>
> <XML table column pattern> ::= <character string literal>
>
> so I think this resolves "PASSING BY {REF,VALUE} <XML query argument>",
> but what
> we have in gram.y is:
>
> /* We allow several variants for SQL and other compatibility. */
> xmlexists_argument:
> PASSING c_expr
> {
> $$ = $2;
> }
> | PASSING c_expr BY REF
> {
> $$ = $2;
> }
> | PASSING BY REF c_expr
> {
> $$ = $4;
> }
> | PASSING BY REF c_expr BY REF
> {
> $$ = $4;
> }
> ;
>
> I'm not sure why we allow "PASSING c_expr" at all. Maybe if BY VALUE/BY
> REF is not specified, we should just not have PASSING at all?
>
>
If we extended this for XMLEXISTS for compatibility with some other
> product, perhaps we should look into what that product supports for
> XMLTABLE; maybe XMLTABLE does not need all the same options as
> XMLEXISTS.
>
>
The reason is a compatibility with other products - DB2. XMLTABLE uses same
options like XMLEXISTS. These options has zero value for Postgres, but its
are important - compatibility, and workable examples.

> The fourth option seems very dubious to me. This was added by commit
> 641459f26, submitted here:
> https://www.postgresql.org/message-id/4C0F6DBF.9000001@mlfowler.com
>
> ... Hm, actually some perusal of the XMLEXISTS predicate in the standard
> shows that it's quite a different thing from XMLTABLE. Maybe we
> shouldn't reuse xmlexists_argument here at all.
>

not sure If I understand

Regards

Pavel

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