Re: patch: function xmltable

2016-11-28 23:34 GMT+01:00 Alvaro Herrera <alvherre@2ndquadrant.com>:

> Pavel Stehule wrote:
>
> > Here is updated patch without default namespace support (and without
> XPath
> > expression transformation).
> >
> > Due last changes in parser
> > https://github.com/postgres/postgres/commit/
> 906bfcad7ba7cb3863fe0e2a7810be8e3cd84fbd
> > I had to use c_expr on other positions ( xmlnamespace definition).
> >
> > I don't think it is limit - in 99% there will be const literal.
>
> Argh.  I can't avoid the feeling that I'm missing some parser trickery
> here.  We have the XMLNAMESPACES keyword and the clause-terminating
> comma to protect these clauses, there must be a way to define this piece
> of the grammar so that there's no conflict, without losing the freedom
> in the expressions.  But I don't see how.  Now I agree that xml
> namespace definitions are going to be string literals in almost all
> cases (or in extra sophisticated cases, column names) ... it's probably
> better to spend the bison-fu in the document expression or the column
> options, or better yet the xmlexists_argument stuff.  But I don't see
> possibility of improvements in any of those places, so let's put it
> aside -- we can improve later, if need arises.
>

The problem is in unreserved keyword "PASSING" probably.


>
> In any case, it looks like we can change c_expr to b_expr in a few
> places, which is good because then operators work (in particular, unless
> I misread the grammar, foo||bar doesn't work with c_expr and does work
> with b_expr, which seems the most useful in this case).  Also, it makes
> no sense to support (in the namespaces clause) DEFAULT a_expr if the
> IDENT case uses only b_expr, so let's reduce both to just b_expr.
>

I changed all what was possible to b_expr.


>
> While I'm looking at node definitions, I see a few things that could use
> some naming improvement.  For example, "expr" for TableExpr is a bit
> unexpressive.  We could use "document_expr" there, perhaps.  "row_path"
> seems fixated on the XML case and the expression be path; let's use
> "row_expr" there.  And "cols" could be "column_exprs" perhaps.  (All
> those renames cause fall-out in various node-related files, so let's
> think carefully to avoid renaming them multiple times.)
>

Columns is not only expr - list - so I renamed it to "columns". Other
renamed like you proposed


>
> In primnodes, you kept the comment that says "xpath".  Please update
> that to not-just-XML reality.
>

fixed


>
> Please fix the comment in XmlTableAddNs; NULL is no longer a valid value.
>

fixed


>
> parse_expr.c has two unused variables; please remove them.
>

fixed


>
> This test in ExecEvalTableExprProtected looks weird:
>         if (i != tstate->for_ordinality_col - 1)
> please change to comparing "i + 1" (convert array index into attribute
> number), and invert the boolean expression, leaving the for_ordinality
> case on top and the rest in the "else".  That seems easier to read.
> Also, we customarily use post-increment (rownum++) instead of pre-incr.
>
>
fiexed


> In execQual.c I think it's neater to have ExecEvalTableExpr go before
> its subroutine.  Actually, I wonder whether it is really necessary to
> have a subroutine in the first place; you could just move the entire
> contents of that subroutine to within the PG_TRY block instead.  The
> only thing you lose is one indentation level.  I'm not sure about this
> one, but it's worth considering.
>

done


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

Regards

Pavel