Re: xpath_array with namespaces support

As a result of discussion with Peter, I provide modified patch for
xpath_array() with namespaces support.

The signature is:
_xml xpath_array(text xpathQuery, xml xmlValue[, _text namespacesBindings])

The third argument is 2-dimensional array defining bindings for
namespaces. Simple examples:

xmltest=# SELECT xpath_array('//text()', '<local:data
xmlns:local="http://127.0.0.1"><local:piece id="1">number
one</local:piece><local:piece id="2" /></local:data>');
xpath_array
----------------
{"number one"}
(1 row)

xmltest=# SELECT xpath_array('//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'], ARRAY['http://127.0.0.1']]);
xpath_array
-------------
{1,2}
(1 row)

Thoughts regarding other XPath functions were exposed a couple of days
ago: http://archives.postgresql.org/pgsql-patches/2007-02/msg00373.php

If there is no objections, we could call the function provided in this
patch as xpath() or xmlpath() (the latter is similar to SQL/XML
functions).

Also, maybe someone can suggest better approach for passing namespace
bindings (more convenient than ARRAY[ARRAY[...], ARRAY[...]])?

--
Best regards,
Nikolay

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Nikolay Samokhvalov wrote:
> As a result of discussion with Peter, I provide modified patch for
> xpath_array() with namespaces support.
>
> The signature is:
> _xml xpath_array(text xpathQuery, xml xmlValue[, _text namespacesBindings])
>
> The third argument is 2-dimensional array defining bindings for
> namespaces. Simple examples:
>
> xmltest=# SELECT xpath_array('//text()', '<local:data
> xmlns:local="http://127.0.0.1"><local:piece id="1">number
> one</local:piece><local:piece id="2" /></local:data>');
> xpath_array
> ----------------
> {"number one"}
> (1 row)
>
> xmltest=# SELECT xpath_array('//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'], ARRAY['http://127.0.0.1']]);
> xpath_array
> -------------
> {1,2}
> (1 row)
>
> Thoughts regarding other XPath functions were exposed a couple of days
> ago: http://archives.postgresql.org/pgsql-patches/2007-02/msg00373.php
>
> If there is no objections, we could call the function provided in this
> patch as xpath() or xmlpath() (the latter is similar to SQL/XML
> functions).
>
> Also, maybe someone can suggest better approach for passing namespace
> bindings (more convenient than ARRAY[ARRAY[...], ARRAY[...]])?
>
> --
> Best regards,
> Nikolay

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org

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

+ If your life is a hard drive, Christ can be your backup. +

I tried this patch bug found this regression failure:

-- Considering only built-in procs (prolang = 12), look for multiple uses
-- of the same internal function (ie, matching prosrc fields). It's OK to
-- have several entries with different pronames for the same internal function,
-- but conflicts in the number of arguments and other critical items should
-- be complained of. (We don't check data types here; see next query.)
-- Note: ignore aggregate functions here, since they all point to the same
-- dummy built-in function.
SELECT p1.oid, p1.proname, p2.oid, p2.proname
FROM pg_proc AS p1, pg_proc AS p2
WHERE p1.oid < p2.oid AND
p1.prosrc = p2.prosrc AND
p1.prolang = 12 AND p2.prolang = 12 AND
(p1.proisagg = false OR p2.proisagg = false) AND
(p1.prolang != p2.prolang OR
p1.proisagg != p2.proisagg OR
p1.prosecdef != p2.prosecdef OR
p1.proisstrict != p2.proisstrict OR
p1.proretset != p2.proretset OR
p1.provolatile != p2.provolatile OR
p1.pronargs != p2.pronargs);
oid | proname | oid | proname
------+-------------+------+-------------
2931 | xpath_array | 2932 | xpath_array
(1 row)

This is because you are calling xpath_array with 2 and 3 arguments.
Seems we don't do this anywhere else.

I also had to add a #ifdef USE_LIBXML around xml_xmlnodetotext(). Please
research a fix to this an resubmit. Thanks.

---------------------------------------------------------------------------

Nikolay Samokhvalov wrote:
> As a result of discussion with Peter, I provide modified patch for
> xpath_array() with namespaces support.
>
> The signature is:
> _xml xpath_array(text xpathQuery, xml xmlValue[, _text namespacesBindings])
>
> The third argument is 2-dimensional array defining bindings for
> namespaces. Simple examples:
>
> xmltest=# SELECT xpath_array('//text()', '<local:data
> xmlns:local="http://127.0.0.1"><local:piece id="1">number
> one</local:piece><local:piece id="2" /></local:data>');
> xpath_array
> ----------------
> {"number one"}
> (1 row)
>
> xmltest=# SELECT xpath_array('//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'], ARRAY['http://127.0.0.1']]);
> xpath_array
> -------------
> {1,2}
> (1 row)
>
> Thoughts regarding other XPath functions were exposed a couple of days
> ago: http://archives.postgresql.org/pgsql-patches/2007-02/msg00373.php
>
> If there is no objections, we could call the function provided in this
> patch as xpath() or xmlpath() (the latter is similar to SQL/XML
> functions).
>
> Also, maybe someone can suggest better approach for passing namespace
> bindings (more convenient than ARRAY[ARRAY[...], ARRAY[...]])?
>
> --
> Best regards,
> Nikolay

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org

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

+ If your life is a hard drive, Christ can be your backup. +

On 3/3/07, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> I tried this patch bug found this regression failure:
>
> -- Considering only built-in procs (prolang = 12), look for multiple uses
> -- of the same internal function (ie, matching prosrc fields). It's OK to
> -- have several entries with different pronames for the same internal function,
> -- but conflicts in the number of arguments and other critical items should
> -- be complained of. (We don't check data types here; see next query.)
> -- Note: ignore aggregate functions here, since they all point to the same
> -- dummy built-in function.
> SELECT p1.oid, p1.proname, p2.oid, p2.proname
> FROM pg_proc AS p1, pg_proc AS p2
> WHERE p1.oid < p2.oid AND
> p1.prosrc = p2.prosrc AND
> p1.prolang = 12 AND p2.prolang = 12 AND
> (p1.proisagg = false OR p2.proisagg = false) AND
> (p1.prolang != p2.prolang OR
> p1.proisagg != p2.proisagg OR
> p1.prosecdef != p2.prosecdef OR
> p1.proisstrict != p2.proisstrict OR
> p1.proretset != p2.proretset OR
> p1.provolatile != p2.provolatile OR
> p1.pronargs != p2.pronargs);
> oid | proname | oid | proname
> ------+-------------+------+-------------
> 2931 | xpath_array | 2932 | xpath_array
> (1 row)
>
> This is because you are calling xpath_array with 2 and 3 arguments.
> Seems we don't do this anywhere else.
>
> I also had to add a #ifdef USE_LIBXML around xml_xmlnodetotext(). Please
> research a fix to this an resubmit. Thanks.

OK.
I'll fix these issues and extend the patch with resgression tests and
docs for xpath_array(). I'll resubmit it very soon.

--
Best regards,
Nikolay

On 3/4/07, Nikolay Samokhvalov <nikolay(at)samokhvalov(dot)com> wrote:
> I'll fix these issues and extend the patch with resgression tests and
> docs for xpath_array(). I'll resubmit it very soon.

Here is a new version of the patch. I didn't change any part of docs yet.
Since there were no objections I've changed the name of the function
to xmlpath().

--
Best regards,
Nikolay

What about it? W/o this not large patch XML functionality in 8.3 will be weak...
Will it be accepted?

On 3/5/07, Nikolay Samokhvalov <samokhvalov(at)gmail(dot)com> wrote:
> On 3/4/07, Nikolay Samokhvalov <nikolay(at)samokhvalov(dot)com> wrote:
> > I'll fix these issues and extend the patch with resgression tests and
> > docs for xpath_array(). I'll resubmit it very soon.
>
> Here is a new version of the patch. I didn't change any part of docs yet.
> Since there were no objections I've changed the name of the function
> to xmlpath().
>

--
Best regards,
Nikolay

Nikolay Samokhvalov wrote:
> What about it? W/o this not large patch XML functionality in 8.3 will
> be weak...
> Will it be accepted?
>
>
In principle I am in favor of the patch.

Would it be better to use some more unlikely name for the dummy root
element used to process fragments than <x> ?

Perhaps even something in a special namespace?

cheers

andrew

On 3/17/07, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> In principle I am in favor of the patch.
>
> Would it be better to use some more unlikely name for the dummy root
> element used to process fragments than <x> ?
>
> Perhaps even something in a special namespace?
>

I did think about it, but I didn't find any difficulties with simple
<x>...</x>. The thing is that regardless the element name we have
corresponding shift in XPath epression -- so, there cannot be any
problem from my point of view... But maybe I don't see something and
it's better to avoid _possible_ problem. It depends on PostgreSQL code
style itself -- what is the best approach in such cases? To avoid
unknown possible difficulties or to be clear?

--
Best regards,
Nikolay

Nikolay Samokhvalov wrote:
> On 3/17/07, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> In principle I am in favor of the patch.
>>
>> Would it be better to use some more unlikely name for the dummy root
>> element used to process fragments than <x> ?
>>
>> Perhaps even something in a special namespace?
>>
>
> I did think about it, but I didn't find any difficulties with simple
> <x>...</x>. The thing is that regardless the element name we have
> corresponding shift in XPath epression -- so, there cannot be any
> problem from my point of view... But maybe I don't see something and
> it's better to avoid _possible_ problem. It depends on PostgreSQL code
> style itself -- what is the best approach in such cases? To avoid
> unknown possible difficulties or to be clear?
>

If you are sure that it won't cause a problem then I think it's ok to
leave it, as long as there is a comment in the code that says why we are
sure it's ok.

cheers

andrew

On 3/5/07, Nikolay Samokhvalov <samokhvalov(at)gmail(dot)com> wrote:
> On 3/4/07, Nikolay Samokhvalov <nikolay(at)samokhvalov(dot)com> wrote:
> > I'll fix these issues and extend the patch with resgression tests and
> > docs for xpath_array(). I'll resubmit it very soon.
>
> Here is a new version of the patch. I didn't change any part of docs yet.
> Since there were no objections I've changed the name of the function
> to xmlpath().

Updated version of the patch contains bugfix: there were a problem
with path queries that pointed to elements (cases when a set of
document parts that correspond to subtrees should be returned).
Example is (included in regression test):

xmltest=# SELECT xmlpath('//b', '<a>one <b>two</b> three <b>etc</b></a>');
xmlpath
-------------------------
{<b>two</b>,<b>etc</b>}
(1 row)

Waiting for more feedback, please check it.

--
Best regards,
Nikolay

Patch applied.

Please provide a documentation addition. Thanks.

---------------------------------------------------------------------------

Nikolay Samokhvalov wrote:
> On 3/4/07, Nikolay Samokhvalov <nikolay(at)samokhvalov(dot)com> wrote:
> > I'll fix these issues and extend the patch with resgression tests and
> > docs for xpath_array(). I'll resubmit it very soon.
>
> Here is a new version of the patch. I didn't change any part of docs yet.
> Since there were no objections I've changed the name of the function
> to xmlpath().
>
> --
> Best regards,
> Nikolay

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

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

+ If your life is a hard drive, Christ can be your backup. +

Applying newest version of this patch now; still needs documentation.

---------------------------------------------------------------------------

Nikolay Samokhvalov wrote:
> On 3/5/07, Nikolay Samokhvalov <samokhvalov(at)gmail(dot)com> wrote:
> > On 3/4/07, Nikolay Samokhvalov <nikolay(at)samokhvalov(dot)com> wrote:
> > > I'll fix these issues and extend the patch with resgression tests and
> > > docs for xpath_array(). I'll resubmit it very soon.
> >
> > Here is a new version of the patch. I didn't change any part of docs yet.
> > Since there were no objections I've changed the name of the function
> > to xmlpath().
>
> Updated version of the patch contains bugfix: there were a problem
> with path queries that pointed to elements (cases when a set of
> document parts that correspond to subtrees should be returned).
> Example is (included in regression test):
>
> xmltest=# SELECT xmlpath('//b', '<a>one <b>two</b> three <b>etc</b></a>');
> xmlpath
> -------------------------
> {<b>two</b>,<b>etc</b>}
> (1 row)
>
> Waiting for more feedback, please check it.
>
> --
> Best regards,
> Nikolay

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

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

+ If your life is a hard drive, Christ can be your backup. +

Nikolay Samokhvalov wrote:
> Here is a new version of the patch. I didn't change any part of docs
> yet. Since there were no objections I've changed the name of the
> function to xmlpath().

I didn't see any discussion about changing the name to xmlpath. Seeing
that the function implements xpath, and xpath is a recognized name,
this change is wrong.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Bruce Momjian wrote:
> Patch applied.

This code seems to think that if an xml datum starts with "<?xml" it's a
document. That is completely bogus.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Nikolay Samokhvalov wrote:
> On 3/4/07, Nikolay Samokhvalov <nikolay(at)samokhvalov(dot)com> wrote:
> > I'll fix these issues and extend the patch with resgression tests
> > and docs for xpath_array(). I'll resubmit it very soon.
>
> Here is a new version of the patch. I didn't change any part of docs
> yet. Since there were no objections I've changed the name of the
> function to xmlpath().

Why is the function not strict?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Andrew Dunstan wrote:
> Would it be better to use some more unlikely name for the dummy root
> element used to process fragments than <x> ?

Why do we even need to support xpath on fragments?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Nikolay Samokhvalov wrote:
> Also, maybe someone can suggest better approach for passing namespace
> bindings (more convenient than ARRAY[ARRAY[...], ARRAY[...]])?

Your code assumes

ARRAY[ARRAY['myns', 'myns2'], ARRAY['http://example.com', 'http://example2.com']]

Shouldn't it be

ARRAY[ARRAY['myns', 'http://example.com'], ARRAY['myns2', 'http://example2.com']]

?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On 3/23/07, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>
> Andrew Dunstan wrote:
> > Would it be better to use some more unlikely name for the dummy root
> > element used to process fragments than <x> ?
>
> Why do we even need to support xpath on fragments?
>

Why not? I find it useful and convenient.

--
Best regards,
Nikolay

Am Mittwoch, 4. April 2007 14:42 schrieb Nikolay Samokhvalov:
> Maybe it's worth to start keeping additional information in xml datum (i.e.
> bit IS_DOCUMENT and, what is more important for xpath() function, a bit
> indicating that XML value has only one root and can be considered as a tree
> => there is no need to wrap with <x> .. </x>). But this change requires
> additional time to design xml datum structure and to rework the code
> (macros, I/O functions...).

To determine if an XML datum is a document, call xml_is_document(). The
implementation of that function is probably not the best possible one, but
what the xpath() code does it totally wrong nevertheless.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Am Mittwoch, 4. April 2007 14:42 schrieb Nikolay Samokhvalov:
> > Why is the function not strict?
>
> Because in case of 3rd argument (NS mappings) being NULL, we shouldn't
> return NULL immediately:

If the namespace mapping is NULL then it is unknown, and therefore the result
of the XPath expression cannot be evaluated with certainty. If no namespace
mapping is to be passed, then you should pass a list(/array/...) of length
zero.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Am Mittwoch, 4. April 2007 14:43 schrieb Nikolay Samokhvalov:
> > Why do we even need to support xpath on fragments?
>
> Why not? I find it useful and convenient.

Well, rather than inventing bogus root wrapper elements, why not let users
call xmlelement() to produce the wrapper element themselves?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On 4/4/07, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>
> Am Mittwoch, 4. April 2007 14:43 schrieb Nikolay Samokhvalov:
> > > Why do we even need to support xpath on fragments?
> >
> > Why not? I find it useful and convenient.
>
> Well, rather than inventing bogus root wrapper elements, why not let users
> call xmlelement() to produce the wrapper element themselves?

User may even don't know in what case wrapper element is needed. I mean, if
user works with XML column containing both documents and fragments, then
what must he do? Add wrapper anyway? So, users will add XMLELEMENT in almost
any case.

I'd prefer to keep external interfaces simpler (less thinking in such cases
for users).

--
Best regards,
Nikolay

Am Mittwoch, 4. April 2007 15:20 schrieb Nikolay Samokhvalov:
> > To determine if an XML datum is a document, call xml_is_document(). The
> > implementation of that function is probably not the best possible one,
> > but what the xpath() code does it totally wrong nevertheless.
>
> You are proposing 2-3 (depends on the case) parsing times for the one XML
> value instead of current 1-2

I know it's bad, and something like adding a bit (byte) to mark this in the
value would be good, but that doesn't change the fact that

(xmlStrncmp((xmlChar *) VARDATA(data), (xmlChar *) "<?xml", 5) == 0)

is not a valid method to tell apart a document from a fragment. Proof:

pei=# select xml '<?xml version="1.0"?><foo>bar</foo>' IS DOCUMENT;
?column?
----------
t
(1 row)

pei=# select xml '<?xml version="1.0"?><foo>bar</foo><foo>bar</foo>' IS
DOCUMENT;
?column?
----------
f
(1 row)

pei=# select xml '<foo>bar</foo>' IS DOCUMENT;
?column?
----------
t
(1 row)

pei=# select xml '<foo>bar</foo><foo>bar</foo>' IS DOCUMENT;
?column?
----------
f
(1 row)

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On 4/4/07, Nikolay Samokhvalov <nikolay(at)samokhvalov(dot)com> wrote:
>
>
> So, choosing between two inefficient approaches:
> 1. mine, which in some cases use dummy element wrapping, that we could
> escape;
> 2. proposed by you, which leads to +1 parsing.
> ... I'd definitely choose the first one.
>

I'd make it a bit more clear.

We have different cases for XML value as input of xpath():
a. document with prolog ('<?xml...')
b. document w/o prolog (value that can be represented as a tree -- i.e. we
have one root)
c. fragment with one root element (can be represented as a tree)
d. fragment w/o root element (cannot be represented as a tree, e.g.
'bla'::xml)

So, the current implementation works w/o wrapping in case a) and use
wrapping for cases b)-d).
But we _need_ wrapping _only_ in case d) -- so there is space for
optimization (I would keep bit "this value is not a tree" in the value
itself).

--
Best regards,
Nikolay