Indeed, createPQExpBuffer is not needed. v2 is much better. Looks good to me.
Original
From: Ranier Vilela <[email protected]>
Date: 2026-03-09 20:54
To: Álvaro Herrera <[email protected]>
Cc: Michael Paquier <[email protected]>, yangyz <[email protected]>,
Chao Li <[email protected]>, Pg Hackers
<[email protected]>
Subject: Re: Avoid resource leak (src/bin/pg_dump/pg_dumpall.c)
Em seg., 9 de mar. de 2026 às 09:40, Álvaro Herrera <[email protected]>
escreveu:
On 2026-Mar-09, Michael Paquier wrote:
> On Mon, Mar 09, 2026 at 11:21:35AM +0800, yangyz wrote:
> > I think it should be modified.
> >
> > Move createPQExpBuffer inside the conditional block to match its
destroy counterpart.
> > This improves code clarity and satisfies static analyzers, even
though the actual memory
> > leak is minimal in practice.
>
> destroyPQExpBuffer() is called for each tuple from pg_database except
> if dealing with "template{0,1}" or "postgres". It means that we
would
> just leak a few bytes for these three cases. I agree that the
> variable declaration can be placed better, but it's really not worth
> bothering in this context.
True, but at the same time it looks as if this routine is wastefully
written -- I mean, why spend time with a stringinfo here at all? We
could write this in much simpler form, as in the attached, which is even
three lines shorter. In fact, before 763aaa06f034, this is exactly how
this routine was written, and I don't see why it was changed this way.
+1
LGTM.
best regards,
Ranier Vilela
--
Álvaro Herrera 48°01'N
7°57'E — https://www.EnterpriseDB.com/
"Just treat us the way you want to be treated + some extra allowance
for ignorance."
(Michael
Brusser)
