Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
čt 31. 1. 2019 v 14:44 odesílatel Andres Freund napsal: > Hi, > > On 2018-09-18 11:33:38 -0400, Tom Lane wrote: > > Pavel Stehule writes: > > > [ xml-xpath-default-ns-7.patch ] > > > > At Andrew's prompting, I took a look over this patch. I don't know much > > of anything about XML, so I have no idea as to standards compliance here, > > but I do have some comments: > > > > * I'm fairly uncomfortable with the idea that we're going to maintain > > our own XPath parser. That seems like a recipe for lots of future > > work ... and the code is far too underdocumented for anyone to actually > > maintain it. (Case in point: _transformXPath's arguments are not > > documented at all, and in fact there's hardly a word about what it's > > even supposed to *do*.) > > We were looking at this patch at the pgday developer meeting: Our > impression is that this patch should be rejected. If really desired, the > best approach seems to be actually implement this in libxml, and then go > from there. > Unfortunately, the development of libxml2 is frozen. I have to accept it. Regards Pavel > Greetings, > > Andres Freund >
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hi, On 2018-09-18 11:33:38 -0400, Tom Lane wrote: > Pavel Stehule writes: > > [ xml-xpath-default-ns-7.patch ] > > At Andrew's prompting, I took a look over this patch. I don't know much > of anything about XML, so I have no idea as to standards compliance here, > but I do have some comments: > > * I'm fairly uncomfortable with the idea that we're going to maintain > our own XPath parser. That seems like a recipe for lots of future > work ... and the code is far too underdocumented for anyone to actually > maintain it. (Case in point: _transformXPath's arguments are not > documented at all, and in fact there's hardly a word about what it's > even supposed to *do*.) We were looking at this patch at the pgday developer meeting: Our impression is that this patch should be rejected. If really desired, the best approach seems to be actually implement this in libxml, and then go from there. Greetings, Andres Freund
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
pá 30. 11. 2018 v 9:26 odesílatel Kyotaro HORIGUCHI < horiguchi.kyot...@lab.ntt.co.jp> napsal: > Hello. > > At Fri, 30 Nov 2018 07:48:26 +0100, Pavel Stehule > wrote in < > cafj8prd7zg07t4npzu09t4rgxz0btvyyg2emvoh+o_drnoi...@mail.gmail.com> > > Hi > > > > čt 29. 11. 2018 v 14:44 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > > napsal: > > > > > > On Fri, Sep 21, 2018 at 1:30 PM Pavel Stehule < > pavel.steh...@gmail.com> > > > wrote: > > > > > > > > Thank you for comments > > > > > > > > Attached updated patch > > > > > > Unfortunately, current version of the patch doesn't pass make check, > > > something > > > is missing for xml tests. Could you please rebase it? > > > > > > After that I hope someone from reviewers (Kyotaro?) can probably > confirm if > > > it's still in a good shape. For now I'm moving it to the next CF. > > Sure. Sorry for coming late. I reconfirmed this. > > The most significant change in this version is namespace name > generaton. > > - We can remove strlen from mutate_name by mutating the string in > reverse order. We don't need to mutate it in a human-affinity > order. The name would be 1-letter in almost all cases. > > Concretely, the order in my mind is the follows: > > "" "a" "b" ..."z" "aa" "ba" "ca"... "za" "ab".. > > done > - Might the 'propriety' correctly be 'properties'? > > + /* register namespace if all propriety are available */ > > fixed > - Is the "if" a mistake of "in"? > > + * collect ns names if ResTarget format for possible usage > + * in getUniqNames function. > > yup, fixed > - I suppose the following should be like "register default > namespace definition if any". > > +/* get default namespace name when it is required */ > > fixed > > Maybe the followings are not new. (Note that I'm not a naitive speaker.) > > > - I cannot read this. (I might be to blame..) > > + * default namespace for XPath expressions. Because there are not any > API > + * how to transform or access to parsed XPath expression we have to > parse > + * XPath here. > > - This might need to explain "by what". > > + * Those functionalities are implemented with a simple XPath parser/ > + * preprocessor. This XPath parser transforms a XPath expression to > another > + * XPath expression that can be used by libxml2 XPath evaluation. It > doesn't > + * replace libxml2 XPath parser or libxml2 XPath expression evaluation. > > - "add" -> "adds", "def_namespace_name" seems to need to be > replaced with something else. > > + * This transformation add def_namespace_name to any unqualified node > name > + * or attribute name of xpath expression. > I tried to formulate it better, but I am sorry, my English is not good. > > (Sorry, I'll look further later.) > I am sending a updated patch Regards Pavel > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > default-namespaces-20181130-2.patch.gz Description: application/gzip
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hello. At Fri, 30 Nov 2018 07:48:26 +0100, Pavel Stehule wrote in > Hi > > čt 29. 11. 2018 v 14:44 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > napsal: > > > > On Fri, Sep 21, 2018 at 1:30 PM Pavel Stehule > > wrote: > > > > > > Thank you for comments > > > > > > Attached updated patch > > > > Unfortunately, current version of the patch doesn't pass make check, > > something > > is missing for xml tests. Could you please rebase it? > > > > After that I hope someone from reviewers (Kyotaro?) can probably confirm if > > it's still in a good shape. For now I'm moving it to the next CF. Sure. Sorry for coming late. I reconfirmed this. The most significant change in this version is namespace name generaton. - We can remove strlen from mutate_name by mutating the string in reverse order. We don't need to mutate it in a human-affinity order. The name would be 1-letter in almost all cases. Concretely, the order in my mind is the follows: "" "a" "b" ..."z" "aa" "ba" "ca"... "za" "ab".. - Might the 'propriety' correctly be 'properties'? + /* register namespace if all propriety are available */ - Is the "if" a mistake of "in"? + * collect ns names if ResTarget format for possible usage + * in getUniqNames function. - I suppose the following should be like "register default namespace definition if any". +/* get default namespace name when it is required */ Maybe the followings are not new. (Note that I'm not a naitive speaker.) - I cannot read this. (I might be to blame..) + * default namespace for XPath expressions. Because there are not any API + * how to transform or access to parsed XPath expression we have to parse + * XPath here. - This might need to explain "by what". + * Those functionalities are implemented with a simple XPath parser/ + * preprocessor. This XPath parser transforms a XPath expression to another + * XPath expression that can be used by libxml2 XPath evaluation. It doesn't + * replace libxml2 XPath parser or libxml2 XPath expression evaluation. - "add" -> "adds", "def_namespace_name" seems to need to be replaced with something else. + * This transformation add def_namespace_name to any unqualified node name + * or attribute name of xpath expression. (Sorry, I'll look further later.) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hi čt 29. 11. 2018 v 14:44 odesílatel Dmitry Dolgov <9erthali...@gmail.com> napsal: > > On Fri, Sep 21, 2018 at 1:30 PM Pavel Stehule > wrote: > > > > Thank you for comments > > > > Attached updated patch > > Unfortunately, current version of the patch doesn't pass make check, > something > is missing for xml tests. Could you please rebase it? > > After that I hope someone from reviewers (Kyotaro?) can probably confirm if > it's still in a good shape. For now I'm moving it to the next CF. > here is rebased patch Regards Pavel default-namespaces-20181130.patch.gz Description: application/gzip
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
> On Fri, Sep 21, 2018 at 1:30 PM Pavel Stehule wrote: > > Thank you for comments > > Attached updated patch Unfortunately, current version of the patch doesn't pass make check, something is missing for xml tests. Could you please rebase it? After that I hope someone from reviewers (Kyotaro?) can probably confirm if it's still in a good shape. For now I'm moving it to the next CF.
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hi út 18. 9. 2018 v 17:33 odesílatel Tom Lane napsal: > Pavel Stehule writes: > > [ xml-xpath-default-ns-7.patch ] > > At Andrew's prompting, I took a look over this patch. I don't know much > of anything about XML, so I have no idea as to standards compliance here, > but I do have some comments: > > * I'm fairly uncomfortable with the idea that we're going to maintain > our own XPath parser. That seems like a recipe for lots of future > work ... and the code is far too underdocumented for anyone to actually > maintain it. (Case in point: _transformXPath's arguments are not > documented at all, and in fact there's hardly a word about what it's > even supposed to *do*.) > I understand, and I would be much more happy if xmllib2 supports this feature. But the development of new feature of this lib was frozen - and there are not any possibility. On second hand the parser is very simple - tokenizer detects identifiers, strings, numbers, and parser try to find unqualified names and prints default namespace identifier before. It doesn't do more. I renamed function _transformXPath to transform_xpath_recurse and I descibed params > * I think the business with "pgdefnamespace.pgsqlxml.internal" is just > plain awful. It's a wart, and I don't think it's even saving you any > code once you account for all the places where you have to throw errors > for somebody trying to use that as a regular name. This should be done > with out-of-band signaling if possible. The existing convention above > this code is that a NULL pointer means a default namespace; can't that > be continued throughout? If that's not practical, can you pick a string > that simply can't syntactically be a namespace name? (In the SQL world, > for example, an empty string is an illegal identifier so that that could > be used for the purpose. But I don't know if that applies to XML.) > Or perhaps you can build a random name that is chosen just to make it > different from any of the listed namespaces? If none of those work, > I think passing around an explicit "isdefault" boolean alongside the name > would be better than having a reserved word. > The string used like default namespace name should be valid XML namespace name, because it is injected to XPath expression and it is passed to libxml2 as one namespace name. libxml2 requires not null valid value. I changed it and now the namespace name is generated. > > * _transformXPath recurses, so doesn't it need check_stack_depth()? > fixed > > * I'm not especially in love with using function names that start > with an underscore. I do not think that adds anything, and it's > unlike the style in most places in PG. > renamed > * This is a completely unhelpful error message: > + if (!parser->buffer_is_empty) > + elog(ERROR, "internal error"); > If you can't be bothered to write a message that says something > useful, either drop the test or turn it into an Assert. I see some > other internal errors that aren't up to project norms either. > use assert > > * Either get rid of the "elog(DEBUG1)"s, or greatly lower their > message priority. They might've been appropriate for developing > this patch, but they are not okay to commit that way. > removed > * Try to reduce the amount of random whitespace changes in the patch. > The formatting was really strange, fixed > > * .h files should never #include "postgres.h", per project policy. > I moved the code to xml.c like you propose > > * I'm not sure I'd bother with putting the new code into a separate > file rather than cramming it into xml.c. The main reason why not > is that you're going to get "empty translation unit" warnings from > some compilers in builds without USE_LIBXML. > > * Documentation, comments, and error messages could all use some > copy-editing by a native English speaker (you knew that of course). > I hope so some native speakers looks there. Thank you for comments Attached updated patch Regards Pavel > regards, tom lane > default_namespace-20180921.patch.gz Description: application/gzip
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Pavel Stehule writes: > [ xml-xpath-default-ns-7.patch ] At Andrew's prompting, I took a look over this patch. I don't know much of anything about XML, so I have no idea as to standards compliance here, but I do have some comments: * I'm fairly uncomfortable with the idea that we're going to maintain our own XPath parser. That seems like a recipe for lots of future work ... and the code is far too underdocumented for anyone to actually maintain it. (Case in point: _transformXPath's arguments are not documented at all, and in fact there's hardly a word about what it's even supposed to *do*.) * I think the business with "pgdefnamespace.pgsqlxml.internal" is just plain awful. It's a wart, and I don't think it's even saving you any code once you account for all the places where you have to throw errors for somebody trying to use that as a regular name. This should be done with out-of-band signaling if possible. The existing convention above this code is that a NULL pointer means a default namespace; can't that be continued throughout? If that's not practical, can you pick a string that simply can't syntactically be a namespace name? (In the SQL world, for example, an empty string is an illegal identifier so that that could be used for the purpose. But I don't know if that applies to XML.) Or perhaps you can build a random name that is chosen just to make it different from any of the listed namespaces? If none of those work, I think passing around an explicit "isdefault" boolean alongside the name would be better than having a reserved word. * _transformXPath recurses, so doesn't it need check_stack_depth()? * I'm not especially in love with using function names that start with an underscore. I do not think that adds anything, and it's unlike the style in most places in PG. * This is a completely unhelpful error message: + if (!parser->buffer_is_empty) + elog(ERROR, "internal error"); If you can't be bothered to write a message that says something useful, either drop the test or turn it into an Assert. I see some other internal errors that aren't up to project norms either. * Either get rid of the "elog(DEBUG1)"s, or greatly lower their message priority. They might've been appropriate for developing this patch, but they are not okay to commit that way. * Try to reduce the amount of random whitespace changes in the patch. * .h files should never #include "postgres.h", per project policy. * I'm not sure I'd bother with putting the new code into a separate file rather than cramming it into xml.c. The main reason why not is that you're going to get "empty translation unit" warnings from some compilers in builds without USE_LIBXML. * Documentation, comments, and error messages could all use some copy-editing by a native English speaker (you knew that of course). regards, tom lane
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
po 17. 9. 2018 v 23:15 odesílatel Thomas Munro < thomas.mu...@enterprisedb.com> napsal: > On Mon, Sep 17, 2018 at 5:36 PM Pavel Stehule > wrote: > > po 17. 9. 2018 v 2:05 odesílatel Thomas Munro < > thomas.mu...@enterprisedb.com> napsal: > >> On Fri, Aug 10, 2018 at 6:26 AM Andrew Dunstan > >> wrote: > >> > On 01/24/2018 04:30 AM, Pavel Stehule wrote: > >> > > > >> > > I am sending updated version. > >> > > > >> > > Very much thanks for very precious review > >> > > >> > Thomas, > >> > > >> > I am unable to replicate the Linux failure seen in the cfbot on my > >> > Fedora machine. Both when building with libxml2 and without, after > >> > applying the latest patch the tests pass without error. Can you please > >> > investigate what's going on here? > >> > >> Well this is strange... I can't reproduce the problem either with or > >> without --with-libxml on a Debian box (was trying to get fairly close > >> to the OS that Travis runs on). But I see the same failure when I > >> apply the patch on my FreeBSD 12 laptop and test without > >> --with-libxml. Note that when cfbot runs it, the patch is applied > >> with FreeBSD patch, and then it's tested without --with-libxml on > >> Ubuntu (Travis's default OS). [Side note: I should change it to build > >> --with-libxml, but that's not the point.] So the common factor is a > >> different patch implementation. I wonder if a hunk is being > >> misinterpreted. > > > > > > This patch is not too large. Please, can me send a related files, I can > check it manually. > > I confirmed that xml_1.out is different depending on which 'patch' you > use. I've attached the output from FreeBSD patch 2.0-12u11 and GNU > patch 2.5.8. It's an interesting phenomenon, probably due to having a > huge long file with a lot of repeated text and a slightly different > algorithms or parameters, but I don't think you need to worry about it > for this. Sorry for the distraction. > ok. Thank you for info Regards Pavel > > -- > Thomas Munro > http://www.enterprisedb.com >
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
po 17. 9. 2018 v 2:05 odesílatel Thomas Munro napsal: > On Fri, Aug 10, 2018 at 6:26 AM Andrew Dunstan > wrote: > > On 01/24/2018 04:30 AM, Pavel Stehule wrote: > > > > > > I am sending updated version. > > > > > > Very much thanks for very precious review > > > > Thomas, > > > > I am unable to replicate the Linux failure seen in the cfbot on my > > Fedora machine. Both when building with libxml2 and without, after > > applying the latest patch the tests pass without error. Can you please > > investigate what's going on here? > > Well this is strange... I can't reproduce the problem either with or > without --with-libxml on a Debian box (was trying to get fairly close > to the OS that Travis runs on). But I see the same failure when I > apply the patch on my FreeBSD 12 laptop and test without > --with-libxml. Note that when cfbot runs it, the patch is applied > with FreeBSD patch, and then it's tested without --with-libxml on > Ubuntu (Travis's default OS). [Side note: I should change it to build > --with-libxml, but that's not the point.] So the common factor is a > different patch implementation. I wonder if a hunk is being > misinterpreted. > This patch is not too large. Please, can me send a related files, I can check it manually. Regards Pavel > -- > Thomas Munro > http://www.enterprisedb.com >
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
On Fri, Aug 10, 2018 at 6:26 AM Andrew Dunstan wrote: > On 01/24/2018 04:30 AM, Pavel Stehule wrote: > > > > I am sending updated version. > > > > Very much thanks for very precious review > > Thomas, > > I am unable to replicate the Linux failure seen in the cfbot on my > Fedora machine. Both when building with libxml2 and without, after > applying the latest patch the tests pass without error. Can you please > investigate what's going on here? Well this is strange... I can't reproduce the problem either with or without --with-libxml on a Debian box (was trying to get fairly close to the OS that Travis runs on). But I see the same failure when I apply the patch on my FreeBSD 12 laptop and test without --with-libxml. Note that when cfbot runs it, the patch is applied with FreeBSD patch, and then it's tested without --with-libxml on Ubuntu (Travis's default OS). [Side note: I should change it to build --with-libxml, but that's not the point.] So the common factor is a different patch implementation. I wonder if a hunk is being misinterpreted. -- Thomas Munro http://www.enterprisedb.com
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
On 01/24/2018 04:30 AM, Pavel Stehule wrote: I am sending updated version. Very much thanks for very precious review Thomas, I am unable to replicate the Linux failure seen in the cfbot on my Fedora machine. Both when building with libxml2 and without, after applying the latest patch the tests pass without error. Can you please investigate what's going on here? cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hello. I reviewed this and think that this is in Ready for Committer stage. The patch is available here. https://www.postgresql.org/message-id/CAFj8pRBVUVvG1CXxgrs0UipTziUX6M788z-%3DL9gQvwAB4UGLeg%40mail.gmail.com The following list consists of the same items in upthread message as confirmation. - This applies to the current master HEAD cleanly. - The code looks fine. - This patch translates the given XPath expression by prefixing unprefixed tag names with a special namespace prefix only in the case where default namespace is defined, so the existing behavior is not affected. - The syntax of default namespace is existing but just not usable so I don't think no arguemnts needed here. - It undocumentedly inhibits the usage of the namespace prefix "pgdefnamespace.pgsqlxml.internal" but I believe no one can notice that. - The default-ns translator (xpath_parser.c) seems working perfectly with some harmless exceptions. (Behavior about context variables and user-defined xml functions, which are not handled by PostgreSQL.) - Dodumentation looks enough. - Regression test doesn't cover the XPath syntax but I think it's not viable. I am fine with the basic test cases added by the current patch. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hello. At Wed, 24 Jan 2018 10:30:39 +0100, Pavel Stehulewrote in
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hi 2018-01-23 8:13 GMT+01:00 Kyotaro HORIGUCHI: > Hello, I returned to this. > > I thouroughly checked the translator's behavior against the XPath > specifications and checkd out the documentation and regression > test. Almost everything is fine for me and this would be the last > comment from me. > > At Fri, 24 Nov 2017 18:32:43 +0100, Pavel Stehule > wrote in mail.gmail.com> > > 2017-11-24 18:13 GMT+01:00 Pavel Stehule : > > > > > > > > > > > 2017-11-24 17:53 GMT+01:00 Pavel Stehule : > > > > > >> Hi > > >> > > >> 2017-11-22 22:49 GMT+01:00 Thomas Munro < > thomas.mu...@enterprisedb.com>: > > >> > > >>> On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehule < > pavel.steh...@gmail.com> > > >>> wrote: > > >>> > Attached new version. > > >>> > > >>> Hi Pavel, > > >>> > > >>> FYI my patch testing robot says[1]: > > >>> > > >>> xml ... FAILED > > >>> > > >>> regression.diffs says: > > >>> > > >>> + SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'), > > >>> '/rows/row' PASSING t1.doc COLUMNS data int PATH > > >>> 'child::a[1][attribute::hoge="haha"]') as x; > > >>> + data > > >>> + -- > > >>> + (0 rows) > > >>> + > > >>> > > >>> Maybe you forgot to git-add the expected file? > > >>> > > >>> [1] https://travis-ci.org/postgresql-cfbot/postgresql/ > builds/305979133 > > >> > > >> > > >> unfortunately xml.out has 3 versions and is possible so one version > > >> should be taken elsewhere than my comp. > > >> > > >> please can me send your result xml.out file? > > >> > > > > > > looks like this case is without xml support so I can fix on my comp. > > > > > > > > fixed regress test > > (I wouldn't have found that..) > > I have three comments on the behavior and one on documentation. > > 1. Lack of syntax handling. > > ["'" [^'] "'"] is also a string literal, but getXPathToken is > forgetting that and applying default namespace mistakenly to the > astring content. > In this case, I am not sure if I understand well. Within expressions, literal strings are delimited by single or double quotation marks, which are also used to delimit XML attributes. To avoid a quotation mark in an expression being interpreted by the XML processor as terminating the attribute value the quotation mark can be entered as a character reference ( or ). Alternatively, the expression can use single quotation marks if the XML attribute is delimited with double quotation marks or vice-versa. So if I understand well, then XML string can looks like ' some " some ' or " some ' some ". I fixed it. > 2. Additional comment might be good. > > It might be better having additional description about default > namespace in the comment starts from "Namespace mappings are > passed as text[]" in xpth_internal(). > fixed > 3. Inconsistent behavior from named namespace. > > | - function context, aliases are local). > | + function context, aliases are local). Default > namespace has > | + empty name (empty string) and should be only one. > > This works as the description, on the other hand the same > namespace prefix can be defined twice or more in the array and > the last one is in effect. I don't see a reason for > differenciating the default namespace case. > It looks like libxml2 bug. There is no sense to use more than one default namespace, and although it is inconsistent with other namespaces, I am thinking so it is correct. Is better to raise error early. In this case Postgres expects so libxml2 ensure all namespace checks and it is tolerant. Default namespace is implemented inside Postgres, and I don't see any advantage of tolerant behave. More default namespaces is disallowed for XMLTABLE - so some inconsistency there should be. In this case I prefer raise error to signalize ambiguous or badly formatted input clearly. > > > 4. Comments on the documentation part. > > # Even though I'm not sutable for commenting on wording... > > | + Inside predicate literals and, > or, > | + div and mod are used as > keywords > | + (XPath operators) every time and default namespace are not applied > there. > > *I*'d like to have a comma between the predicate and literals, > and have a 'a' before prediate. Or 'Literals .. inside a > predicate' might be better? > fixed > > 'are used as keywords' might be better being 'are identifed as > keywords'? > fixed > Default namespace is applied to tag names except the listed > keywords even inside a predicate. So 'are not applied there' > might be better being 'are not applied to them'? Or 'are not > applied in the case'? > > | + If you would to use these literals like tag names, then the > default namespace > | + should not be used, and these literals should be explicitly > | + labeled. > | + > > Default namespace is not applied *only to* such keywords inside a >
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hi 2018-01-23 8:13 GMT+01:00 Kyotaro HORIGUCHI: > Hello, I returned to this. > > I thouroughly checked the translator's behavior against the XPath > specifications and checkd out the documentation and regression > test. Almost everything is fine for me and this would be the last > comment from me. > > At Fri, 24 Nov 2017 18:32:43 +0100, Pavel Stehule > wrote in mail.gmail.com> > > 2017-11-24 18:13 GMT+01:00 Pavel Stehule : > > > > > > > > > > > 2017-11-24 17:53 GMT+01:00 Pavel Stehule : > > > > > >> Hi > > >> > > >> 2017-11-22 22:49 GMT+01:00 Thomas Munro < > thomas.mu...@enterprisedb.com>: > > >> > > >>> On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehule < > pavel.steh...@gmail.com> > > >>> wrote: > > >>> > Attached new version. > > >>> > > >>> Hi Pavel, > > >>> > > >>> FYI my patch testing robot says[1]: > > >>> > > >>> xml ... FAILED > > >>> > > >>> regression.diffs says: > > >>> > > >>> + SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'), > > >>> '/rows/row' PASSING t1.doc COLUMNS data int PATH > > >>> 'child::a[1][attribute::hoge="haha"]') as x; > > >>> + data > > >>> + -- > > >>> + (0 rows) > > >>> + > > >>> > > >>> Maybe you forgot to git-add the expected file? > > >>> > > >>> [1] https://travis-ci.org/postgresql-cfbot/postgresql/ > builds/305979133 > > >> > > >> > > >> unfortunately xml.out has 3 versions and is possible so one version > > >> should be taken elsewhere than my comp. > > >> > > >> please can me send your result xml.out file? > > >> > > > > > > looks like this case is without xml support so I can fix on my comp. > > > > > > > > fixed regress test > > (I wouldn't have found that..) > > I have three comments on the behavior and one on documentation. > > 1. Lack of syntax handling. > > ["'" [^'] "'"] is also a string literal, but getXPathToken is > forgetting that and applying default namespace mistakenly to the > astring content. > > 2. Additional comment might be good. > > It might be better having additional description about default > namespace in the comment starts from "Namespace mappings are > passed as text[]" in xpth_internal(). > > 3. Inconsistent behavior from named namespace. > > | - function context, aliases are local). > | + function context, aliases are local). Default > namespace has > | + empty name (empty string) and should be only one. > > This works as the description, on the other hand the same > namespace prefix can be defined twice or more in the array and > the last one is in effect. I don't see a reason for > differenciating the default namespace case. > > > 4. Comments on the documentation part. > > # Even though I'm not sutable for commenting on wording... > > | + Inside predicate literals and, > or, > | + div and mod are used as > keywords > | + (XPath operators) every time and default namespace are not applied > there. > > *I*'d like to have a comma between the predicate and literals, > and have a 'a' before prediate. Or 'Literals .. inside a > predicate' might be better? > > 'are used as keywords' might be better being 'are identifed as > keywords'? > > Default namespace is applied to tag names except the listed > keywords even inside a predicate. So 'are not applied there' > might be better being 'are not applied to them'? Or 'are not > applied in the case'? > > | + If you would to use these literals like tag names, then the > default namespace > | + should not be used, and these literals should be explicitly > | + labeled. > | + > > Default namespace is not applied *only to* such keywords inside a > predicate. Even if an Xpath expression contains such a tag name, > default namespace still works for other tags. Does the following > make sense? > > + Use named namespace to qualify such tag names appear in an > + XPath predicate. > > please, can you append examples of mentioned issues. I'll fix it faster. Thank you very much Pavel > > > === > After the aboves are addressed (even or rejected), I think I > don't have no additional comment. > > > - This current patch applies safely (with small shifts) on the > current master. > > - The code looks fine for me. > > - This patch translates the given XPath expression by prefixing > unprefixed tag names with a special namespace prefix only in > the case where default namespace is defined, so the existing > behavior is not affected. > > - The syntax is existing but just not implemented so I don't > think no arguemnts needed here. > > - It undocumentedly inhibits the usage of the namespace prefix > "pgdefnamespace.pgsqlxml.internal" but I believe no one can > notice that. > > - The default-ns translator (xpath_parser.c) seems working > perfectly with some harmless exceptions. > > (xpath specifications is here: https://www.w3.org/TR/1999/ >
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
Hello, I returned to this. I thouroughly checked the translator's behavior against the XPath specifications and checkd out the documentation and regression test. Almost everything is fine for me and this would be the last comment from me. At Fri, 24 Nov 2017 18:32:43 +0100, Pavel Stehulewrote in > 2017-11-24 18:13 GMT+01:00 Pavel Stehule : > > > > > > > 2017-11-24 17:53 GMT+01:00 Pavel Stehule : > > > >> Hi > >> > >> 2017-11-22 22:49 GMT+01:00 Thomas Munro : > >> > >>> On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehule > >>> wrote: > >>> > Attached new version. > >>> > >>> Hi Pavel, > >>> > >>> FYI my patch testing robot says[1]: > >>> > >>> xml ... FAILED > >>> > >>> regression.diffs says: > >>> > >>> + SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'), > >>> '/rows/row' PASSING t1.doc COLUMNS data int PATH > >>> 'child::a[1][attribute::hoge="haha"]') as x; > >>> + data > >>> + -- > >>> + (0 rows) > >>> + > >>> > >>> Maybe you forgot to git-add the expected file? > >>> > >>> [1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/305979133 > >> > >> > >> unfortunately xml.out has 3 versions and is possible so one version > >> should be taken elsewhere than my comp. > >> > >> please can me send your result xml.out file? > >> > > > > looks like this case is without xml support so I can fix on my comp. > > > > > fixed regress test (I wouldn't have found that..) I have three comments on the behavior and one on documentation. 1. Lack of syntax handling. ["'" [^'] "'"] is also a string literal, but getXPathToken is forgetting that and applying default namespace mistakenly to the astring content. 2. Additional comment might be good. It might be better having additional description about default namespace in the comment starts from "Namespace mappings are passed as text[]" in xpth_internal(). 3. Inconsistent behavior from named namespace. | - function context, aliases are local). | + function context, aliases are local). Default namespace has | + empty name (empty string) and should be only one. This works as the description, on the other hand the same namespace prefix can be defined twice or more in the array and the last one is in effect. I don't see a reason for differenciating the default namespace case. 4. Comments on the documentation part. # Even though I'm not sutable for commenting on wording... | + Inside predicate literals and, or, | + div and mod are used as keywords | + (XPath operators) every time and default namespace are not applied there. *I*'d like to have a comma between the predicate and literals, and have a 'a' before prediate. Or 'Literals .. inside a predicate' might be better? 'are used as keywords' might be better being 'are identifed as keywords'? Default namespace is applied to tag names except the listed keywords even inside a predicate. So 'are not applied there' might be better being 'are not applied to them'? Or 'are not applied in the case'? | + If you would to use these literals like tag names, then the default namespace | + should not be used, and these literals should be explicitly | + labeled. | + Default namespace is not applied *only to* such keywords inside a predicate. Even if an Xpath expression contains such a tag name, default namespace still works for other tags. Does the following make sense? + Use named namespace to qualify such tag names appear in an + XPath predicate. === After the aboves are addressed (even or rejected), I think I don't have no additional comment. - This current patch applies safely (with small shifts) on the current master. - The code looks fine for me. - This patch translates the given XPath expression by prefixing unprefixed tag names with a special namespace prefix only in the case where default namespace is defined, so the existing behavior is not affected. - The syntax is existing but just not implemented so I don't think no arguemnts needed here. - It undocumentedly inhibits the usage of the namespace prefix "pgdefnamespace.pgsqlxml.internal" but I believe no one can notice that. - The default-ns translator (xpath_parser.c) seems working perfectly with some harmless exceptions. (xpath specifications is here: https://www.w3.org/TR/1999/REC-xpath-19991116/) Related unused features (and not documented?): context variables ($n notations), user-defined functions (or function names prefixed by a namespace prefix) Newly documented behavior: the default namespace isn't applied to and/or/div/mod. - Dodumentation looks enough. - Regression test doesn't cover the XPath syntax but it's not viable. I am fine with the basic test cases added by the current patch. regards, -- Kyotaro
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
At Wed, 29 Nov 2017 14:33:08 +0900, Michael Paquierwrote in > On Sat, Nov 25, 2017 at 2:32 AM, Pavel Stehule > wrote: > > fixed regress test > > The last patch still applies, but did not get any reviews. > Horiguchi-san, you are marked as a reviewer of this patch. Could you > look at it? For now, I am moving it to next CF. Sorry for the absense. It is near to complete. I'll look this soon. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
2017-11-24 18:13 GMT+01:00 Pavel Stehule: > > > 2017-11-24 17:53 GMT+01:00 Pavel Stehule : > >> Hi >> >> 2017-11-22 22:49 GMT+01:00 Thomas Munro : >> >>> On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehule >>> wrote: >>> > Attached new version. >>> >>> Hi Pavel, >>> >>> FYI my patch testing robot says[1]: >>> >>> xml ... FAILED >>> >>> regression.diffs says: >>> >>> + SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'), >>> '/rows/row' PASSING t1.doc COLUMNS data int PATH >>> 'child::a[1][attribute::hoge="haha"]') as x; >>> + data >>> + -- >>> + (0 rows) >>> + >>> >>> Maybe you forgot to git-add the expected file? >>> >>> [1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/305979133 >> >> >> unfortunately xml.out has 3 versions and is possible so one version >> should be taken elsewhere than my comp. >> >> please can me send your result xml.out file? >> > > looks like this case is without xml support so I can fix on my comp. > > fixed regress test > >> Regards >> >> Pavel >> >> >>> >>> -- >>> Thomas Munro >>> http://www.enterprisedb.com >>> >> >> > diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 4dd9d029e6..b871e82a73 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -10489,7 +10489,8 @@ SELECT xml_is_well_formed_document('http://postgresql.org/stuf second the namespace URI. It is not required that aliases provided in this array be the same as those being used in the XML document itself (in other words, both in the XML document and in the xpath - function context, aliases are local). + function context, aliases are local). Default namespace has + empty name (empty string) and should be only one. @@ -10505,11 +10506,20 @@ SELECT xpath('/my:a/text()', 'http://example.com;>test', ]]> + + Inside predicate literals and, or, + div and mod are used as keywords + (XPath operators) every time and default namespace are not applied there. + If you would to use these literals like tag names, then the default namespace + should not be used, and these literals should be explicitly + labeled. + + To deal with default (anonymous) namespaces, do something like this:
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
2017-11-24 17:53 GMT+01:00 Pavel Stehule: > Hi > > 2017-11-22 22:49 GMT+01:00 Thomas Munro : > >> On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehule >> wrote: >> > Attached new version. >> >> Hi Pavel, >> >> FYI my patch testing robot says[1]: >> >> xml ... FAILED >> >> regression.diffs says: >> >> + SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'), >> '/rows/row' PASSING t1.doc COLUMNS data int PATH >> 'child::a[1][attribute::hoge="haha"]') as x; >> + data >> + -- >> + (0 rows) >> + >> >> Maybe you forgot to git-add the expected file? >> >> [1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/305979133 > > > unfortunately xml.out has 3 versions and is possible so one version should > be taken elsewhere than my comp. > > please can me send your result xml.out file? > looks like this case is without xml support so I can fix on my comp. > Regards > > Pavel > > >> >> -- >> Thomas Munro >> http://www.enterprisedb.com >> > >
Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)
On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehulewrote: > Attached new version. Hi Pavel, FYI my patch testing robot says[1]: xml ... FAILED regression.diffs says: + SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'), '/rows/row' PASSING t1.doc COLUMNS data int PATH 'child::a[1][attribute::hoge="haha"]') as x; + data + -- + (0 rows) + Maybe you forgot to git-add the expected file? [1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/305979133 -- Thomas Munro http://www.enterprisedb.com