Thread: [PoC] Add CANONICAL option to xmlserialize

Hi,

In order to compare pairs of XML documents for equivalence it is 
necessary to convert them first to their canonical form, as described at 
W3C Canonical XML 1.1.[1] This spec basically defines a standard 
physical representation of xml documents that have more then one 
possible representation, so that it is possible to compare them, e.g. 
forcing UTF-8 encoding, entity reference replacement, attributes 
normalization, etc.

Although it is not part of the XML/SQL standard, it would be nice to 
have the option CANONICAL in xmlserialize. Additionally, we could also 
add the attribute WITH [NO] COMMENTS to keep or remove xml comments from 
the documents.

Something like this:

WITH t(col) AS (
  VALUES
   ('<?xml version="1.0" encoding="ISO-8859-1"?>
   <!DOCTYPE doc SYSTEM "doc.dtd" [
   <!ENTITY val "42">
   <!ATTLIST xyz attr CDATA "default">
   ]>

   <!-- ordering of attributes -->
   <foo ns:c = "3" ns:b = "2" ns:a = "1"
     xmlns:ns="http://postgresql.org">

     <!-- Normalization of whitespace in start and end tags -->
     <!-- Elimination of superfluous namespace declarations,
          as already declared in <foo> -->
  <bar     xmlns:ns="http://postgresql.org" >&val;</bar     >

     <!-- Empty element conversion to start-end tag pair -->
     <empty/>

     <!-- Effect of transcoding from a sample encoding to UTF-8 -->
     <iso8859>©</iso8859>

     <!-- Addition of default attribute -->
     <!-- Whitespace inside tag preserved -->
     <xyz> 321 </xyz>
   </foo>
   <!-- comment outside doc -->'::xml)
)
SELECT xmlserialize(DOCUMENT col AS text CANONICAL) FROM t;
xmlserialize

--------------------------------------------------------------------------------------------------------------------------------------------------------
  <foo xmlns:ns="http://postgresql.org" ns:a="1" ns:b="2" 
ns:c="3"><bar>42</bar><empty></empty><iso8859>©</iso8859><xyz 
attr="default"> 321 </xyz></foo>
(1 row)

-- using WITH COMMENTS

WITH t(col) AS (
  VALUES
   (' <foo ns:c = "3" ns:b = "2" ns:a = "1"
     xmlns:ns="http://postgresql.org">
     <!-- very important comment -->
     <xyz> 321 </xyz>
   </foo>'::xml)
)
SELECT xmlserialize(DOCUMENT col AS text CANONICAL WITH COMMENTS) FROM t;
xmlserialize

------------------------------------------------------------------------------------------------------------------------
  <foo xmlns:ns="http://postgresql.org" ns:a="1" ns:b="2" ns:c="3"><!-- 
very important comment --><xyz> 321 </xyz></foo>
(1 row)


Another option would be to simply create a new function, e.g. 
xmlcanonical(doc xml, keep_comments boolean), but I'm not sure if this 
would be the right approach.

Attached a very short draft. What do you think?

Best, Jim

1- https://www.w3.org/TR/xml-c14n11/

On 27.02.23 14:16, I wrote:
> Hi,
>
> In order to compare pairs of XML documents for equivalence it is 
> necessary to convert them first to their canonical form, as described 
> at W3C Canonical XML 1.1.[1] This spec basically defines a standard 
> physical representation of xml documents that have more then one 
> possible representation, so that it is possible to compare them, e.g. 
> forcing UTF-8 encoding, entity reference replacement, attributes 
> normalization, etc.
>
> Although it is not part of the XML/SQL standard, it would be nice to 
> have the option CANONICAL in xmlserialize. Additionally, we could also 
> add the attribute WITH [NO] COMMENTS to keep or remove xml comments 
> from the documents.
>
> Something like this:
>
> WITH t(col) AS (
>  VALUES
>   ('<?xml version="1.0" encoding="ISO-8859-1"?>
>   <!DOCTYPE doc SYSTEM "doc.dtd" [
>   <!ENTITY val "42">
>   <!ATTLIST xyz attr CDATA "default">
>   ]>
>
>   <!-- ordering of attributes -->
>   <foo ns:c = "3" ns:b = "2" ns:a = "1"
>     xmlns:ns="http://postgresql.org">
>
>     <!-- Normalization of whitespace in start and end tags -->
>     <!-- Elimination of superfluous namespace declarations,
>          as already declared in <foo> -->
>  <bar     xmlns:ns="http://postgresql.org" >&val;</bar     >
>
>     <!-- Empty element conversion to start-end tag pair -->
>     <empty/>
>
>     <!-- Effect of transcoding from a sample encoding to UTF-8 -->
>     <iso8859>©</iso8859>
>
>     <!-- Addition of default attribute -->
>     <!-- Whitespace inside tag preserved -->
>     <xyz> 321 </xyz>
>   </foo>
>   <!-- comment outside doc -->'::xml)
> )
> SELECT xmlserialize(DOCUMENT col AS text CANONICAL) FROM t;
> xmlserialize
>
--------------------------------------------------------------------------------------------------------------------------------------------------------

>
>  <foo xmlns:ns="http://postgresql.org" ns:a="1" ns:b="2" 
> ns:c="3"><bar>42</bar><empty></empty><iso8859>©</iso8859><xyz 
> attr="default"> 321 </xyz></foo>
> (1 row)
>
> -- using WITH COMMENTS
>
> WITH t(col) AS (
>  VALUES
>   (' <foo ns:c = "3" ns:b = "2" ns:a = "1"
>     xmlns:ns="http://postgresql.org">
>     <!-- very important comment -->
>     <xyz> 321 </xyz>
>   </foo>'::xml)
> )
> SELECT xmlserialize(DOCUMENT col AS text CANONICAL WITH COMMENTS) FROM t;
> xmlserialize
>
------------------------------------------------------------------------------------------------------------------------

>
>  <foo xmlns:ns="http://postgresql.org" ns:a="1" ns:b="2" ns:c="3"><!-- 
> very important comment --><xyz> 321 </xyz></foo>
> (1 row)
>
>
> Another option would be to simply create a new function, e.g. 
> xmlcanonical(doc xml, keep_comments boolean), but I'm not sure if this 
> would be the right approach.
>
> Attached a very short draft. What do you think?
>
> Best, Jim
>
> 1- https://www.w3.org/TR/xml-c14n11/

The attached version includes documentation and tests to the patch.

I hope things are clearer now :)

Best, Jim

On Mon, Mar 6, 2023 at 7:44 AM Jim Jones <jim.jones@uni-muenster.de> wrote:
> The attached version includes documentation and tests to the patch.

The CI run for that failed in an interesting way, only on Debian +
Meson, 32 bit.  The diffs appear to show that psql has a different
opinion of the column width, while building its header (the "------"
you get at the top of psql's output), even though the actual column
contents was the same.  regression.diff[2] shows that there is a "£1"
in the output, which is how UTF-8 "£1" looks if you view it with
Latin1 glasses on.  Clearly this patch involves transcoding, Latin1
and UTF-8 and I haven't studied it, but it's pretty weird for the 32
bit build to give a different result...  could be something to do with
our environment, since .cirrus.yml sets LANG=C in the 32 bit test run
-- maybe try that locally?

That run also generated a core dump, but I think that's just our open
SIGQUIT problem[3] and not relevant here.

[1] https://cirrus-ci.com/build/6319462375227392
[2]
https://api.cirrus-ci.com/v1/artifact/task/5800598633709568/testrun/build-32/testrun/regress/regress/regression.diffs
[3] https://www.postgresql.org/message-id/flat/20230214202927.xgb2w6b7gnhq6tvv%40awork3.anarazel.de

On 05.03.23 22:00, Thomas Munro wrote:
> The CI run for that failed in an interesting way, only on Debian +
> Meson, 32 bit.  The diffs appear to show that psql has a different
> opinion of the column width, while building its header (the "------"
> you get at the top of psql's output), even though the actual column
> contents was the same.  regression.diff[2] shows that there is a "£1"
> in the output, which is how UTF-8 "£1" looks if you view it with
> Latin1 glasses on.  Clearly this patch involves transcoding, Latin1
> and UTF-8
One of the use cases of this patch is exactly the transcoding of a non 
utf-8 document to utf-8 - as described in the XML canonical spec.
> and I haven't studied it, but it's pretty weird for the 32
> bit build to give a different result...
Yeah, it's pretty weird indeed. I'll try to reproduce this environment 
in a container to see if I get the same diff. Although I'm not sure that 
by "fixing" the result set for this environment it won't break all the 
others.
> could be something to do with
> our environment, since .cirrus.yml sets LANG=C in the 32 bit test run
> -- maybe try that locally?
Also using LANGUAGE=C the result is the same for me - all tests pass 
just fine.
> That run also generated a core dump, but I think that's just our open
> SIGQUIT problem[3] and not relevant here.
>
> [1] https://cirrus-ci.com/build/6319462375227392
> [2]
https://api.cirrus-ci.com/v1/artifact/task/5800598633709568/testrun/build-32/testrun/regress/regress/regression.diffs
> [3] https://www.postgresql.org/message-id/flat/20230214202927.xgb2w6b7gnhq6tvv%40awork3.anarazel.de

Thanks for the quick reply. Much appreciated!

On Mon, Mar 6, 2023 at 11:20 AM Jim Jones <jim.jones@uni-muenster.de> wrote:
> On 05.03.23 22:00, Thomas Munro wrote:
> > could be something to do with
> > our environment, since .cirrus.yml sets LANG=C in the 32 bit test run
> > -- maybe try that locally?

> Also using LANGUAGE=C the result is the same for me - all tests pass
> just fine.

I couldn't reproduce that locally either, but I just tested on CI with
your patch applied  saw the failure, and then removed
"PYTHONCOERCECLOCALE=0 LANG=C" and it's all green:

https://github.com/macdice/postgres/commit/91999f5d13ac2df6f7237a301ed6cf73f2bb5b6d

Without looking too closely, my first guess would have been that this
just isn't going to work without UTF-8 database encoding, so you might
need to skip the test (see for example
src/test/regress/expected/unicode_1.out).  It's annoying that "xml"
already has 3 expected variants... hmm.  BTW shouldn't it be failing
in a more explicit way somewhere sooner if the database encoding is
not UTF-8, rather than getting confused?

On 06.03.23 00:32, Thomas Munro wrote:
> I couldn't reproduce that locally either, but I just tested on CI with
> your patch applied  saw the failure, and then removed
> "PYTHONCOERCECLOCALE=0 LANG=C" and it's all green:
>
> https://github.com/macdice/postgres/commit/91999f5d13ac2df6f7237a301ed6cf73f2bb5b6d
>
> Without looking too closely, my first guess would have been that this
> just isn't going to work without UTF-8 database encoding, so you might
> need to skip the test (see for example
> src/test/regress/expected/unicode_1.out).  It's annoying that "xml"
> already has 3 expected variants... hmm.  BTW shouldn't it be failing
> in a more explicit way somewhere sooner if the database encoding is
> not UTF-8, rather than getting confused?

I guess this confusion is happening because xml_parse() was being called 
with the database encoding from GetDatabaseEncoding().

I added a condition before calling xml_parse() to check if the xml 
document has a different encoding than UTF-8

parse_xml_decl(xml_text2xmlChar(data), NULL, NULL, &encodingStr, NULL);
encoding = encodingStr ? xmlChar_to_encoding(encodingStr) : PG_UTF8;

doc = xml_parse(data, XMLOPTION_DOCUMENT, false, encoding, NULL);

v2 attached.

Thanks!

Best, Jim

On 06.03.23 11:50, I wrote:
> I guess this confusion is happening because xml_parse() was being 
> called with the database encoding from GetDatabaseEncoding().
>
> I added a condition before calling xml_parse() to check if the xml 
> document has a different encoding than UTF-8
>
> parse_xml_decl(xml_text2xmlChar(data), NULL, NULL, &encodingStr, NULL);
> encoding = encodingStr ? xmlChar_to_encoding(encodingStr) : PG_UTF8;
>
> doc = xml_parse(data, XMLOPTION_DOCUMENT, false, encoding, NULL);

It seems that this bug fix didn't change the output of the CI on Debian 
+ Meson, 32bit.

I slightly changed the test case to a character that both encodings can 
deal with.

v3 attached.

v4 attached fixes an encoding issue at the xml_parse call. It now uses 
GetDatabaseEncoding().

Best, Jim

v5 attached is a rebase over the latest changes in xmlserialize (INDENT 
output).

After some more testing I realized that v5 was leaking the xmlDocPtr.

Now fixed in v6.

'Persistent worker failed to start the task: tart isolation failed: failed to create VM cloned from "ghcr.io/cirruslabs/macos-ventura-base:latest": tart command returned non-zero exit code: ""'

Is this a problem in my code or in the CI itself?

Thanks!

Jim

On Thu, Sep 14, 2023 at 11:54 PM Jim Jones <jim.jones@uni-muenster.de> wrote:
> The cfbot started complaining about this patch on "macOS - Ventura - Meson"
>
> 'Persistent worker failed to start the task: tart isolation failed: failed to create VM cloned from
"ghcr.io/cirruslabs/macos-ventura-base:latest":tart command returned non-zero exit code: ""' 
>
> Is this a problem in my code or in the CI itself?

There was a temporary glitch on one of the new Mac CI runner machines
that caused a few tests to fail like that, but it was fixed so that
should turn red again later today.

On Fri, 17 Mar 2023 at 18:01, Jim Jones <jim.jones@uni-muenster.de> wrote:
>
> After some more testing I realized that v5 was leaking the xmlDocPtr.
>
> Now fixed in v6.

Few comments:
1) Why the default option was chosen without comments shouldn't it be
the other way round?
+opt_xml_serialize_format:
+                       INDENT
                         { $$ = XMLSERIALIZE_INDENT; }
+                       | NO INDENT
                         { $$ = XMLSERIALIZE_NO_FORMAT; }
+                       | CANONICAL
                         { $$ = XMLSERIALIZE_CANONICAL; }
+                       | CANONICAL WITH NO COMMENTS
 { $$ = XMLSERIALIZE_CANONICAL; }
+                       | CANONICAL WITH COMMENTS
         { $$ = XMLSERIALIZE_CANONICAL_WITH_COMMENTS; }
+                       | /*EMPTY*/
                         { $$ = XMLSERIALIZE_NO_FORMAT; }

2) This should be added to typedefs.list:
+typedef enum XmlSerializeFormat
+{
+       XMLSERIALIZE_INDENT,                                    /*
pretty-printed xml serialization  */
+       XMLSERIALIZE_CANONICAL,                                 /*
canonical form without xml comments */
+       XMLSERIALIZE_CANONICAL_WITH_COMMENTS,   /* canonical form with
xml comments */
+       XMLSERIALIZE_NO_FORMAT                                  /*
unformatted xml representation */
+} XmlSerializeFormat;

3) This change is not required:
        return result;
+
 #else
        NO_XML_SUPPORT();
        return NULL;

4) This comment body needs slight reformatting:
+       /*
+       * Parse the input according to the xmloption.
+       * XML canonical expects a well-formed XML input, so here in case of
+       * XMLSERIALIZE_CANONICAL or XMLSERIALIZE_CANONICAL_WITH_COMMENTS we
+       * force xml_parse() to parse 'data' as XMLOPTION_DOCUMENT despite
+       * of the XmlOptionType given in 'xmloption_arg'. This enables the
+       * canonicalization of CONTENT fragments if they contain a singly-rooted
+       * XML - xml_parse() will thrown an error otherwise.
+       */

5) Similarly here too:
-                       if (newline == NULL || xmlerrcxt->err_occurred)
+                       * Emit declaration only if the input had one.
Note: some versions of
+                       * xmlSaveToBuffer leak memory if a non-null
encoding argument is
+                       * passed, so don't do that.  We don't want any
encoding conversion
+                       * anyway.
+                       */
+                       if (decl_len == 0)

6) Similarly here too:
+                               /*
+                               * Deal with the case where we have
non-singly-rooted XML.
+                               * libxml's dump functions don't work
well for that without help.
+                               * We build a fake root node that
serves as a container for the
+                               * content nodes, and then iterate over
the nodes.
+                               */

7) Similarly here too:
+                               /*
+                               * We use this node to insert newlines
in the dump.  Note: in at
+                               * least some libxml versions,
xmlNewDocText would not attach the
+                               * node to the document even if we
passed it.  Therefore, manage
+                               * freeing of this node manually, and
pass NULL here to make sure
+                               * there's not a dangling link.
+                               */

8) Should this:
+       * of the XmlOptionType given in 'xmloption_arg'. This enables the
+       * canonicalization of CONTENT fragments if they contain a singly-rooted
+       * XML - xml_parse() will thrown an error otherwise.
Be:
+       * of the XmlOptionType given in 'xmloption_arg'. This enables the
+       * canonicalization of CONTENT fragments if they contain a singly-rooted
+       * XML - xml_parse() will throw an error otherwise.

Regards,
Vignesh

Hi Vignesh

Thanks for the thorough review!

On 04.10.23 11:39, vignesh C wrote:
> Few comments:
> 1) Why the default option was chosen without comments shouldn't it be
> the other way round?
> +opt_xml_serialize_format:
> +                       INDENT
>                           { $$ = XMLSERIALIZE_INDENT; }
> +                       | NO INDENT
>                           { $$ = XMLSERIALIZE_NO_FORMAT; }
> +                       | CANONICAL
>                           { $$ = XMLSERIALIZE_CANONICAL; }
> +                       | CANONICAL WITH NO COMMENTS
>   { $$ = XMLSERIALIZE_CANONICAL; }
> +                       | CANONICAL WITH COMMENTS
>           { $$ = XMLSERIALIZE_CANONICAL_WITH_COMMENTS; }
> +                       | /*EMPTY*/
>                           { $$ = XMLSERIALIZE_NO_FORMAT; }
I'm not sure it is the way to go. The main idea is to check if two 
documents have the same content, and comments might be different even if 
the contents of two documents are identical. What are your concerns 
regarding this default behaviour?
> 2) This should be added to typedefs.list:
> +typedef enum XmlSerializeFormat
> +{
> +       XMLSERIALIZE_INDENT,                                    /*
> pretty-printed xml serialization  */
> +       XMLSERIALIZE_CANONICAL,                                 /*
> canonical form without xml comments */
> +       XMLSERIALIZE_CANONICAL_WITH_COMMENTS,   /* canonical form with
> xml comments */
> +       XMLSERIALIZE_NO_FORMAT                                  /*
> unformatted xml representation */
> +} XmlSerializeFormat;
added.
> 3) This change is not required:
>          return result;
> +
>   #else
>          NO_XML_SUPPORT();
>          return NULL;
removed.
>
> 4) This comment body needs slight reformatting:
> +       /*
> +       * Parse the input according to the xmloption.
> +       * XML canonical expects a well-formed XML input, so here in case of
> +       * XMLSERIALIZE_CANONICAL or XMLSERIALIZE_CANONICAL_WITH_COMMENTS we
> +       * force xml_parse() to parse 'data' as XMLOPTION_DOCUMENT despite
> +       * of the XmlOptionType given in 'xmloption_arg'. This enables the
> +       * canonicalization of CONTENT fragments if they contain a singly-rooted
> +       * XML - xml_parse() will thrown an error otherwise.
> +       */
reformatted.
> 5) Similarly here too:
> -                       if (newline == NULL || xmlerrcxt->err_occurred)
> +                       * Emit declaration only if the input had one.
> Note: some versions of
> +                       * xmlSaveToBuffer leak memory if a non-null
> encoding argument is
> +                       * passed, so don't do that.  We don't want any
> encoding conversion
> +                       * anyway.
> +                       */
> +                       if (decl_len == 0)
reformatted.
> 6) Similarly here too:
> +                               /*
> +                               * Deal with the case where we have
> non-singly-rooted XML.
> +                               * libxml's dump functions don't work
> well for that without help.
> +                               * We build a fake root node that
> serves as a container for the
> +                               * content nodes, and then iterate over
> the nodes.
> +                               */
reformatted.
> 7) Similarly here too:
> +                               /*
> +                               * We use this node to insert newlines
> in the dump.  Note: in at
> +                               * least some libxml versions,
> xmlNewDocText would not attach the
> +                               * node to the document even if we
> passed it.  Therefore, manage
> +                               * freeing of this node manually, and
> pass NULL here to make sure
> +                               * there's not a dangling link.
> +                               */
reformatted.
> 8) Should this:
> +       * of the XmlOptionType given in 'xmloption_arg'. This enables the
> +       * canonicalization of CONTENT fragments if they contain a singly-rooted
> +       * XML - xml_parse() will thrown an error otherwise.
> Be:
> +       * of the XmlOptionType given in 'xmloption_arg'. This enables the
> +       * canonicalization of CONTENT fragments if they contain a singly-rooted
> +       * XML - xml_parse() will throw an error otherwise.

I didn't understand the suggestion in 8) :)

Thanks again for the review. Much appreciated!

v7 attached.

Best, Jim

On 2023-10-04 12:19, Jim Jones wrote:
> On 04.10.23 11:39, vignesh C wrote:
>> 1) Why the default option was chosen without comments shouldn't it be
>> the other way round?
> I'm not sure it is the way to go. The main idea is to check if two 
> documents have the same content, and comments might be different even 
> if the contents of two documents are identical. What are your concerns 
> regarding this default behaviour?

I hope I'm not butting in, but I too would be leery of any default
behavior that's going to say thing1 and thing2 are the same thing
but ignoring (name part of thing here). If that's the comparison
I mean to make, and it's as easy as CANONICAL WITHOUT COMMENTS
to say that's what I mean, I'd be happy to write that. It also means
that the next person reading my code will know "oh, he means
'same' in *that* way", without having to think about it.

Regards,
-Chap

Hi Chap

On 04.10.23 23:05, Chapman Flack wrote:
> I hope I'm not butting in, but I too would be leery of any default
> behavior that's going to say thing1 and thing2 are the same thing
> but ignoring (name part of thing here). If that's the comparison
> I mean to make, and it's as easy as CANONICAL WITHOUT COMMENTS
> to say that's what I mean, I'd be happy to write that. It also means
> that the next person reading my code will know "oh, he means
> 'same' in *that* way", without having to think about it.

That's a very compelling argument. Thanks for that!

It is indeed clearer to only remove items from the result set if 
explicitly said so.

v8 attached changes de default behaviour to WITH COMMENTS.

Best,

Jim

On 05.10.23 09:38, Jim Jones wrote:
>
> v8 attached changes de default behaviour to WITH COMMENTS.
v9 attached with rebase due to changes done to primnodes.h in 615f5f6

-- 
Jim

On 09.02.24 14:19, Jim Jones wrote:
> v9 attached with rebase due to changes done to primnodes.h in 615f5f6
>
v10 attached with rebase due to changes in primnodes, parsenodes.h, and
gram.y

-- 
Jim

On 19.06.24 10:59, Jim Jones wrote:
> On 09.02.24 14:19, Jim Jones wrote:
>> v9 attached with rebase due to changes done to primnodes.h in 615f5f6
>>
> v10 attached with rebase due to changes in primnodes, parsenodes.h, and
> gram.y
>
v11 attached with rebase due to changes in xml.c

-- 
Jim

Hi Pavel

On 25.08.24 20:57, Pavel Stehule wrote:
>
> There is unwanted white space in the patch
>
> -<-><--><-->xmlFreeDoc(doc);
> +<->else if (format == XMLSERIALIZE_CANONICAL || format ==
> XMLSERIALIZE_CANONICAL_WITH_NO_COMMENTS)
> + <>{
> +<-><-->xmlChar    *xmlbuf = NULL;
> +<-><-->int         nbytes;
> +<-><-->int    
>
I missed that one. Just removed it, thanks!
> 1. the xml is serialized to UTF8 string every time, but when target
> type is varchar or text, then it should be every time encoded to
> database encoding. Is not possible to hold utf8 string in latin2
> database varchar.
I'm calling xml_parse using GetDatabaseEncoding(), so I thought I would
be on the safe side

if(format ==XMLSERIALIZE_CANONICAL ||format
==XMLSERIALIZE_CANONICAL_WITH_NO_COMMENTS)
doc =xml_parse(data, XMLOPTION_DOCUMENT, false,
GetDatabaseEncoding(), NULL, NULL, NULL);
... or you mean something else?

> 2. The proposed feature can increase some confusion in implementation
> of NO IDENT. I am not an expert on this area, so I checked other
> databases. DB2 does not have anything similar. But Oracle's "NO IDENT"
> clause is very similar to the proposed "CANONICAL". Unfortunately,
> there is different behaviour of NO IDENT - Oracle's really removes
> formatting, Postgres does nothing.

Coincidentally, the [NO] INDENT support for xmlserialize is an old patch
of mine.
It basically "does nothing" and prints the xml as is, e.g.

SELECT xmlserialize(DOCUMENT '<foo><bar><val z="1"
a="8"><![CDATA[0&1]]></val></bar></foo>' AS text INDENT);
                xmlserialize                
--------------------------------------------
 <foo>                                     +
   <bar>                                   +
     <val z="1" a="8"><![CDATA[0&1]]></val>+
   </bar>                                  +
 </foo>                                    +
 
(1 row)

SELECT xmlserialize(DOCUMENT '<foo><bar><val z="1"
a="8"><![CDATA[0&1]]></val></bar></foo>' AS text NO INDENT);
                         xmlserialize                         
--------------------------------------------------------------
 <foo><bar><val z="1" a="8"><![CDATA[0&1]]></val></bar></foo>
(1 row)

SELECT xmlserialize(DOCUMENT '<foo><bar><val z="1"
a="8"><![CDATA[0&1]]></val></bar></foo>' AS text);
                         xmlserialize                         
--------------------------------------------------------------
 <foo><bar><val z="1" a="8"><![CDATA[0&1]]></val></bar></foo>
(1 row)

.. while CANONICAL converts the xml to its canonical form,[1,2] e.g.
sorting attributes and replacing CDATA strings by its value:

SELECT xmlserialize(DOCUMENT '<foo><bar><val z="1"
a="8"><![CDATA[0&1]]></val></bar></foo>' AS text CANONICAL);
                     xmlserialize                     
------------------------------------------------------
 <foo><bar><val a="8" z="1">0&1</val></bar></foo>
(1 row)

xmlserialize CANONICAL does not exist in any other database and it's not
part of the SQL/XML standard.

Regarding the different behaviour of NO INDENT in Oracle and PostgreSQL:
it is not entirely clear to me if SQL/XML states that NO INDENT must
remove the indentation from xml strings.
It says:

"INDENT — the choice of whether to “pretty-print” the serialized XML by
means of indentation, either
True or False.
....
i) If <XML serialize indent> is specified and does not contain NO, then
let IND be True.
ii) Otherwise, let IND be False."

When I wrote the patch I assumed it meant to leave the xml as is .. but
I might be wrong.
Perhaps it would be best if we open a new thread for this topic.

Thank you for reviewing this patch. Much appreciated!

Best,

-- 
Jim

1 - https://www.w3.org/TR/xml-c14n11/
2 - https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-c14n.html

On 26.08.24 12:30, Pavel Stehule wrote:
> I think so there should be specified the target of CANONICAL - it is a
> partial replacement of NO INDENT or it produces format  just for
> comparing? The CANONICAL format is not probably extra standardized,
> because libxml2 removes indenting, but examples in
> https://www.w3.org/TR/xml-c14n11/ doesn't do it. So this format makes
> sense just for local operations.
My idea with CANONICAL was not to replace NO INDENT. The intent was to
format xml strings in an standardized way, so that they can be compared.
For instance, removing comments, sorting attributes, converting CDATA
strings, converting empty elements to start-end tag pairs, removing
white spaces between elements, etc ...

The W3C recommendation for Canonical XML[1] dictates the following
regarding the removal of whitespaces between elements :

* Whitespace outside of the document element and within start and end
tags is normalized
* All whitespace in character content is retained (excluding characters
removed during line feed normalization)

>
> I like this functionality, and it is great so the functionality from
> libxml2 can be used, but I think, so the fact that there are  four not
> compatible implementations of xmlserialize is messy. Can be nice, if 
> we find some intersection between SQL/XML, Oracle  instead of new
> proprietary syntax. 
>
> In Oracle syntax the CANONICAL is +/- NO INDENT SHOW DEFAULT ?

No.
XMLSERIALIZE ... NO INDENT is supposed, as the name suggests, to
serialize an xml string without indenting it. One could argue that not
indenting can be translated as removing indentation, but I couldn't find
anything concrete about this in the SQL/XML spec. If it's indeed the
case, we should correct XMLSERIALIZE .. NO INDENT, but it is unrelated
to this patch.

CANONICAL serializes a physical representation of an xml document. In a
nutshell, XMLSERIALIZE ... CANONICAL sort of "rewrites" the xml string
with the following rules (list from the W3C recommendation):

* The document is encoded in UTF-8
* Line breaks normalized to #xA on input, before parsing
* Attribute values are normalized, as if by a validating processor
* Character and parsed entity references are replaced
* CDATA sections are replaced with their character content
* The XML declaration and document type declaration are removed
* Empty elements are converted to start-end tag pairs
* Whitespace outside of the document element and within start and end
tags is normalized
* All whitespace in character content is retained (excluding characters
removed during line feed normalization)
* Attribute value delimiters are set to quotation marks (double quotes)
* Special characters in attribute values and character content are
replaced by character references
* Superfluous namespace declarations are removed from each element
* Default attributes are added to each element
* Fixup of xml:base attributes [C14N-Issues] is performed
* Lexicographic order is imposed on the namespace declarations and
attributes of each element

btw: Oracle's SIZE =, HIDE DEFAULTS, and SHOW DEFAULTS are not part of
the SQL/XML standard either :)

> My objection against CANONICAL so SQL/XML and Oracle allows to
> parametrize XMLSERIALIZE more precious and before implementing new
> feature, we should to clean table and say, what we want to have in
> XMLSERIALIZE.
>
> An alternative of enhancing of XMLSERIALIZE I can imagine just
> function "to_canonical(xml, without_comments bool default false)". In
> this case we don't need to solve relations against SQL/XML or Oracle.

To create a separated serialization function would be IMHO way less
elegant than to parametrize XMLSERIALIZE, but it would be something I
could live with in case we decide to go down this path.

Thanks!

-- 
Jim

1 - https://www.w3.org/TR/xml-c14n11/

On 26.08.24 14:15, Pavel Stehule wrote:
> I am not strongly against enhancing XMLSERIALIZE, but it can be nice
> to see some wider concept first. Currently the state looks just random
> - and I didn't see any serious discussion about implementation fo
> SQL/XML. We don't need to be necessarily compatible with Oracle, but
> it can help if we have a functionality that can be used for conversions.

Fair point. A road map definitely wouldn't hurt. Not quite sure how to
start this motion though :D
So far I've just picked the missing SQL/XML features that were listed in
the PostgreSQL todo list and that I need for any of my projects. But I
would gladly change the priorities if there is any interest in the
community for specific features.

-- 
Jim

On 26.08.24 16:59, Pavel Stehule wrote:
>
> 1. what about behaviour of NO INDENT - the implementation is not too
> old, so it can be changed if we want (I think), and it is better to do
> early than too late

While checking the feasibility of removing indentation with NO INDENT I
may have found a bug in XMLSERIALIZE ... INDENT.
xmlSaveToBuffer seems to ignore elements if there are whitespaces
between them:

SELECT xmlserialize(DOCUMENT '<foo><bar>42</bar></foo>' AS text INDENT);
  xmlserialize   
-----------------
 <foo>          +
   <bar>42</bar>+
 </foo>         +
 
(1 row)

SELECT xmlserialize(DOCUMENT '<foo> <bar>42</bar> </foo>'::xml AS text
INDENT);
        xmlserialize        
----------------------------
 <foo> <bar>42</bar> </foo>+
 
(1 row)

I'll take a look at it.

Regarding removing indentation: yes, it would be possible with libxml2.
The question is if it would be right to do so.
> 2. Are we able to implement SQL/XML syntax with libxml2?
>
> 3. Are we able to implement Oracle syntax with libxml2? And there are
> benefits other than higher possible compatibility?
I guess it would be beneficial if you're migrating from oracle to
postgres - or the other way around. It certainly wouldn't hurt, but so
far I personally had little use for the oracle's extra xmlserialize
features.
>
> 4. Can there be some possible collision (functionality, syntax) with
> CANONICAL?
I couldn't find anything in the SQL/XML spec that might refer to
canonocal xml.
>
> 5. SQL/XML XMLSERIALIZE supports other target types than varchar. I
> can imagine XMLSERIALIZE with CANONICAL to bytea (then we don't need
> to force database encoding). Does it make sense? Are the results
> comparable?
|
As of pg16 bytea is not supported. Currently type| can be |character|,
|character varying|, or |text - also their other flavours like 'name'.

|

-- 
Jim

On 29.08.24 20:50, Pavel Stehule wrote:
>
> I know, but theoretically, there can be some benefit for CANONICAL if
> pg supports bytea there. Lot of databases still use non utf8 encoding.
>
> It is a more theoretical question - if pg supports different types
> there in future  (because SQL/XML or Oracle), then CANONICAL can be
> used without limit,
I like the idea of extending the feature to support bytea. I can
definitely take a look at it, but perhaps in another patch? This change
would most likely involve transformXmlSerialize in parse_expr.c, and I'm
not sure of the impact in other usages of XMLSERIALIZE.
> or CANONICAL can be used just for text? And you are sure, so you can
> compare text X text, instead xml X xml?
Yes, currently it only supports varchar or text - and their cousins. The
idea is to format the xml and serialize it as text in a way that they
can compared based on their content, independently of how they were
written, e.g '<foo a="1" b="2"/>' is equal to '<foo b="2" a="1"/>'.

>
> +SELECT xmlserialize(CONTENT doc AS text CANONICAL) =
> xmlserialize(CONTENT doc AS text CANONICAL WITH COMMENTS) FROM
> xmltest_serialize;
> + ?column?
> +----------
> + t
> + t
> +(2 rows)
>
> Maybe I am a little bit confused by these regress tests, because at
> the end it is not too useful - you compare two identical XML, and WITH
> COMMENTS and WITHOUT COMMENTS is tested elsewhere. I tried to search
> for a sense of this test.  Better to use really different documents
> (columns) instead.

Yeah, I can see that it's confusing. In this example I actually just
wanted to test that the default option of CANONICAL is CANONICAL WITH
COMMENTS, even if you don't mention it. In the docs I mentioned it like
this:

"The optional parameters WITH COMMENTS (which is the default) or WITH NO
COMMENTS, respectively, keep or remove XML comments from the given
document."

Perhaps I should rephrase it? Or maybe a comment in the regression tests
would suffice?

Thanks a lot for the input!

-- 
Jim

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

LGTM

The new status of this patch is: Ready for Committer

On Sun, 2024-09-08 at 11:43 +0000, Oliver Ford wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, failed
> Implements feature:       tested, failed
> Spec compliant:           tested, failed
> Documentation:            tested, failed
>
> LGTM
>
> The new status of this patch is: Ready for Committer

Huh?  Do you mean "tested, passes"?

Yours,
Laurenz Albe

On Sun, Sep 8, 2024 at 2:44 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Sun, 2024-09-08 at 11:43 +0000, Oliver Ford wrote:
> > The following review has been posted through the commitfest application:
> > make installcheck-world:  tested, failed
> > Implements feature:       tested, failed
> > Spec compliant:           tested, failed
> > Documentation:            tested, failed
> >
> > LGTM
> >
> > The new status of this patch is: Ready for Committer
>
> Huh?  Do you mean "tested, passes"?
>
> Yours,
> Laurenz Albe

Whoops, yes all tests and docs pass!

Hi Oliver

On 08.09.24 15:56, Oliver Ford wrote:
> Whoops, yes all tests and docs pass!

Thanks for the review!

Best, Jim

Jim Jones <jim.jones@uni-muenster.de> writes:
> This patch introduces the CANONICAL option to xmlserialize, which
> serializes xml documents in their canonical form - as described in
> the W3C Canonical XML Version 1.1 specification. This option can
> be used with the additional parameter WITH [NO] COMMENTS to keep
> or remove xml comments from the canonical xml output.

While I don't object to providing this functionality in some form,
I think that doing it with this specific syntax is a seriously
bad idea.  I think there's significant risk that at some point
the SQL committee will either standardize this syntax with a
somewhat different meaning or standardize some other syntax for
the same functionality.

How about instead introducing a plain function along the lines of
"xml_canonicalize(xml, bool keep_comments) returns text" ?  The SQL
committee will certainly never do that, but we won't regret having
created a plain function whenever they get around to doing something
in the same space.

            regards, tom lane