Re: Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function
Mike Fowler m...@mlfowler.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function
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
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 8693,8698 SELECT xpath('//mydefns:b/text()', 'a xmlns=http://example.com;btest/b/a
--- 8693,8731
(1 row)
]]/screen
/para
+
+sect3
+ titlexpath_exists/title
+
+ indexterm
+ primaryxpath_exists/primary
+ /indexterm
+
+ synopsis
+ functionxpath_exists/function(replaceablexpath/replaceable, replaceablexml/replaceableoptional, replaceablensarray/replaceable/optional)
+ /synopsis
+
+ para
+ The function functionxpath_exists/function is a specialised form
+ of the functionxpath/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
***
*** 3541,3543 Datum xmlexists(PG_FUNCTION_ARGS)
--- 3541,3567
return 0;
#endif
}
+
+ /*
+ * Determines if the node specified by the supplied XPath exists
+ * in a given XML document, returning a boolean. Differs from
+ * xmlexists as it supports namespaces and is not defined in SQL/XML.
+ */
+ 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);
+ int res_nitems;
+
+ xpath_internal(xpath_expr_text, data, namespaces,
+ res_nitems, NULL);
+
+ PG_RETURN_BOOL(res_nitems 0);
+ #else
+ NO_XML_SUPPORT();
+ return 0;
+ #endif
+ }
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***
*** 4390,4395 DESCR(evaluate XPath expression);
--- 4390,4400
DATA(insert OID = 2614 ( xmlexists PGNSP PGUID 12 1 0 0 f f f t f i 2 0 16 25 142 _null_ _null_ _null_ _null_ xmlexists _null_ _null_ _null_ ));
DESCR(test XML value against 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 xmlexists(PG_FUNCTION_ARGS);
extern Datum table_to_xml(PG_FUNCTION_ARGS);
*** a/src/test/regress/expected/xml.out
--- b/src/test/regress/expected/xml.out
***
*** 502,507 SELECT xpath('//b', 'aone btwo/b three betc/b/a');
--- 502,560
{btwo/b,betc/b}
(1 row)
+ -- Test xpath_exists evaluation
+ SELECT xpath_exists('//town[text() = ''Toronto'']','townstownBidford-on-Avon/towntownCwmbran/towntownBristol/town/towns'::xml);
+ xpath_exists
+ --
+ f
+ (1 row)
+
+ SELECT xpath_exists('//town[text() = ''Cwmbran'']','townstownBidford-on-Avon/towntownCwmbran/towntownBristol/town/towns'::xml);
+ xpath_exists
+ --
+ t
+ (1 row)
+
+ INSERT INTO xmltest VALUES (4, 'menubeersnameBudvar/namecostfree/costnameCarling/namecostlots/cost/beers/menu'::xml);
+ INSERT INTO xmltest VALUES (5, 'menubeersnameMolson/namecostfree/costnameCarling/namecostlots/cost/beers/menu'::xml);
+ INSERT INTO xmltest VALUES (6, 'myns:menu
Re: Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function
== 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'']','townstownBidford-on-Avon/towntownCwmbran/towntownBristol/town/towns'::xml);
Dollar quote:
SELECT xpath_exists($$//town[text() =
'Cwmbran']$$,'townstownBidford-on-Avon/towntownCwmbran/towntownBristol/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
+ titlexpath_exists/title
+
+ indexterm
+ primaryxpath_exists/primary
+ /indexterm
+
+ synopsis
+ functionxpath_exists/function(replaceablexpath/replaceable,
replaceablexml/replaceableoptional,
Re: Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function
On Tue, Jul 27, 2010 at 7:33 PM, David Fetter da...@fetter.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'']','townstownBidford-on-Avon/towntownCwmbran/towntownBristol/town/towns'::xml);
Dollar quote:
SELECT xpath_exists($$//town[text() =
'Cwmbran']$$,'townstownBidford-on-Avon/towntownCwmbran/towntownBristol/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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [RRR] Review: Re: [PATCH] Re: [HACKERS] Adding xpath_exists function
On Tue, 2010-07-27 at 19:41 -0400, Robert Haas wrote:
On Tue, Jul 27, 2010 at 7:33 PM, David Fetter da...@fetter.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'']','townstownBidford-on-Avon/towntownCwmbran/towntownBristol/town/towns'::xml);
Dollar quote:
SELECT xpath_exists($$//town[text() =
'Cwmbran']$$,'townstownBidford-on-Avon/towntownCwmbran/towntownBristol/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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
