Avoid resource leak (src/bin/pg_dump/pg_dumpall.c)

On Mar 9, 2026, at 07:05, Ranier Vilela <ranier.vf@gmail.com> wrote:

Hi.

Per Coverity.

Coverity complains about one resource leak in the function
*drops_DBs*.

CID 1645454: (#1 of 1): Resource leak (RESOURCE_LEAK)
19. leaked_storage: Variable delQry going out of scope leaks the storage it points to.

Fix by avoiding creating the buffer unnecessarily.

Trivial patch attached.

best regards,
Ranier Vilela
<avoid-resource-leak-pg_dumpall.patch>

I confirmed this is a leak, but only leaks 3 tuples, not much memory leaked. Given pg_dump call is not a long-live frontend process, such leak won’t hurt much.

Instead of claiming a memory leak, I would tend to claim a logic mistake. Because createPQExpBuffer is before the “if” clause, but the corresponding destroyPQExpBuffer is placed inside the “if” clause, they are logically mismatch.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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.

If this function is reused in the future or used in long-lived processes, it might become a problem.

Regards,
Yuanzhuo Yang

Original

From: Chao Li <li.evan.chao@gmail.com&gt;
Date: 2026-03-09 08:04
To: Ranier Vilela <ranier.vf@gmail.com&gt;
Cc: Pg Hackers <pgsql-hackers@postgresql.org&gt;
Subject: Re: Avoid resource leak (src/bin/pg_dump/pg_dumpall.c)

&gt;&nbsp;On&nbsp;Mar&nbsp;9,&nbsp;2026,&nbsp;at&nbsp;07:05,&nbsp;Ranier&nbsp;Vilela&nbsp;<ranier.vf@gmail.com&gt;&nbsp;wrote:
&gt;&nbsp;
&gt;&nbsp;Hi.
&gt;&nbsp;
&gt;&nbsp;Per&nbsp;Coverity.
&gt;&nbsp;
&gt;&nbsp;Coverity&nbsp;complains&nbsp;about&nbsp;one&nbsp;resource&nbsp;leak&nbsp;in&nbsp;the&nbsp;function
&gt;&nbsp;*drops_DBs*.
&gt;&nbsp;
&gt;&nbsp;CID&nbsp;1645454:&nbsp;(#1&nbsp;of&nbsp;1):&nbsp;Resource&nbsp;leak&nbsp;(RESOURCE_LEAK)&nbsp;
&gt;&nbsp;19.&nbsp;leaked_storage:&nbsp;Variable&nbsp;delQry&nbsp;going&nbsp;out&nbsp;of&nbsp;scope&nbsp;leaks&nbsp;the&nbsp;storage&nbsp;it&nbsp;points&nbsp;to.
&gt;&nbsp;
&gt;&nbsp;Fix&nbsp;by&nbsp;avoiding&nbsp;creating&nbsp;the&nbsp;buffer&nbsp;unnecessarily.
&gt;&nbsp;
&gt;&nbsp;Trivial&nbsp;patch&nbsp;attached.
&gt;&nbsp;
&gt;&nbsp;best&nbsp;regards,
&gt;&nbsp;Ranier&nbsp;Vilela
&gt;&nbsp;<avoid-resource-leak-pg_dumpall.patch&gt;

I&nbsp;confirmed&nbsp;this&nbsp;is&nbsp;a&nbsp;leak,&nbsp;but&nbsp;only&nbsp;leaks&nbsp;3&nbsp;tuples,&nbsp;not&nbsp;much&nbsp;memory&nbsp;leaked.&nbsp;Given&nbsp;pg_dump&nbsp;call&nbsp;is&nbsp;not&nbsp;a&nbsp;long-live&nbsp;frontend&nbsp;process,&nbsp;such&nbsp;leak&nbsp;won’t&nbsp;hurt&nbsp;much.

Instead&nbsp;of&nbsp;claiming&nbsp;a&nbsp;memory&nbsp;leak,&nbsp;I&nbsp;would&nbsp;tend&nbsp;to&nbsp;claim&nbsp;a&nbsp;logic&nbsp;mistake.&nbsp;Because&nbsp;createPQExpBuffer&nbsp;is&nbsp;before&nbsp;the&nbsp;“if”&nbsp;clause,&nbsp;but&nbsp;the&nbsp;corresponding&nbsp;destroyPQExpBuffer&nbsp;is&nbsp;placed&nbsp;inside&nbsp;the&nbsp;“if”&nbsp;clause,&nbsp;they&nbsp;are&nbsp;logically&nbsp;mismatch.

Best&nbsp;regards,
--
Chao&nbsp;Li&nbsp;(Evan)
HighGo&nbsp;Software&nbsp;Co.,&nbsp;Ltd.
https://www.highgo.com/

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.
--
Michael

On Mar 9, 2026, at 12:17, Michael Paquier <michael@paquier.xyz> 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.
--
Michael

Hi Michael,

From a memory-leak perspective, this is certainly a very minor issue. Still, I think the current placement hurts the code quality a bit, even if the runtime impact is tiny.

If this patch feels too trivial to take separately, perhaps it could be pushed into your bi-monthly stack.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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.

--
Á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)

Em seg., 9 de mar. de 2026 às 09:40, Álvaro Herrera <alvherre@kurilemu.de>
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

On 2026-03-09 Mo 8:40 AM, Álvaro Herrera wrote:

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.

I forget why, but your change looks good. Do you want to apply it?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2026-Mar-09, Andrew Dunstan wrote:

I forget why, but your change looks good. Do you want to apply it?

Sure, will do, thanks for looking.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"No tengo por qué estar de acuerdo con lo que pienso"
(Carlos Caszeli)

Indeed, createPQExpBuffer is not needed. v2 is much better. Looks good to me.

Original

From: Ranier Vilela <ranier.vf@gmail.com&gt;
Date: 2026-03-09 20:54
To: Álvaro Herrera <alvherre@kurilemu.de&gt;
Cc: Michael Paquier <michael@paquier.xyz&gt;, yangyz <1197620467@qq.com&gt;, Chao Li <li.evan.chao@gmail.com&gt;, Pg Hackers <pgsql-hackers@postgresql.org&gt;
Subject: Re: Avoid resource leak (src/bin/pg_dump/pg_dumpall.c)

Em seg., 9 de mar. de 2026 às 09:40, Álvaro Herrera <alvherre@kurilemu.de&gt; escreveu:
On 2026-Mar-09, Michael Paquier wrote:

&gt; On Mon, Mar 09, 2026 at 11:21:35AM +0800, yangyz wrote:
&gt; &gt; I think it should be modified.
&gt; &gt;
&gt; &gt; Move createPQExpBuffer inside the conditional block to match its destroy counterpart.
&gt; &gt; This improves code clarity and satisfies static analyzers, even though the actual memory
&gt; &gt; leak is minimal in practice.
&gt;
&gt; destroyPQExpBuffer() is called for each tuple from pg_database except
&gt; if dealing with "template{0,1}" or "postgres".&nbsp; It means that we would
&gt; just leak a few bytes for these three cases.&nbsp; I agree that the
&gt; variable declaration can be placed better, but it's really not worth
&gt; 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?&nbsp; We
could write this in much simpler form, as in the attached, which is even
three lines shorter.&nbsp; 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&nbsp;

--
Álvaro Herrera&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;48°01'N 7°57'E&nbsp; —&nbsp; https://www.EnterpriseDB.com/
"Just treat us the way you want to be treated + some extra allowance
&nbsp;for ignorance."&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; (Michael Brusser)

On Mon, Mar 09, 2026 at 01:40:51PM +0100, Alvaro Herrera wrote:

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.

Yeah, what you are doing here looks like the sensible thing to do.
--
Michael