Re: proposal - Default namespaces for XPath expressions (PostgreSQL 11)

Thank you for the new patch.

- The latest patch is missing xpath_parser.h at least since
ns-3. That of the first (not-numbered) version was still
usable.

- c29c578 conflicts on doc/src/sgml/func.sgml

At Sun, 15 Oct 2017 12:06:11 +0200, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote in <CAFj8pRCYBH+a6oJoEYUFDUpBQ1ySwtt2CfnFZxs2Ab9EfONbUQ(at)mail(dot)gmail(dot)com>
> 2017-10-02 12:22 GMT+02:00 Kyotaro HORIGUCHI <
> horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>:
>
> > Hi, thanks for the new patch.
> >
> > # The patch is missing xpath_parser.h. That of the first patch was usable.
> >
> > At Thu, 28 Sep 2017 07:59:41 +0200, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> > wrote in <CAFj8pRBMQa07a=+qQAVMtz5M_hqkJBhiQSOP76+-BrFDj37pvg@
> > mail.gmail.com>
> > > Hi
> > >
> > > now xpath and xpath_exists supports default namespace too
> >
> > At Wed, 27 Sep 2017 22:41:52 +0200, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> > wrote in <CAFj8pRCZ8oneG7g2vxs9ux71n8A9twwUO7zQpJiuz+7RGSpSuw(at)mail(dot)
> > gmail.com>
> > > > 1. Uniformity among simliar features
> > > >
> > > > As mentioned in the proposal, but it is lack of uniformity that
> > > > the xpath transformer is applied only to xmltable and not for
> > > > other xpath related functions.
> > > >
> > >
> > > I have to fix the XPath function. The SQL/XML function Xmlexists doesn't
> > > support namespaces/
> >
> > Sorry, I forgot to care about that. (And the definition of
> > namespace array is of course fabricated by me). I'd like to leave
> > this to committers. Anyway it is working but the syntax (or
> > whether it is acceptable) is still arguable.
> >
> > SELECT xpath('/a/text()', '<my:a xmlns:my="http://example.com">
> > test</my:a>',
> > ARRAY[ARRAY['', 'http://example.com']]);
> > | xpath
> > | --------
> > | {test}
> > | (1 row)
> >
> >
> > The internal name is properly rejected, but the current internal
> > name (pgdefnamespace.pgsqlxml.internal) seems a bit too long. We
> > are preserving some short names and reject them as
> > user-defined. Doesn't just 'pgsqlxml' work?
> >
> >
> > Default namespace correctly become to be applied on bare
> > attribute names.
> >
> > > updated doc,
> > > fixed all variants of expected result test file
> >
> > Sorry for one by one comment but I found another misbehavior.
> >
> > create table t1 (id int, doc xml);
> > insert into t1
> > values
> > (5, '<rows xmlns="http://x.y"><row><a hoge="haha">50</a></row></
> > rows>');
> > select x.* from t1, xmltable(XMLNAMESPACES('http://x.y' AS x),
> > '/x:rows/x:row' passing t1.doc columns data int PATH
> > 'child::x:a[1][attribute::hoge="haha"]') as x;
> > | data
> > | ------
> > | 50
> >
> > but the following fails.
> >
> > 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
> > | ------
> > |
> > | (1 row)
> >
> > Perhaps child::a is not prefixed by the transformation.
> >
>
> the problem was in unwanted attribute modification. The parser didn't
> detect "attribute::hoge" as attribute. Updated parser does it. I reduce
> duplicated code there more.

It worked as expected. But the comparison of "attribute" is
missing t1.length = 9 so the following expression wrongly passes.

child::a[1][attributeabcdefg::hoge="haha"

It is confusing that is_qual_name becomes true when t2 is not a
"qual name", and the way it treats a double-colon is hard to
understand.

It essentially does inserting the default namespace before
unqualified non-attribute name. I believe we can easily
look-ahead to detect a double colon and it would make things
simpler. Could you consider something like the attached patch?
(applies on top of ns-4 patch.)

> > XPath might be complex enough so that it's worth switching to
> > yacc/lex based transformer that is formally verifiable and won't
> > need a bunch of cryptic tests that finally cannot prove the
> > completeness. synchronous_standy_names is far simpler than XPath
> > but using yacc/lex parser.
> >
> >
> > Anyway the following is nitpicking of the current xpath_parser.c.
> >
> > - NODENAME_FIRSTCHAR allows '-' as the first char but it is
> > excluded from NameStartChar (https://www.w3.org/TR/REC-
> > xml/#NT-NameStartChar)
> > I think characters with high-bit set is okay.
> > Also IS_NODENAME_CHAR should be changed.
> >
>
> fixed
>
>
> > - NODENAME_FIRSTCHAR and IS_NODENAME_CHAR is in the same category
> > but have different naming schemes. Can these are named in the same way?
> >
>
> fixed
>
>
> > - The current transoformer seems to using up to one token stack
> > depth. Maybe the stack is needless. (pushed token is always
> > popped just after)
> >
>
> fixed

Thank you.

I found another (and should be the last, so sorry..) functional
defect in this. This doesn't add default namespace if the tag
name in a predicate is 'and' or 'or'. It needs to be fixed, or
wrote in the documentation as a restriction. (seem hard to fix
it..)

create table t1 (id int, doc xml);
insert into t1 values (1, '<rows xmlns="http://x.y"><row><val>50</val><and>60</and></row></rows>');
select x.* from t1, xmltable(XMLNAMESPACES('http://x.y' AS x),
'/x:rows/x:row' passing t1.doc columns data int PATH
'x:val[../x:and = 60]') as x;
data
------
50
(1 row)
select x.* from t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'),
'/rows/row' passing t1.doc columns data int PATH
'val[../and = 60]') as x;
data
------

(1 row)

Other comments are follows.

- Please add more comments. XPATH_TOKEN_NAME in _transformXPath
in my patch has more

- Debug output might be needed.

# sorry now time's up. will continue tomorrow.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center