Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_dumpall: simplify coding of dropDBs()

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

    Ranier Vilela <ranier.vf@gmail.com> — 2026-03-08T23:05:06Z

    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

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

    Chao Li <li.evan.chao@gmail.com> — 2026-03-09T00:04:54Z

    
> 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/

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

    yangyz <1197620467@qq.com> — 2026-03-09T03:21:35Z

    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/

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

    Michael Paquier <michael@paquier.xyz> — 2026-03-09T04:17:13Z

    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

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

    Chao Li <li.evan.chao@gmail.com> — 2026-03-09T05:28:32Z

    
> 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/

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-09T12:40:51Z

    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)

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

    Ranier Vilela <ranier.vf@gmail.com> — 2026-03-09T12:54:20Z

    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

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

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

    Andrew Dunstan <andrew@dunslane.net> — 2026-03-09T16:41:03Z

    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

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

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-09T16:51:48Z

    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)

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

    yangyz <1197620467@qq.com> — 2026-03-10T00:27:06Z

    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)

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

    Michael Paquier <michael@paquier.xyz> — 2026-03-10T03:05:40Z

    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