Re: Deprecation warnings on Rocky 10.2 with current dev branch

Rui Zhao <zhaorui126(at)gmail(dot)com> writes:
> 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.

Thanks for looking at it!

> 1. With libxml2 2.9.7, v3 drops U+FEFF after the XML declaration.

Wow, how'd you find that? I do replicate this result on the 2.9.13
that Apple is currently shipping, so I agree it might be worth
worrying about. (But then again, it seems very likely nobody would
ever notice.)

> The attached 0001 fixes this and adds a regression test.

Hmm, 0001 seems quite odd to me. Why bother to install a value for
doc->encoding at all, if we're not going to do so until after all
the calls that will pay attention to it? I tried simply commenting
out the assignment:

Assert(doc->encoding == NULL);
+ #ifdef NOT_USED
doc->encoding = xmlStrdup((const xmlChar *) "UTF-8");
if (doc->encoding == NULL || xmlerrcxt->err_occurred)
xml_ereport(xmlerrcxt, ERROR, ERRCODE_OUT_OF_MEMORY,
"could not allocate XML document");
+ #endif
doc->standalone = standalone;

and that seemed to work fine.

> 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');

Hm. While that's not great, it sure seems like an edge case.
There is no particularly well-defined behavior for whitespace in
INDENT mode anyway, is there? How would you tell which whitespace
was in the original input and which was INDENT's responsibility?
So I'm kind of inclined to let it go. If the proposed fix weren't
so ugly I might feel differently, but it's messy, overly familiar
with libxml2 innards, and perhaps could introduce odd cases of its
own in libxml2 versions other than the ones you tested. Plus the
comments don't actually agree with what the code is doing AFAICT.
(Why do we not need to take action if the input has leading
whitespace? Wouldn't the injected comment mess it up anyway?)

On the whole, if this is the biggest behavioral change we can find
from switching to xmlParseInNodeContext, I think we're in pretty
good shape. I had feared that there might be much worse. My current
thought is to apply this patch to HEAD only, so if there are any
small behavioral changes that users notice, we can write them off
as expected in a major version update.

> 3. I also tried keeping xmlParseBalancedChunkMemory() and passing an
> xmlSAXHandler to control whitespace instead of calling
> xmlKeepBlanksDefault().

I agree that that isn't a good way to go. While libxml2 has only
officially deprecated SAX1, I wonder how much they like SAX2 either.
Even if that worry is baseless, it seems like making use of SAX would
greatly expand the amount of libxml2 API we're depending on --- and
their API behavior has not been so stable that I want to have more
surface area for problems.

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

Yeah, that suggestion isn't real helpful for our purposes.

So my current thought is to cut down 0001 so it just removes code
without adding any, and reject 0002 as not worth trying to fix.
OK by you?

Thanks again for reviewing!

regards, tom lane