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. 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. 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.) In primnodes, you kept the comment that says "xpath". Please update that to not-just-XML reality. Please fix the comment in XmlTableAddNs; NULL is no longer a valid value. parse_expr.c has two unused variables; please remove them. 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. 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. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers