From: Tom Lane <tgl@sss.pgh.pa.us>
To: Jim Jones <jim.jones@uni-muenster.de>
cc: Peter Smith <smithpb2250@gmail.com>,
        Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
        Nikolay Samokhvalov <samokhvalov@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
        Andrey Borodin <amborodin86@gmail.com>
Subject: Re: [PATCH] Add pretty-printed XML output option
In-reply-to: <3057762.1678836321@sss.pgh.pa.us>
References: <2f5df461-dad8-6d7d-4568-08e10608a69b@uni-muenster.de>
 <d79950fd-b3f5-783c-919a-a95de23c0c2c@enterprisedb.com>
 <4c2efca5-8257-2424-6687-07c65e319e95@uni-muenster.de>
 <CAHut+PtMHeBBZ2zv0Bk7-4KMkSrrxZ9PZxUCzhpwTGVk+8G3cw@mail.gmail.com>
 <2eac4d78-2b20-e8ec-54cc-22a88653a1cc@uni-muenster.de>
 <CAHut+PsgXxbzzsG+VqHUa4Kz3G4B-anNmF3-GENwuUdfMJtowA@mail.gmail.com>
 <fb2a543b-e615-d9ba-8402-909631f9b212@uni-muenster.de>
 <CAHut+Pvryt_+wA7TwKVaaF0e6Pm5LaPZQLKatp=78oFd8vMh3A@mail.gmail.com>
 <6cb296fe-4b49-3691-633e-2e2366997b04@uni-muenster.de>
 <3baf8f25-3bd7-7db5-0b31-e4c92b6269ba@enterprisedb.com>
 <1cbe1087-6acb-2b0a-dc9b-0a2d5b99e466@uni-muenster.de>
 <CAHut+PvBUTf9p+Y6+mpWpz59=3eL_EKQ+7Ysxpa9hYGdncDMdg@mail.gmail.com>
 <532259.1678393296@sss.pgh.pa.us> <906cfd1d-63a0-3cf5-7291-2dadb2e!
 149ff@uni-muenster.de> <803304.1678458776@sss.pgh.pa.us> <efe0b19b-!
 d41c-f8ab-f3b8-afb0108f3706@uni-muenster.de>
 <2752578.1678815625@sss.pgh.pa.us>
 <abd25443-ef6d-7b8a-c593-a2a991d3e5ce@uni-!
 ! muenster.de> <3057762.1678836321@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 14 Mar 2023 19:25:21 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3622265.1678914816.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 15 Mar 2023 17:13:36 -0400
Message-ID: <3622266.1678914816@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3622266.1678914816%40sss.pgh.pa.us>
Precedence: bulk

I wrote:
> Huh, interesting.  That is a legitimate pretty-fication of the input,
> I suppose, but some people might think it goes beyond the charter of
> "indentation".  I'm okay with it personally; anyone want to object?

Hearing no objections to that, I moved ahead with this.

It occurred to me to test v23 for memory leaks, and it had bad ones:

* the "newline" node used in the CONTENT case never got freed.
Making another one for each line wasn't helping, either.

* libxml, at least in the 2.9.7 version I have here, turns out to
leak memory if you pass a non-null encoding to xmlSaveToBuffer.
But AFAICS we don't really need to do that, because the last thing
we want is for libxml to try to do any encoding conversion.

After cleaning that up, I saw that we were indeed doing essentially
duplicative xml_parse calls for the DOCUMENT check and the indentation
work, so I refactored to allow just one call to serve.

Pushed with those changes and some other cosmetic cleanup.
Thanks for working so hard on this!

(Now to keep an eye on the buildfarm, to see if other versions of
libxml work like mine ...)

BTW, the libxml leak problem seems to extend to other cases too.
I tested with code like

do $$
declare x xml; t text;
begin
x :=3D '<?xml version=3D"1.0" encoding=3D"utf8"?><foo><bar><val>73</val></=
bar></foo>';
for i in 1..10000000 loop
  t :=3D xmlserialize(document x as text);
end loop;
raise notice 't =3D %', t;
end;
$$;

That case is fine, but if you change the encoding spec to "latin1",
it leaks like mad.  That problem is not the fault of this patch,
I don't think.  I wonder if we need to do something to prevent
libxml from seeing encoding declarations other than utf8?

			regards, tom lane