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

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.stehule@gmail.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.stehule@gmail.com> wrote in <CAFj8pRCZ8oneG7g2vxs9ux71n8A9twwUO7zQpJiuz+7RGSpSuw@mail.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.

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.

- NODENAME_FIRSTCHAR and IS_NODENAME_CHAR is in the same category
  but have different naming schemes. Can these are named in the same way?

- The current transoformer seems to using up to one token stack
  depth. Maybe the stack is needless. (pushed token is always
  popped just after)

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center