Re: [PATCH] Re: Adding XMLEXISTS to the grammar

On tis, 2010-06-29 at 12:22 +0100, Mike Fowler wrote:
> Mike Fowler wrote:
> > Thanks again for your help Robert, turns out the fault was in the
> > pg_proc entry (the 3 up there should've been a two!). Once I took the
> > grammar out it was quickly obvious where I'd gone wrong.
> >
> > Attached is a patch with the revised XMLEXISTS function, complete with
> > grammar support and regression tests. The implemented grammar is:
> >
> > XMLEXISTS ( xpath_expression PASSING BY REF xml_value [BY REF] )
> >
> > Though the full grammar makes everything after the xpath_expression
> > optional, I've left it has mandatory simply to avoid lots of rework of
> > the function (would need new null checks, memory handling would need
> > reworking).
> >
>
> As with the xpath_exists patch I've now added the SGML documentation
> detailing this function and extended the regression test a little to
> test XML literals.

Some thoughts, mostly nitpicks:

The snippet of documentation could be clearer. It says "if the xml
satisifies the xpath". Not sure what that means exactly. An XPath
expression, by definition, returns a value. How is that value used to
determine the result?

Naming of parser symbols: xmlexists_list isn't actually a list of
xmlexists's. That particular rule can probably be done away with anyway
and the code be put directly into the XMLEXISTS rule.

Why is the first argument AexprConst instead of a_expr? The SQL
standard says it's a character string literal, but I think we can very
well allow arbitrary expressions.

xmlexists_query_argument_list should be optional.

The rules xml_default_passing_mechanism and xml_passing_mechanism are
pretty useless to have a separate rules. Just mention the tokens where
they are used.

Why c_expr?

Call the C-level function xmlexists for consistency.