Re: patch: function xmltable

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

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.

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.

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

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