Re: Deprecation warnings on Rocky 10.2 with current dev branch

Hi Tom,

Thanks for updating the XML content parser. I compared v3 with master
at 412848dac37 and found two changes in XMLSERIALIZE ... INDENT output
with older libxml2 versions.

I tested with libxml2 2.9.7 from my system, an upstream build of 2.12.5
to match the version reported in this thread, and the latest upstream
release, 2.15.4.

1. With libxml2 2.9.7, v3 drops U+FEFF after the XML declaration.
The attached 0001 fixes this and adds a regression test.

In a UTF8 database:

SELECT xmlserialize(CONTENT xmlparse(CONTENT
'<?xml version="1.0"?>' || chr(65279) || 'abc') AS text INDENT);

master: &#xFEFF;abc
v3: abc
v3 + 0001: &#xFEFF;abc

chr(65279) is U+FEFF, the character used as a byte-order mark (BOM).
Here it follows the XML declaration and is part of the content.
xml_parse() skips the declaration
<?xml version="1.0"?> before calling xmlParseInNodeContext(), leaving
U+FEFF at the start of its input.
With doc->encoding set to UTF-8, libxml2 2.9.7 calls
xmlSwitchToEncoding() and removes the character as a BOM.

0001 moves the doc->encoding assignment after xmlParseInNodeContext().
The input is already UTF-8, so the parser can read it with doc->encoding
unset.

To check whether setting doc->encoding to UTF-8 later could cause
other problems, I read the libxml2 source. I checked all functions
called between the old and new assignment locations in versions
2.9.7, 2.12.5 and 2.15.4.

xmlDocSetRootElement(), xmlAddChildList() and their helpers do not use
doc->encoding. xmlParseInNodeContext() reads it to set the input
encoding in all three versions. xml_parse() has already converted the
input to UTF-8, which this parser also uses with doc->encoding unset.
Only 2.9.7 strips U+FEFF during explicit encoding setup.
0001 sets doc->encoding to UTF-8 after xmlParseInNodeContext(),
before xml_parse() returns. The XML output still uses the same encoding.
I found no other side effects in these functions.

Master and v3 already retain U+FEFF with libxml2 2.12.5 and 2.15.4.
With libxml2 2.9.7, the new test fails on v3 and passes on v3 + 0001.
It also passes on v3 + 0001 with 2.12.5 and 2.15.4.

2. With libxml2 2.9.7 and 2.12.5, v3 removes the space between <a/>
and <b/> in this mixed-content input:

SELECT replace(xmlserialize(CONTENT 'abc<a/> <b/>' AS text INDENT),
E'\n', E'\\n');

master: abc\n<a/> \n<b/>
v3: abc\n<a/>\n<b/>
v3 + 0001 + 0002: abc\n<a/> \n<b/>

I traced this to areBlanks() in libxml2 2.12.5. One of the checks in
its source keeps whitespace when the parent's first child is a text
node. With the old API, that child is the text node "abc", so the
space between <a/> and <b/> is kept. xmlParseInNodeContext() inserts
a temporary comment before "abc". The first child is then a comment,
so that check no longer preserves the space, and the parser removes it.

With libxml2 2.15.4, both master and v3 retain the space, but PG users
can still use old versions.

0002 fixes this in xml_parse() by adding an empty text child to the
dummy root before xmlParseInNodeContext(), then removing it after
parsing. This gives areBlanks() a text node as the first child again.
It does this when whitespace removal is enabled and the content starts
with nonblank text or a character reference. Leading literal whitespace
is left alone.

0001 applies on top of v3, and 0002 applies on top of 0001. With both
patches, all 239 core regression tests passed on each of the three
libxml2 versions.

3. I also tried keeping xmlParseBalancedChunkMemory() and passing an
xmlSAXHandler to control whitespace instead of calling
xmlKeepBlanksDefault(). In xml_parse(), the key change was:

xmlSAXHandler sax;

memset(&sax, 0, sizeof(sax));
xmlSAXVersion(&sax, 2);
if (!preserve_whitespace)
sax.ignorableWhitespace = xmlSAX2IgnorableWhitespace;

res_code = xmlParseBalancedChunkMemory(doc, &sax, NULL, 0,
utf8string + count,
parsed_nodes);

This is a small change. Both examples above and all 239 core
regression tests passed with all three libxml2 versions.
However, on 2.9.7 and 2.12.5, forcing the parser's internal xmlNewDoc()
to fail aborted the PostgreSQL backend. In libxml2 2.12.5,
xmlParseBalancedChunkMemory() calls
xmlParseBalancedChunkMemoryRecover(), which has these two error paths:

/* This return does not free the caller's sax. */
ctxt = xmlCreateDocParserCtxt(string);
if (ctxt == NULL) return(-1);

/* This later failure frees it through xmlFreeParserCtxt(). */
oldsax = ctxt->sax;
ctxt->sax = sax;
newDoc = xmlNewDoc(BAD_CAST "1.0");
if (newDoc == NULL) {
xmlFreeParserCtxt(ctxt);
return(-1);
}

xmlFreeParserCtxt() frees our xmlSAXHandler with:

xmlFree(ctxt->sax);

This is an invalid free for a stack handler. Normal cleanup restores
oldsax first, but the xmlNewDoc() failure path does not.

Allocating sax on the heap does not solve this: both errors return -1,
but only the second frees sax. PG cannot tell which error occurred.
Calling xmlFree(sax) on every error would double-free it after the
xmlNewDoc() failure. Skipping cleanup would leak it after the
xmlCreateDocParserCtxt() failure.

Since libxml2 2.13.0, the parser makes its own copy of the caller's
xmlSAXHandler, avoiding this error path [1]. But PostgreSQL still
supports the affected older libraries, so I dropped the SAX alternative.

The warning comes from the deprecated xmlKeepBlanksDefault(). Its
documentation recommends parser options with XML_PARSE_NOBLANKS [2].
xmlParseBalancedChunkMemory() has no options parameter, while
xmlParseInNodeContext() does, as Andrew pointed out. Your change thus
removes the deprecated calls while still controlling whitespace.

The docs also suggest xmlCtxtParseContent() as an alternative to
xmlParseInNodeContext(), but it is only available since 2.14. Given
the older versions PostgreSQL supports and the results above, I think
your approach is the best fit for this change, with these two fixes.

[1] https://gitlab.gnome.org/GNOME/libxml2/-/commit/ecfbcc8a
[2] https://gnome.pages.gitlab.gnome.org/libxml2/html/parser_8h.html

Regards,
Rui