Indeed, createPQExpBuffer is not needed. v2 is much better. Looks good to me.
From: Ranier Vilela <ranier.vf@gmail.com> Date: 2026-03-09 20:54 To: Álvaro Herrera <alvherre@kurilemu.de> Cc: Michael Paquier <michael@paquier.xyz>, yangyz <1197620467@qq.com>, Chao Li <li.evan.chao@gmail.com>, Pg Hackers <pgsql-hackers@postgresql.org> Subject: Re: Avoid resource leak (src/bin/pg_dump/pg_dumpall.c) |
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)
