Обсуждение: 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
frontendprocess, 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/

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

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)

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)

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