Re: [PATCH] Add pretty-printed XML output option

Hi Tom

On 21.05.25 22:20, Tom Lane wrote:
> Just when you thought it was safe to go back in the water ...
>
> Experimenting with the improved valgrind leak detection code at [1],
> I discovered that XMLSERIALIZE(... INDENT) has yet a different memory
> leak problem.  It turns out that xmlDocSetRootElement() doesn't
> merely install the given root node: it unlinks the document's old
> root node and returns it to you.  If you don't free it, it's leaked
> (for the session, since this is a malloc not palloc).

Yeah, I just read the same in the docs

/"returns the unlinked old root element or NULL if the document didn't
have a root element or a memory allocation failed. "/

The xmlsoft examples are a bit misleading though [1]

    /* 
     * Creates a new document, a node and set it as a root node
     */
    doc = xmlNewDoc(BAD_CAST "1.0");
    root_node = xmlNewNode(NULL, BAD_CAST "root");
    xmlDocSetRootElement(doc, root_node);

and [2]

    /* Make ELEMENT the root node of the tree */
    xmlDocSetRootElement(doc, node);

It seems that xml_parse has the same issue[3]
Should we attempt to free the result of xmlDocSetRootElement() there too? v2 attached.

>   The amount of
> leakage isn't that large, seems to be a couple hundred bytes per
> iteration, which may explain why this escaped our notice in the
> previous testing.  Still, it could add up under extensive usage.
> So I think we need to apply the attached, back to PG 16.

Definitely. It could add up quickly under heavy usage.

Thanks for fixing it!

Best, Jim


1 - http://xmlsoft.org/examples/tree2.c
2 - http://xmlsoft.org/examples/testWriter.c
3 -
https://github.com/postgres/postgres/blob/f3622b64762bb5ee5242937f0fadcacb1a10f30e/src/backend/utils/adt/xml.c#L1872