Re: patch: function xmltable

2016-11-19 5:19 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 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.
>

It is not possible PASSING is unreserved keyword too.

Regards

Pavel

>
>
>>
>> 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
>>
>
>