Re: [REVIEW] Re: Fix xpath() to return namespace definitions - Mailing list pgsql-hackers
From | Ali Akbar |
---|---|
Subject | Re: [REVIEW] Re: Fix xpath() to return namespace definitions |
Date | |
Msg-id | CACQjQLqK4jJGabXRONr+742NacAxynq2q3XKewo7GvqDGDNpHw@mail.gmail.com Whole thread Raw |
In response to | Re: [REVIEW] Re: Fix xpath() to return namespace definitions (Ali Akbar <the.apaan@gmail.com>) |
List | pgsql-hackers |
2014-11-05 21:50 GMT+07:00 Ali Akbar <the.apaan@gmail.com>:
2014-11-04 22:16 GMT+07:00 Peter Eisentraut <peter_e@gmx.net>:*** 584,593 ****is sound, but I'd ask you to go back to the xmlCopyNode() version, andI think the problem this patch is addressing is real, and your approach
try to add a test case for why the second argument = 1 is necessary. I
don't see any other problems.OK. Because upstream code is fixed in current version, i'll revert to the previous version. Test case added to regression test. With =1 argument, the result is correct:
<local:piece xmlns:local=\"http://127.0.0.1\" xmlns=\"http://127.0.0.2\" id=\"1\">
<internal>number one</internal>
<internal2/>
</local:piece>without the argument, the result is not correct, all children will be lost. Because of that, the other regression test will fail too because the children is not copied:
-- Text XPath expressions evaluation
SELECT xpath('/value', data) FROM xmltest;
! xpath
! ----------------------
! {<value>one</value>}
! {<value>two</value>}
(2 rows)
SELECT xpath(NULL, NULL) IS NULL FROM xmltest;
--- 584,593 ----
-- Text XPath expressions evaluation
SELECT xpath('/value', data) FROM xmltest;
! xpath
! ------------
! {<value/>}
! {<value/>}
(2 rows)
SELECT xpath(NULL, NULL) IS NULL FROM xmltest;
***************... <cut>updated patch attached.I noticed somewhat big performance regression if the xml is large (i use PRODML Product Volume sample from energistics.org):* Without patch (tested 3 times):
select unnest(xpath('//a:flow', x, ARRAY[['a','http://www.prodml.org/schemas/1series']])) from u;
unnest
-----------------------------------------------------------------------------------------------
<flow> +
<kind>gas lift</kind> +
...
Time: 84,012 ms
Time: 85,683 ms
Time: 88,766 ms* With latest v6 patch (notice the correct result with namespace definition):select unnest(xpath('//a:flow', x, ARRAY[['a','http://www.prodml.org/schemas/1series']])) from u;
unnest
-----------------------------------------------------------------------------------------------
<flow xmlns="http://www.prodml.org/schemas/1series"> +
...
Time: 108,912 ms
Time: 108,267 ms
Time: 114,848 msIt's 23% performance regression.* Just curious, i'm also testing v5 patch performance (notice the namespace in the result):
select unnest(xpath('//a:flow', x, ARRAY[['a','http://www.prodml.org/schemas/1series']])) from u;
unnest
-----------------------------------------------------------------------------------------------
<flow xmlns="http://www.prodml.org/schemas/1series"> +
<kind>gas lift</kind> +
Time: 92,552 ms
Time: 97,440 ms
Time: 99,309 msThe regression is only 13%. I know the xmlCopyNode() version (v6 patch) is much more cleaner than v5patch, should we consider the performance benefit?Anyway, thanks for the review! :)
commit bac2739 in master by Tom Lane changes *astate definition in xml_xpathobjtoxmlarray, this attached v7 patch rebases v6 patch with latest master.
For performance comparison, i also rebased the v5 patch attached with name v5-141126.patch
Ali Akbar
Attachment
pgsql-hackers by date: