Обсуждение: Re: Add XMLNamespaces to XMLElement

Hi,
+1 for the enhancement.

I haven't compiled and reviewed the full patch yet, please see a few
comments from my side based on static analysis.

1. Though changes are targeted for XMLNAMESPACES for XMLElement but in
my opinion it will affect XMLTABLE as well because the
'xml_namespace_list' rule is shared now.
Adding 'NO DEFAULT' in xml_namespace_list will allow users to use it
with XMLTABLE XMLNAMESPACES as well.PostgreSQL grammar allow to
specify DEFAULT in NAMESPACE but resulting in following error:
"ERROR:  DEFAULT namespace is not supported"
What would be behavior with this change for XMLTABLE, should this be
allowed and the error messages need to be updated (may be this will
not be an error at all) or we need to restrict users to not use 'NO
DEFAULT' with XMLTable.

2. Should we reuse the 'xml_namespaces' rule for XMLTable, as the
definition is the same.

3. In this patch 'NO DEFAULT' behavior is like DEFAULT '<blank>'
(empty uri) , should not it be more like 'DEFAULT NULL' to result in
the following ?
SELECT xmlelement(NAME "root", xmlnamespaces(NO DEFAULT));
    xmlelement
------------------
<root/>

instead of

SELECT xmlelement(NAME "root", xmlnamespaces(NO DEFAULT));
    xmlelement
------------------
<root xmlns=""/>

Regards
Umar Hayat

On Sat, 21 Dec 2024 at 14:57, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
> so 21. 12. 2024 v 0:51 odesílatel Jim Jones <jim.jones@uni-muenster.de> napsal:
>>
>> Hi,
>>
>> I'd like to propose the implementation of the XMLNamespaces option for
>> XMLElement.
>>
>> XMLNAMESPACES(nsuri AS nsprefix)
>> XMLNAMESPACES(DEFAULT default-nsuri)
>> XMLNAMESPACES(NO DEFAULT)
>>
>> * nsprefix:              Namespace's prefix.
>> * nsuri:                 Namespace's URI.
>> * DEFAULT default-nsuri: Specifies the DEFAULT namespace to use within
>> the scope of a namespace declaration.
>> * NO DEFAULT:            Specifies that NO DEFAULT namespace is to be
>> used within the scope of a namespace declaration.
>>
>> This basically works pretty much like XMLAttributes, but with a few more
>> restrictions (see SQL/XML:2023, 11.2 <XML lexically scoped options>):
>>
>> * XML namespace declaration shall contain at most one DEFAULT namespace
>> declaration item.
>> * No namespace prefix shall be equivalent to xml or xmlns.
>> * No namespace URI shall be identical to http://www.w3.org/2000/xmlns/
>> or to http://www.w3.org/XML/1998/namespace.
>> * The value of a namespace URI contained in an regular namespace
>> declaration item (no DEFAULT) shall not be a zero-length string.
>>
>> Examples:
>>
>> SELECT xmlelement(NAME "foo", xmlnamespaces('http://x.y' AS bar));
>>           xmlelement
>> -------------------------------
>>  <foo xmlns:bar="http://x.y"/>
>>
>> SELECT xmlelement(NAME "foo", xmlnamespaces(DEFAULT 'http://x.y'));
>>         xmlelement
>> ---------------------------
>>  <foo xmlns="http://x.y"/>
>>
>> SELECT xmlelement(NAME "foo", xmlnamespaces(NO DEFAULT));
>>    xmlelement
>> -----------------
>>  <foo xmlns=""/>
>>
>> In transformXmlExpr() it seemed convenient to use the same parameters to
>> store the prefixes and URIs as in XMLAttributes (arg_names and
>> named_args), but I am still not so sure it is the right approach. Is
>> there perhaps a better way?
>>
>> Any thoughts? Feedback welcome!
>
>
> +1
>
> Pavel
>>
>>
>> Best, Jim



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

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)

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)

Changing status in commitfest, feel free to revert back.

Regards
Umar Hayat

On Sun, 19 Jan 2025 at 18:33, Jim Jones <jim.jones@uni-muenster.de> wrote:
>
>
> On 16.01.25 08:36, Pavel Stehule wrote:
> > Now, I have not any objections against the code
> >
> > The patch has doc and enough regress tests
> > The patching and compilation without problems
> > make check-world passed
> >
> > I'll mark this patch as ready for committer
>
>
> rebase due to gram.y changes introduced in 80feb72
>
>
> --
> Jim



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

On Tue, 21 Jan 2025 at 04:04, Jim Jones <jim.jones@uni-muenster.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/)

On 21.01.25 11:48, Umar Hayat wrote:
> 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.


I'm not sure I completely get your 'print string' argument. Namespaces
are added to the element using libxml2's xmlTextWriterWriteAttributeNS
function, so we’re letting libxml2 handle whether a namespace
declaration is valid or not. Of course, there are still some extra
checks required by the SQL/XML standard.


>
> - 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.


I don't think discarding a feature just because the input data might
raise an exception in a long transaction is a strong argument here. For
your specific case, the user can always use a WHERE clause to filter out
any URIs that aren’t valid for XMLNamespace. Additionally, the
documentation already mentions this specific limitation:

"The value of a <replaceable>regular-nsuri</replaceable> cannot be a
zero-length string."

So it shouldn’t really catch anyone off guard :)


Thanks for the review!

Best, Jim

Pavel and Jim,
If that's the case, it looks good to me.
Just wanted to highlight potential issues and implementation
differences compared to other databases.

Regards
Umar Hayat

I've attached v6, which addresses two issues:

* Fixes a bug where XMLNAMESPACES declarations within a view were being
serialized as XMLATTRIBUTES.
* Prevents the makeString function from being called with a NULL
parameter - discussed in this thread [1].

Best regards, Jim

[1]
https://www.postgresql.org/message-id/CAFj8pRC24FEBNfTUrDgAK8f2nqDVvzWCuq%3DR%3DT19nUjL9GuLBA%40mail.gmail.com

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hi Pavel,

I have tested this patch, and it proves to be highly useful when handling XMLNAMESPACES() with both DEFAULT and NO
DEFAULToptions. The following test cases confirm its correctness:
 


SELECT xmlelement(
    NAME "foo",
    XMLNAMESPACES('http://x.y' AS xy, 'http://a.b' AS ab, DEFAULT 'http://d.e'),
    xmlelement(NAME "foot", 
        xmlelement(NAME "xy:shoe"), 
        xmlelement(NAME "ab:lace")
    )
);

SELECT xmlelement(
    NAME "foo",
    XMLNAMESPACES('http://x.y' AS xy, 'http://a.b' AS ab, NO DEFAULT),
    xmlelement(NAME "foot", 
        xmlelement(NAME "xy:shoe"), 
        xmlelement(NAME "ab:lace")
    )
);
Additionally, I verified that the patch correctly supports multiple namespaces when using both DEFAULT and NO DEFAULT,
ensuringexpected behavior across different use cases.
 

Great work on this improvement!

rebase

-- 
Jim

rebased due to recent changes in doc/src/sgml/func.sgml

-- 
Jim