Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function

Hi hackers,

Although this is a very small change I figured I'd practice the policy
of outlining your change before you write the code and attempt a patch
submission. Essentially I see the function as a convenience function
that exposes the results of the xpath built in exists() function as a
boolean for easier SQL. The syntax will match the xpath function already
present:

xpath_exists(xpath, xml[, nsarray])

The implementation will check that the xpath value contains
'exists(.....)' and add it if not present for added usability. I can't
blindly wrap the value with exists(...) as exists(exists(.....)) will
always return true in xpath. I've not allocated the oid yet, but will
try to earn my bonus points for getting it close to xpath()'s oid. :)
Appropriate regression tests will be added after I've ensured that 'make
check' is happy I've not regressed anything.

Can I confirm that contrib/xml2 is deprecated and I should be carrying
out my work in backend/utils/adt/xml.c?

I shall be coding it up over the next day or two, work & family permitting!

Thanks,

--
Mike Fowler
Registered Linux user: 379787

On Thu, May 6, 2010 at 10:10 AM, Mike Fowler <mike(at)mlfowler(dot)com> wrote:
> Hi hackers,
>
> Although this is a very small change I figured I'd practice the policy of
> outlining your change before you write the code and attempt a patch
> submission. Essentially I see the function as a convenience function that
> exposes the results of the xpath built in exists() function as a boolean for
> easier SQL. The syntax will match the xpath function already present:
>
> xpath_exists(xpath, xml[, nsarray])
>
> The implementation will check that the xpath value contains 'exists(.....)'
> and add it if not present for added usability. I can't blindly wrap the
> value with exists(...) as exists(exists(.....)) will always return true in
> xpath. I've not allocated the oid yet, but will try to earn my bonus points
> for getting it close to xpath()'s oid. :) Appropriate regression tests will
> be added after I've ensured that 'make check' is happy I've not regressed
> anything.

I'm not sure I understand how this more convenient than just using
xpath() with exists()?

> Can I confirm that contrib/xml2 is deprecated and I should be carrying out
> my work in backend/utils/adt/xml.c?

Yes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Quoting Robert Haas <robertmhaas(at)gmail(dot)com>:

>
> I'm not sure I understand how this more convenient than just using
> xpath() with exists()?
>

It will save a lot of complexity in WHERE clauses. For example using
exists() in xpath() you might construct something like:

WHERE array_dims(xpath('exists(/foo/bar)','<bar><foo/></bar>'::xml) IS
NOT NULL ...

Whereas a dedicated xpath_exists() would look like:

WHERE xpath_exists('/foo/bar','<bar><foo/></bar>'::xml) ....

I accept this example is quite basic, but I hope it illustrates the
added usability. I think xml in sql is complex enough, especially when
you start considering namespaces, that anything we can do to simplify
common use cases can only help improve the uptake of postgres xml.

Thanks,

--
Mike Fowler

On Thu, May 6, 2010 at 5:53 PM, <mike(at)mlfowler(dot)com> wrote:
> Quoting Robert Haas <robertmhaas(at)gmail(dot)com>:
>
>>
>> I'm not sure I understand how this more convenient than just using
>> xpath() with exists()?
>>
>
> It will save a lot of complexity in WHERE clauses. For example using
> exists() in xpath() you might construct something like:
>
> WHERE array_dims(xpath('exists(/foo/bar)','<bar><foo/></bar>'::xml) IS NOT
> NULL ...
>
> Whereas a dedicated xpath_exists() would look like:
>
> WHERE xpath_exists('/foo/bar','<bar><foo/></bar>'::xml) ....
>
> I accept this example is quite basic, but I hope it illustrates the added
> usability. I think xml in sql is complex enough, especially when you start
> considering namespaces, that anything we can do to simplify common use cases
> can only help improve the uptake of postgres xml.

Oh, I see. Well, that might be reasonable syntactic sugar, although I
think you should make it wrap the path in exists() unconditionally,
rather than testing for an existing wrap.

Please email your patch to the list (replying to this email is fine)
and add it here:
https://commitfest.postgresql.org/action/commitfest_view/open

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas wrote:
> Oh, I see. Well, that might be reasonable syntactic sugar, although I
> think you should make it wrap the path in exists() unconditionally,
> rather than testing for an existing wrap.
>

I'll leave it out for now, it saves me some effort after all.

> Please email your patch to the list (replying to this email is fine)
> and add it here:
> https://commitfest.postgresql.org/action/commitfest_view/open
>

Will do once I've finished. Thanks for your feedback.

Regards,

--
Mike Fowler
Registered Linux user: 379787

Robert Haas wrote:
> Please email your patch to the list (replying to this email is fine)
> and add it here:
> https://commitfest.postgresql.org/action/commitfest_view/open
>
Here's my patch, developed against HEAD, that adds the function
'xpath_exists'. The function is a lot simpler than originally thought,
so none of the string manipulation previously discussed was required.
I've also included some regression tests that test the function with and
without xml namespaces. I should note that before I added my tests all
existing tests passed.

One observation that can be made is that I've largely copied the
existing xpath function and altered it to use a different method from
the libxml API. I've done it to save me redoing all the namespace
handling, however it's apparent to me that if we wanted to expose more
of the libxml api we will quickly start having a lot of duplicate code.
I notice that refactoring existing code whilst adding new code is
generally frowned upon, so once this patch is accepted I will look to
refactor the xpath and xpath_exists function. I could even add an
xpath_count method at the same time ;) .

Thanks in advance for any and all feedback,

--
Mike Fowler
Registered Linux user: 379787

"I could be a genius if I just put my mind to it, and I,
I could do anything, if only I could get 'round to it"
-PULP 'Glory Days'

Mike Fowler wrote:
> Robert Haas wrote:
> > Please email your patch to the list (replying to this email is fine)
> > and add it here:
> > https://commitfest.postgresql.org/action/commitfest_view/open
> >
> Here's my patch, developed against HEAD, that adds the function
> 'xpath_exists'. The function is a lot simpler than originally thought,
> so none of the string manipulation previously discussed was required.
> I've also included some regression tests that test the function with and
> without xml namespaces. I should note that before I added my tests all
> existing tests passed.
>
> One observation that can be made is that I've largely copied the
> existing xpath function and altered it to use a different method from
> the libxml API. I've done it to save me redoing all the namespace
> handling, however it's apparent to me that if we wanted to expose more
> of the libxml api we will quickly start having a lot of duplicate code.
> I notice that refactoring existing code whilst adding new code is
> generally frowned upon, so once this patch is accepted I will look to
> refactor the xpath and xpath_exists function. I could even add an
> xpath_count method at the same time ;) .
>
> Thanks in advance for any and all feedback,

I have added this to the next commit-fest:

https://commitfest.postgresql.org/action/commitfest_view?id=6

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ None of us is going to be here forever. +

Bruce Momjian wrote:
> I have added this to the next commit-fest:
>
> https://commitfest.postgresql.org/action/commitfest_view?id=6
>
>
Thanks Bruce. Attached is a revised patch which changes the code
slightly such that it uses an older version of the libxml library. I've
added comments to the code so that we remember why we didn't use the
latest function.

Regards,

--
Mike Fowler
Registered Linux user: 379787

Mike Fowler wrote:
> Bruce Momjian wrote:
>> I have added this to the next commit-fest:
>>
>> https://commitfest.postgresql.org/action/commitfest_view?id=6
> Thanks Bruce. Attached is a revised patch which changes the code
> slightly such that it uses an older version of the libxml library.
> I've added comments to the code so that we remember why we didn't use
> the latest function.

After seeing some other posts in the last couple of days, I realised I
hadn't documented the function in the SGML. I have now done so, and
added a couple of tests with XML literals. Please find the patch
attached. Now time to go correct the xmlexists patch too...

--
Mike Fowler
Registered Linux user: 379787

== Submission review ==

* Is the patch in context diff format?

Yes.

* Does it apply cleanly to the current CVS HEAD?

Yes.

patch -p1 < ../xpath_exists-3.patch
patching file doc/src/sgml/func.sgml
Hunk #1 succeeded at 8642 (offset 16 lines).
patching file src/backend/utils/adt/xml.c
patching file src/include/catalog/pg_proc.h
Hunk #1 succeeded at 4391 (offset 6 lines).
patching file src/include/utils/xml.h
patching file src/test/regress/expected/xml.out
patching file src/test/regress/sql/xml.sql

* Does it include reasonable tests, necessary doc patches, etc?

Tests:

As this is new functionality, it doesn't really need to test
much for interactions with other parts of the system.

I'm not really an XML expert, so I'd like to punt as to
whether it tests enough functionality.

Minor quibble with the regression tests: should we be using
dollar quotes in things like this? Doubled-up quote marks:

SELECT xpath_exists('//town[text() = ''Cwmbran'']','<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);

Dollar quote:

SELECT xpath_exists($$//town[text() = 'Cwmbran']$$,'<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);

Doc patches: Good up to cross-Atlantic differences in spelling
(speciali[sz]ed), e.g.

== Usability review ==

Read what the patch is supposed to do, and consider:

* Does the patch actually implement that?

Yes.

* Do we want that?

Yes.

* Do we already have it?

Not really. There are kludges to accomplish these things, but
they're available mostly in the sense that a general-purpose
language allows you to write code to do anything a Turing machine
can do.

* Does it follow SQL spec, or the community-agreed behavior?

Yes.

* Does it include pg_dump support (if applicable)?

Not applicable.

* Are there dangers?

Not that I can see.

* Have all the bases been covered?

To the extent of my XML knowledge, yes.

== Feature test ==

Apply the patch, compile it and test:

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

Not that I've found. See above re: XML and my vast ignorance of
same.

* Are there any assertion failures or crashes?

No.

== Performance review ==

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

No such claim made. The kludges needed to reproduce the
functionality would certainly consume an enormous number of
developer hours, though.

* Does it slow down other things?

Not that I've found. There might be some minuscule slowing down
of the code to the existence of more code paths, but we're a long,
long way from having that be something other than noise.

== Coding review ==

Read the changes to the code in detail and consider:

* Does it follow the project [http://developer.postgresql.org/pgdocs/postgres/source.html coding guidelines]?

Yes.

* Are there portability issues?

Not that I can see.

* Will it work on Windows/BSD etc?

Should do.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

Yes, subject to, etc.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

== Architecture review ==

Consider the changes to the code in the context of the project as a whole:

* Is everything done in a way that fits together coherently with other features/modules?

Yes.

* Are there interdependencies that can cause problems?

Not that I've found.

Cheers,
David.
On Tue, Jun 29, 2010 at 11:37:28AM +0100, Mike Fowler wrote:
> Mike Fowler wrote:
> >Bruce Momjian wrote:
> >>I have added this to the next commit-fest:
> >>
> >> https://commitfest.postgresql.org/action/commitfest_view?id=6
> >Thanks Bruce. Attached is a revised patch which changes the code
> >slightly such that it uses an older version of the libxml library.
> >I've added comments to the code so that we remember why we didn't
> >use the latest function.
>
> After seeing some other posts in the last couple of days, I realised
> I hadn't documented the function in the SGML. I have now done so,
> and added a couple of tests with XML literals. Please find the patch
> attached. Now time to go correct the xmlexists patch too...
>
> --
> Mike Fowler
> Registered Linux user: 379787
>

> *** a/doc/src/sgml/func.sgml
> --- b/doc/src/sgml/func.sgml
> ***************
> *** 8626,8631 **** SELECT xpath('/my:a/text()', '<my:a xmlns:my="http://example.com">test</my:a>',
> --- 8626,8664 ----
> (1 row)
> ]]></screen>
> </para>
> +
> + <sect3>
> + <title>xpath_exists</title>
> +
> + <indexterm>
> + <primary>xpath_exists</primary>
> + </indexterm>
> +
> + <synopsis>
> + <function>xpath_exists</function>(<replaceable>xpath</replaceable>, <replaceable>xml</replaceable><optional>, <replaceable>nsarray</replaceable></optional>)
> + </synopsis>
> +
> + <para>
> + The function <function>xpath_exists</function> is a specialised form
> + of the <function>xpath</function> function. Though the functions are
> + syntactically the same the xpath expressions are evaluated in differing
> + contexts. Instead of returning the XML values that satisfy the xpath, this
> + function returns a boolean indicating whether the query was satisfied or not.
> + </para>
> +
> + <para>
> + Example:
> + <screen><![CDATA[
> + SELECT xpath_exists('/my:a/text()', '<my:a xmlns:my="http://example.com">test</my:a>',
> + ARRAY[ARRAY['my', 'http://example.com']]);
> +
> + xpath_exists
> + ------------
> + t
> + (1 row)
> + ]]></screen>
> + </para>
> + </sect3>
> </sect2>
>
> <sect2 id="functions-xml-mapping">
> *** a/src/backend/utils/adt/xml.c
> --- b/src/backend/utils/adt/xml.c
> ***************
> *** 3495,3497 **** xpath(PG_FUNCTION_ARGS)
> --- 3495,3681 ----
> return 0;
> #endif
> }
> +
> + /*
> + * Determines if the node specified by the supplied XPath exists
> + * in a given XML document, returning a boolean.
> + *
> + * It is up to the user to ensure that the XML passed is in fact
> + * an XML document - XPath doesn't work easily on fragments without
> + * a context node being known.
> + */
> + Datum
> + xpath_exists(PG_FUNCTION_ARGS)
> + {
> + #ifdef USE_LIBXML
> + text *xpath_expr_text = PG_GETARG_TEXT_P(0);
> + xmltype *data = PG_GETARG_XML_P(1);
> + ArrayType *namespaces = PG_GETARG_ARRAYTYPE_P(2);
> + ArrayBuildState *astate = NULL;
> + xmlParserCtxtPtr ctxt = NULL;
> + xmlDocPtr doc = NULL;
> + xmlXPathContextPtr xpathctx = NULL;
> + xmlXPathCompExprPtr xpathcomp = NULL;
> + xmlXPathObjectPtr xpathobj = NULL;
> + char *datastr;
> + int32 len;
> + int32 xpath_len;
> + xmlChar *string;
> + xmlChar *xpath_expr;
> + int i;
> + int res_nitems;
> + int ndim;
> + Datum *ns_names_uris;
> + bool *ns_names_uris_nulls;
> + int ns_count;
> + int result;
> +
> + /*
> + * Namespace mappings are passed as text[]. If an empty array is passed
> + * (ndim = 0, "0-dimensional"), then there are no namespace mappings.
> + * Else, a 2-dimensional array with length of the second axis being equal
> + * to 2 should be passed, i.e., every subarray contains 2 elements, the
> + * first element defining the name, the second one the URI. Example:
> + * ARRAY[ARRAY['myns', 'http://example.com'], ARRAY['myns2',
> + * 'http://example2.com']].
> + */
> + ndim = ARR_NDIM(namespaces);
> + if (ndim != 0)
> + {
> + int *dims;
> +
> + dims = ARR_DIMS(namespaces);
> +
> + if (ndim != 2 || dims[1] != 2)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_EXCEPTION),
> + errmsg("invalid array for XML namespace mapping"),
> + errdetail("The array must be two-dimensional with length of the second axis equal to 2.")));
> +
> + Assert(ARR_ELEMTYPE(namespaces) == TEXTOID);
> +
> + deconstruct_array(namespaces, TEXTOID, -1, false, 'i',
> + &ns_names_uris, &ns_names_uris_nulls,
> + &ns_count);
> +
> + Assert((ns_count % 2) == 0); /* checked above */
> + ns_count /= 2; /* count pairs only */
> + }
> + else
> + {
> + ns_names_uris = NULL;
> + ns_names_uris_nulls = NULL;
> + ns_count = 0;
> + }
> +
> + datastr = VARDATA(data);
> + len = VARSIZE(data) - VARHDRSZ;
> + xpath_len = VARSIZE(xpath_expr_text) - VARHDRSZ;
> + if (xpath_len == 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_EXCEPTION),
> + errmsg("empty XPath expression")));
> +
> + string = (xmlChar *) palloc((len + 1) * sizeof(xmlChar));
> + memcpy(string, datastr, len);
> + string[len] = '\0';
> +
> + xpath_expr = (xmlChar *) palloc((xpath_len + 1) * sizeof(xmlChar));
> + memcpy(xpath_expr, VARDATA(xpath_expr_text), xpath_len);
> + xpath_expr[xpath_len] = '\0';
> +
> + pg_xml_init();
> + xmlInitParser();
> +
> + PG_TRY();
> + {
> + /*
> + * redundant XML parsing (two parsings for the same value during one
> + * command execution are possible)
> + */
> + ctxt = xmlNewParserCtxt();
> + if (ctxt == NULL)
> + xml_ereport(ERROR, ERRCODE_OUT_OF_MEMORY,
> + "could not allocate parser context");
> + doc = xmlCtxtReadMemory(ctxt, (char *) string, len, NULL, NULL, 0);
> + if (doc == NULL)
> + xml_ereport(ERROR, ERRCODE_INVALID_XML_DOCUMENT,
> + "could not parse XML document");
> + xpathctx = xmlXPathNewContext(doc);
> + if (xpathctx == NULL)
> + xml_ereport(ERROR, ERRCODE_OUT_OF_MEMORY,
> + "could not allocate XPath context");
> + xpathctx->node = xmlDocGetRootElement(doc);
> + if (xpathctx->node == NULL)
> + xml_ereport(ERROR, ERRCODE_INTERNAL_ERROR,
> + "could not find root XML element");
> +
> + /* register namespaces, if any */
> + if (ns_count > 0)
> + {
> + for (i = 0; i < ns_count; i++)
> + {
> + char *ns_name;
> + char *ns_uri;
> +
> + if (ns_names_uris_nulls[i * 2] ||
> + ns_names_uris_nulls[i * 2 + 1])
> + ereport(ERROR,
> + (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
> + errmsg("neither namespace name nor URI may be null")));
> + ns_name = TextDatumGetCString(ns_names_uris[i * 2]);
> + ns_uri = TextDatumGetCString(ns_names_uris[i * 2 + 1]);
> + if (xmlXPathRegisterNs(xpathctx,
> + (xmlChar *) ns_name,
> + (xmlChar *) ns_uri) != 0)
> + ereport(ERROR, /* is this an internal error??? */
> + (errmsg("could not register XML namespace with name \"%s\" and URI \"%s\"",
> + ns_name, ns_uri)));
> + }
> + }
> +
> + xpathcomp = xmlXPathCompile(xpath_expr);
> + if (xpathcomp == NULL) /* TODO: show proper XPath error details */
> + xml_ereport(ERROR, ERRCODE_INTERNAL_ERROR,
> + "invalid XPath expression");
> +
> + /* Version 2.6.27 introduces a function named xmlXPathCompiledEvalToBoolean
> + * however we can derive the existence by whether any nodes are returned
> + * thereby preventing a library version upgrade */
> + xpathobj = xmlXPathCompiledEval(xpathcomp, xpathctx);
> + if (xpathobj == NULL) /* TODO: reason? */
> + xml_ereport(ERROR, ERRCODE_INTERNAL_ERROR,
> + "could not create XPath object");
> +
> + if (xpathobj->nodesetval == NULL)
> + result = 0;
> + else
> + result = xpathobj->nodesetval->nodeNr;
> + }
> + PG_CATCH();
> + {
> + if (xpathobj)
> + xmlXPathFreeObject(xpathobj);
> + if (xpathcomp)
> + xmlXPathFreeCompExpr(xpathcomp);
> + if (xpathctx)
> + xmlXPathFreeContext(xpathctx);
> + if (doc)
> + xmlFreeDoc(doc);
> + if (ctxt)
> + xmlFreeParserCtxt(ctxt);
> + PG_RE_THROW();
> + }
> + PG_END_TRY();
> +
> + xmlXPathFreeCompExpr(xpathcomp);
> + xmlXPathFreeContext(xpathctx);
> + xmlFreeDoc(doc);
> + xmlFreeParserCtxt(ctxt);
> +
> + PG_RETURN_BOOL(result);
> + #else
> + NO_XML_SUPPORT();
> + return 0;
> + #endif
> + }
> *** a/src/include/catalog/pg_proc.h
> --- b/src/include/catalog/pg_proc.h
> ***************
> *** 4385,4390 **** DESCR("evaluate XPath expression, with namespaces support");
> --- 4385,4395 ----
> DATA(insert OID = 2932 ( xpath PGNSP PGUID 14 1 0 0 f f f t f i 2 0 143 "25 142" _null_ _null_ _null_ _null_ "select pg_catalog.xpath($1, $2, ''{}''::pg_catalog.text[])" _null_ _null_ _null_ ));
> DESCR("evaluate XPath expression");
>
> + DATA(insert OID = 3037 ( xpath_exists PGNSP PGUID 12 1 0 0 f f f t f i 3 0 16 "25 142 1009" _null_ _null_ _null_ _null_ xpath_exists _null_ _null_ _null_ ));
> + DESCR("evaluate XPath expression in a boolean context, with namespaces support");
> + DATA(insert OID = 3038 ( xpath_exists PGNSP PGUID 14 1 0 0 f f f t f i 2 0 16 "25 142" _null_ _null_ _null_ _null_ "select pg_catalog.xpath_exists($1, $2, ''{}''::pg_catalog.text[])" _null_ _null_ _null_ ));
> + DESCR("evaluate XPath expression in a boolean context");
> +
> /* uuid */
> DATA(insert OID = 2952 ( uuid_in PGNSP PGUID 12 1 0 0 f f f t f i 1 0 2950 "2275" _null_ _null_ _null_ _null_ uuid_in _null_ _null_ _null_ ));
> DESCR("I/O");
> *** a/src/include/utils/xml.h
> --- b/src/include/utils/xml.h
> ***************
> *** 37,42 **** extern Datum texttoxml(PG_FUNCTION_ARGS);
> --- 37,43 ----
> extern Datum xmltotext(PG_FUNCTION_ARGS);
> extern Datum xmlvalidate(PG_FUNCTION_ARGS);
> extern Datum xpath(PG_FUNCTION_ARGS);
> + extern Datum xpath_exists(PG_FUNCTION_ARGS);
>
> extern Datum table_to_xml(PG_FUNCTION_ARGS);
> extern Datum query_to_xml(PG_FUNCTION_ARGS);
> *** a/src/test/regress/expected/xml.out
> --- b/src/test/regress/expected/xml.out
> ***************
> *** 502,504 **** SELECT xpath('//b', '<a>one <b>two</b> three <b>etc</b></a>');
> --- 502,557 ----
> {<b>two</b>,<b>etc</b>}
> (1 row)
>
> + -- Test xpath_exists evaluation
> + SELECT xpath_exists('//town[text() = ''Toronto'']','<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);
> + xpath_exists
> + --------------
> + f
> + (1 row)
> +
> + SELECT xpath_exists('//town[text() = ''Cwmbran'']','<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);
> + xpath_exists
> + --------------
> + t
> + (1 row)
> +
> + INSERT INTO xmltest VALUES (4, '<menu><beers><name>Budvar</name><cost>free</cost><name>Carling</name><cost>lots</cost></beers></menu>'::xml);
> + INSERT INTO xmltest VALUES (5, '<menu><beers><name>Molson</name><cost>free</cost><name>Carling</name><cost>lots</cost></beers></menu>'::xml);
> + INSERT INTO xmltest VALUES (6, '<myns:menu xmlns:myns="http://myns.com"><myns:beers><myns:name>Budvar</myns:name><myns:cost>free</myns:cost><myns:name>Carling</myns:name><myns:cost>lots</myns:cost></myns:beers></myns:menu>'::xml);
> + INSERT INTO xmltest VALUES (7, '<myns:menu xmlns:myns="http://myns.com"><myns:beers><myns:name>Molson</myns:name><myns:cost>free</myns:cost><myns:name>Carling</myns:name><myns:cost>lots</myns:cost></myns:beers></myns:menu>'::xml);
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/menu/beer',data);
> + count
> + -------
> + 0
> + (1 row)
> +
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/menu/beers',data);
> + count
> + -------
> + 2
> + (1 row)
> +
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/menu/beers/name[text() = ''Molson'']',data);
> + count
> + -------
> + 1
> + (1 row)
> +
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/myns:menu/myns:beer',data,ARRAY[ARRAY['myns','http://myns.com']]);
> + count
> + -------
> + 0
> + (1 row)
> +
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/myns:menu/myns:beers',data,ARRAY[ARRAY['myns','http://myns.com']]);
> + count
> + -------
> + 2
> + (1 row)
> +
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/myns:menu/myns:beers/myns:name[text() = ''Molson'']',data,ARRAY[ARRAY['myns','http://myns.com']]);
> + count
> + -------
> + 1
> + (1 row)
> +
> *** a/src/test/regress/sql/xml.sql
> --- b/src/test/regress/sql/xml.sql
> ***************
> *** 163,165 **** SELECT xpath('', '<!-- error -->');
> --- 163,181 ----
> SELECT xpath('//text()', '<local:data xmlns:local="http://127.0.0.1"><local:piece id="1">number one</local:piece><local:piece id="2" /></local:data>');
> SELECT xpath('//loc:piece/@id', '<local:data xmlns:local="http://127.0.0.1"><local:piece id="1">number one</local:piece><local:piece id="2" /></local:data>', ARRAY[ARRAY['loc', 'http://127.0.0.1']]);
> SELECT xpath('//b', '<a>one <b>two</b> three <b>etc</b></a>');
> +
> + -- Test xpath_exists evaluation
> + SELECT xpath_exists('//town[text() = ''Toronto'']','<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);
> + SELECT xpath_exists('//town[text() = ''Cwmbran'']','<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);
> +
> + INSERT INTO xmltest VALUES (4, '<menu><beers><name>Budvar</name><cost>free</cost><name>Carling</name><cost>lots</cost></beers></menu>'::xml);
> + INSERT INTO xmltest VALUES (5, '<menu><beers><name>Molson</name><cost>free</cost><name>Carling</name><cost>lots</cost></beers></menu>'::xml);
> + INSERT INTO xmltest VALUES (6, '<myns:menu xmlns:myns="http://myns.com"><myns:beers><myns:name>Budvar</myns:name><myns:cost>free</myns:cost><myns:name>Carling</myns:name><myns:cost>lots</myns:cost></myns:beers></myns:menu>'::xml);
> + INSERT INTO xmltest VALUES (7, '<myns:menu xmlns:myns="http://myns.com"><myns:beers><myns:name>Molson</myns:name><myns:cost>free</myns:cost><myns:name>Carling</myns:name><myns:cost>lots</myns:cost></myns:beers></myns:menu>'::xml);
> +
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/menu/beer',data);
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/menu/beers',data);
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/menu/beers/name[text() = ''Molson'']',data);
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/myns:menu/myns:beer',data,ARRAY[ARRAY['myns','http://myns.com']]);
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/myns:menu/myns:beers',data,ARRAY[ARRAY['myns','http://myns.com']]);
> + SELECT COUNT(id) FROM xmltest WHERE xpath_exists('/myns:menu/myns:beers/myns:name[text() = ''Molson'']',data,ARRAY[ARRAY['myns','http://myns.com']]);

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Jul 27, 2010 at 7:33 PM, David Fetter <david(at)fetter(dot)org> wrote:
>        Minor quibble with the regression tests: should we be using
>        dollar quotes in things like this?  Doubled-up quote marks:
>
>        SELECT xpath_exists('//town[text() = ''Cwmbran'']','<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);
>
>        Dollar quote:
>
>        SELECT xpath_exists($$//town[text() = 'Cwmbran']$$,'<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);

Personally, I don't really see that as an improvement. Dollar-quotes
are really nice for longer strings, or where you would otherwise have
quadrupled quotes (or more), but I don't see a big advantage to it
here. Still, it's a question of opinion more than anything else.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Tue, 2010-07-27 at 19:41 -0400, Robert Haas wrote:
> On Tue, Jul 27, 2010 at 7:33 PM, David Fetter <david(at)fetter(dot)org> wrote:
> > Minor quibble with the regression tests: should we be using
> > dollar quotes in things like this? Doubled-up quote marks:
> >
> > SELECT xpath_exists('//town[text() = ''Cwmbran'']','<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);
> >
> > Dollar quote:
> >
> > SELECT xpath_exists($$//town[text() = 'Cwmbran']$$,'<towns><town>Bidford-on-Avon</town><town>Cwmbran</town><town>Bristol</town></towns>'::xml);
>
> Personally, I don't really see that as an improvement. Dollar-quotes
> are really nice for longer strings, or where you would otherwise have
> quadrupled quotes (or more), but I don't see a big advantage to it
> here. Still, it's a question of opinion more than anything else.

I like the idea of using dollar quotes, but I think they should be used
for both arguments (or neither). Using $$ for one and then shifting to
"'" for the second argument with no whitespace at all seems like the
least readable option.

Regards,
Jeff Davis

Excerpts from Mike Fowler's message of mar jun 29 06:37:28 -0400 2010:

> After seeing some other posts in the last couple of days, I realised I
> hadn't documented the function in the SGML. I have now done so, and
> added a couple of tests with XML literals. Please find the patch
> attached. Now time to go correct the xmlexists patch too...

Hmm, is 0 a valid node number in a xmlNodeSet? If it is, searching for
the zeroth node would make the code call PG_RETURN_BOOL(0), no?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On tis, 2010-07-27 at 16:33 -0700, David Fetter wrote:
> * Do we already have it?
>
> Not really. There are kludges to accomplish these things, but
> they're available mostly in the sense that a general-purpose
> language allows you to write code to do anything a Turing machine
> can do.

I think this has been obsoleted by the xmlexists patch.

On 06/08/10 05:38, Peter Eisentraut wrote:
> On tis, 2010-07-27 at 16:33 -0700, David Fetter wrote:
>
>> * Do we already have it?
>>
>> Not really. There are kludges to accomplish these things, but
>> they're available mostly in the sense that a general-purpose
>> language allows you to write code to do anything a Turing machine
>> can do.
>>
> I think this has been obsoleted by the xmlexists patch

In many ways yes. The only surviving difference is that xpath_exists has
support for namespaces and xmlexists does not as the grammar expects
namespaces to be handled in the xquery. So if people expect namespace
support to be useful that having both functions is useful until I (or
someone who works faster than me) get xquery going.

If the patch is to be committed, does it make sense for me to refine it
such that it uses the new xpath internal function you extracted in the
xmlexists patch?

Regards,

--
Mike Fowler
Registered Linux user: 379787

On fre, 2010-08-06 at 09:04 +0100, Mike Fowler wrote:
> If the patch is to be committed, does it make sense for me to refine
> it such that it uses the new xpath internal function you extracted in
> the xmlexists patch?

Yes, you can probably shrink this patch down to about 20 lines.

On 06/08/10 20:55, Peter Eisentraut wrote:
> On fre, 2010-08-06 at 09:04 +0100, Mike Fowler wrote:
>> If the patch is to be committed, does it make sense for me to refine
>> it such that it uses the new xpath internal function you extracted in
>> the xmlexists patch?
>
> Yes, you can probably shrink this patch down to about 20 lines.
>

Updated the patch so that it will apply to head and re-worked the
function to use the new xpath internal function.

Regards,

--
Mike Fowler
Registered Linux user: 379787

Mike Fowler <mike(at)mlfowler(dot)com> writes:
> On 06/08/10 20:55, Peter Eisentraut wrote:
>> On fre, 2010-08-06 at 09:04 +0100, Mike Fowler wrote:
>>> If the patch is to be committed, does it make sense for me to refine
>>> it such that it uses the new xpath internal function you extracted in
>>> the xmlexists patch?
>>
>> Yes, you can probably shrink this patch down to about 20 lines.

> Updated the patch so that it will apply to head and re-worked the
> function to use the new xpath internal function.

Applied with minor corrections (improved docs, fixed regression tests,
adjusted OIDs for CVS HEAD).

regards, tom lane