pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Jim Jones <jim.jones@uni-muenster.de>
Cc: Peter Smith <smithpb2250@gmail.com>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Nikolay Samokhvalov <samokhvalov@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Andrey Borodin <amborodin86@gmail.com>
Subject: Re: [PATCH] Add pretty-printed XML output option
Date: Wed, 15 Mar 2023 17:13:36 -0400
Message-ID: <3622266.1678914816@sss.pgh.pa.us> (raw)
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>

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 := '<?xml version="1.0" encoding="utf8"?><foo><bar><val>73</val></bar></foo>';
for i in 1..10000000 loop
  t := xmlserialize(document x as text);
end loop;
raise notice 't = %', 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





view thread (70+ messages)  latest in thread

Message-ID: <3622266.1678914816@sss.pgh.pa.us>
Permalink:  ../3622266.1678914816@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/3622266.1678914816@sss.pgh.pa.us

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, jim.jones@uni-muenster.de, smithpb2250@gmail.com, peter.eisentraut@enterprisedb.com, samokhvalov@gmail.com, pgsql-hackers@lists.postgresql.org, amborodin86@gmail.com
  Subject: Re: [PATCH] Add pretty-printed XML output option
  In-Reply-To: <3622266.1678914816@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox