Re: Add XMLNamespaces to XMLElement

On Tue, 21 Jan 2025 at 04:04, Jim Jones <jim(dot)jones(at)uni-muenster(dot)de> wrote:
>
> Hi Umar
>
> On 20.01.25 17:19, Umar Hayat wrote:
> > Hi Jim & Pavel,
> > Sorry for getting back a bit late on this. Few more case you might
> > need consider:
> >
> > As I mentioned in my first static review about XMLTable existing
> > behaviour might change, I give it a run time review and here are few
> > findings:
> >
> > 1. Because of this patch XMLTable namespace will accept NO DEFAULT
> > value, I was expecting an error message based on prior behavior '' but
> > If I run following query it show something different:
> > "
> >> SELECT * FROM XMLTABLE(XMLNAMESPACES(NO DEFAULT),
> > '/rows/row'
> > PASSING '<rows
> > xmlns="http://x.y"><row><a>10</a></row></rows>'
> > COLUMNS a int PATH 'a');
> > ERROR: cache lookup failed for type 0
> > "
> > Can you please re-check this behaviour (there might be something wrong
> > with my build environment)
>
> There is nothing wrong with your environment :) I forgot to test
> XMLTable for NO DEFAULT namespaces. To be consistent with the existing
> code, I added the following condition to transformRangeTableFunc:
>
>
> if (!r->name && !r->val)
> ns_uri = transformExpr(pstate, makeStringConst("", r->location),
> EXPR_KIND_FROM_FUNCTION);
> else
> ns_uri = transformExpr(pstate, r->val, EXPR_KIND_FROM_FUNCTION);
>
>
> It adds an empty string to the uri, which is the value expected for NO
> DEFAULT.
>
> I also added a NO DEFAULT test for XMLTable in xml.sql
>
> v5 attached.
>
> > 2. Seems like namespace as ColumnRef was already allowed (I doubt if
> > that's a correct implementation, I see other DB allows only strings
> > AFAIK), for non-default namespaces may be its fair, should this be
> > allowed for default namespace, any opinion.
> > create table tbl1 ( col1 text);
> > insert into tbl1 values ('abc');
> > insert into tbl1 values ('def');
> > select xmlelement(NAME "root", xmlnamespaces(default col1)) from tbl1;
> > xmlelement
> > ---------------------
> > <root xmlns="abc"/>
> > <root xmlns="def"/>
> > (2 rows)
>
> What are your concerns about supporting ColumnRef for the URI's?
>
> It is currently supported by XMLAttributes:
>
For XMLAttributes attribute it should have ColumnRef/Expr because
that's the data/content we want to generate. But namespaces and xml
tags, IMO they should be considered as part of the structure/schema of
XML. Allowing namespaces (default or otherwise) to be generated
arbitrarily for each record does not seem correct to me, it's like
generating arbitrary XML using print string which does not require XML
functions.

- DB2 allows XMLElements namespace but it does not allow Expr/ColumnRef.
- Oracle Allow namespace in only XMLTable, and it does not allow Expr/ColumnRef.

- Having SConst/String or numeric can limit the error handling at
parsing stage which can validate the schema instead of expression
evaluation per record, which leads to following problem at runtime:

CREATE TABLE xmltab (uri TEXT);
INSERT INTO xmltab VALUES ('good'), ('');
SELECT XMLElement(NAME "root", XMLNamespaces(uri AS zz)) from xmltab;
ERROR: invalid XML namespace URI for "zz"
DETAIL: a regular XML namespace cannot be a zero-length string

Imagine there millions of records and in the middle it fails.

- Also expression evaluation per record and doing validation (valid
URI etc) I believe will have some performance impact ( I haven't
tested it, so can't say for sure , how much )

- It's possible that there might be some other angle that I am missing
right now, maybe Pavel/Àlvaro can shed light on this. I briefly went
through the first implementation thread. There was a bit of discussion
to use b_expr instead of c_expr, but I have yet to explore the
reasoning of not using SConst.

> CREATE TABLE t AS SELECT 'http://x.y' AS uri;
> SELECT xmlelement(NAME el, xmlattributes("uri" AS att)) FROM t;
>
> xmlelement
> ------------------------
> <el att="http://x.y"/>
> (1 row)
>
>
> Many thanks for the review!
>
> Best, Jim
>
>
>
>
> --
> Jim

--
Umar Hayat
Bitnine (https://bitnine.net/)