Re: XMLConcat() and XMLRoot() silently drop xml values with an invalid declaration

Hi Jim,

I tested v1 0001 and 0002 on current master (81b2179b492, built with cassert, tap tests and --with-libxml, libxml2 2.9.14). Both apply cleanly, 0002 on top of 0001.

On master I could reproduce the report. xmlroot() with version '1.0"x' stores <?xml version="1.0"x"?><doc>data</doc>, and xmlconcat('<a/>'::xml, x) returns just <a/> with no error. xml_in(xml_out(x)) fails with the WARNING and ERROR you showed. I stored 12 rows with xmlroot() on master. For the rows with version '1.0"x', with a non-ASCII version, and with a standalone="yes" input plus version '1.0"x', xmlconcat() and xmlagg() returned only the other values, and the document was dropped.

With 0001 only, the same queries fail with "could not parse XML declaration in stored value". The DETAIL is "Parsing XML declaration: '?>' expected." for '1.0"x' and "Invalid character value." for the non-ASCII version. xmlroot() over such a stored row fails the same way. xmlroot() could still create these values.

With 0001 and 0002, xmlroot() rejects these versions: '1.0"x', a version with a non-ASCII character, '1.0" standalone="yes' and '1.0"?><evil/><?x "'. The error is invalid XML version "...". These versions are still accepted and the result reads back through xml_in(): '1.1', '1.2', '2.0', '', ' 1.0', '1.0 ', '1.', 'abc', a version with a single quote, a version with a newline and a 200-digit version. I understand from your message that 0002 does not check the version beyond this, so I only note it. xmlroot() with VERSION '1.0' still returns the document without a declaration, as on master.

For xmlconcat(), xmlagg() and xmlroot() on values with valid declarations (encoding, standalone yes/no, version 1.0 and 1.1, NULL arguments, NO VALUE, nested xmlroot()), the output is the same on master and with the patches in the cases I ran. One observation: a row stored on master with version '1.0" standalone="yes' was not dropped on master, because its declaration still parses, and it is not dropped with the patches either. 0002 now rejects creating it.

make check passes with both patches (243 tests, including xml and xmlmap). With 0001 only, test_setup and xml also pass. I also built without libxml: the xml test passes on master and with both patches.

What I did not test: the isolation and tap suites, pgindent, the docs, libxml2 versions other than 2.9.14, other platforms (I ran one Linux build), the back branches, pg_dump and restore of rows that are already bad, other xml functions that read the declaration (for example xmlserialize and xmltable), and the effect of the new errors on existing applications.

Small suggestions: since queries over already-stored bad rows now fail instead of returning a wrong result, the commit message could say how to find such rows, as you did in your email. I have no objection to the change from what I could test.

Regards,
Joao Rito