Postgres Pro
Downloads
Home   >   mailing lists

Thread: Silence resource leaks alerts

Silence resource leaks alerts

From
Ranier Vilela
Date:
Hi.

Per Coverity.

Coverity has some alerts about resource leaks.
I think that is good silence.

While it is arguable that this is a false warning, there is a benefit in moving the initialization of the string buffer, silencing the warnings that are presented in this case.

1. pg_overexplain.c
2. ruleutils.c

best regards,
Ranier Vilela
Attachment

Re: Silence resource leaks alerts

From
Michael Paquier
Date:
On Thu, Apr 10, 2025 at 03:10:02PM -0300, Ranier Vilela wrote:
> While it is arguable that this is a false warning, there is a benefit in
> moving the initialization of the string buffer, silencing the warnings that
> are presented in this case.
>
> 1. pg_overexplain.c
> 2. ruleutils.c

These code paths are far from being critical and the two ones in
ruleutils.c are older, even if it is a practice that had better be
discouraged particularly as initStringInfo() can allocate some memory
for nothing.  So it could bloat the current memory context if these
code paths are repeatedly taken.

FWIW, I'm with these changes to delay these initializations as you are
proposing.  The RMT has a say about such changes post feature-freeze,
though, even if the one in pg_overexplain.c is new to v18.
--
Michael
Attachment

Re: Silence resource leaks alerts

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Apr 10, 2025 at 03:10:02PM -0300, Ranier Vilela wrote:
>> While it is arguable that this is a false warning, there is a benefit in
>> moving the initialization of the string buffer, silencing the warnings that
>> are presented in this case.
>> 
>> 1. pg_overexplain.c
>> 2. ruleutils.c

> These code paths are far from being critical and the two ones in
> ruleutils.c are older, even if it is a practice that had better be
> discouraged particularly as initStringInfo() can allocate some memory
> for nothing.  So it could bloat the current memory context if these
> code paths are repeatedly taken.

I don't believe either module ever gets run in a long-lived memory
context.  So I think the burden of proof to show that these leaks
are meaningful is on the proposer.

I'm totally not on board with the approach of "if Coverity says this
is a leak then we must fix it".  By and large, it's more efficient
for us to leak small allocations and recover them at context reset or
delete than it is to pfree those allocations retail.  Sure, if we're
talking about big allocations, or if there will be a lot of such
allocations during the lifetime of the context, we'd better do the
retail pfrees.  Sadly, such criteria are outside Coverity's ken.

            regards, tom lane

Re: Silence resource leaks alerts

From
Ranier Vilela
Date:
Thanks Michael, for looking at this.


Em sex., 11 de abr. de 2025 às 02:09, Michael Paquier <michael@paquier.xyz> escreveu:
On Thu, Apr 10, 2025 at 03:10:02PM -0300, Ranier Vilela wrote:
> While it is arguable that this is a false warning, there is a benefit in
> moving the initialization of the string buffer, silencing the warnings that
> are presented in this case.
>
> 1. pg_overexplain.c
> 2. ruleutils.c

These code paths are far from being critical and the two ones in
ruleutils.c are older, even if it is a practice that had better be
discouraged particularly as initStringInfo() can allocate some memory
for nothing.  So it could bloat the current memory context if these
code paths are repeatedly taken.
Yeah, it's a bit annoying to do unnecessary work.
Plus a small gain, by delaying memory allocation until when it is actually needed.


FWIW, I'm with these changes to delay these initializations as you are
proposing.
Thanks.
 
  The RMT has a say about such changes post feature-freeze,
though, even if the one in pg_overexplain.c is new to v18.
I agree.

best regards,
Ranier Vilela

Re: Silence resource leaks alerts

From
Ranier Vilela
Date:
Em sex., 11 de abr. de 2025 às 02:37, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Apr 10, 2025 at 03:10:02PM -0300, Ranier Vilela wrote:
>> While it is arguable that this is a false warning, there is a benefit in
>> moving the initialization of the string buffer, silencing the warnings that
>> are presented in this case.
>>
>> 1. pg_overexplain.c
>> 2. ruleutils.c

> These code paths are far from being critical and the two ones in
> ruleutils.c are older, even if it is a practice that had better be
> discouraged particularly as initStringInfo() can allocate some memory
> for nothing.  So it could bloat the current memory context if these
> code paths are repeatedly taken.

I don't believe either module ever gets run in a long-lived memory
context.  So I think the burden of proof to show that these leaks
are meaningful is on the proposer.
Despite the $subject talking about leaks, the issue in this specific case is doing unnecessary work.
Get the silencing of these alerts for free.


I'm totally not on board with the approach of "if Coverity says this
is a leak then we must fix it". 
I partially agree.
In some cases, it can actually be disastrous.

For example: src/backend/tcop/fastpath.c (line 456)
CID 1608897: (#1 of 1): Resource leak (RESOURCE_LEAK)
10. leaked_storage: Variable abuf going out of scope leaks the storage abuf.data points to

"Fixing" this leak, causes several errors:

diff --strip-trailing-cr -U3 C:/dll/postgres_dev/postgres_commit/src/test/regress/expected/largeobject_1.out C:/dll/postgres_dev/postgres_commit/build/testrun/regress/regress/results/largeobject.out
--- C:/dll/postgres_dev/postgres_commit/src/test/regress/expected/largeobject_1.out 2024-10-24 16:40:59.364703100 -0300
+++ C:/dll/postgres_dev/postgres_commit/build/testrun/regress/regress/results/largeobject.out 2025-04-11 08:55:10.085484800 -0300
@@ -37,11 +37,13 @@
 (1 row)
 
 \lo_unlink 42
+ERROR:  pfree called with invalid pointer 000000C6593FF110 (header 0x000001d03fc52f08)
 \dl
-      Large objects
- ID | Owner | Description
-----+-------+-------------
-(0 rows)
+               Large objects
+ ID |      Owner      |     Description    
+----+-----------------+---------------------
+ 42 | regress_lo_user | the ultimate answer
+(1 row)
 
 -- Load a file
 CREATE TABLE lotest_stash_values (loid oid, fd integer);
@@ -417,19 +419,354 @@
 (1 row)
 
 \lo_import :filename
+ERROR:  pfree called with invalid pointer 000000C6593FF110 (header 0x000001d03fc529e8)
 \set newloid :LASTOID
 -- just make sure \lo_export does not barf
 \set filename :abs_builddir '/results/lotest2.txt'
 \lo_export :newloid :filename
+ERROR:  pfree called with invalid pointer 000000C6593FF110 (header 0x000001d03fc529e8)
 -- This is a hack to test that export/import are reversible
 -- This uses knowledge about the inner workings of large object mechanism
 -- which should not be used outside it.  This makes it a HACK
 SELECT pageno, data FROM pg_largeobject WHERE loid = (SELECT loid from lotest_stash_values)
 EXCEPT
 SELECT pageno, data FROM pg_largeobject WHERE loid = :newloid;
- pageno | data
---------+------
-(0 rows)
+ pageno
<cut>


By and large, it's more efficient
for us to leak small allocations and recover them at context reset or
delete than it is to pfree those allocations retail.
As long as you don't allocate memory in advance and unnecessarily.
Even for small amounts, it is an expensive operation.
And compilers are not able to remove it themselves.

  Sure, if we're
talking about big allocations, or if there will be a lot of such
allocations during the lifetime of the context, we'd better do the
retail pfrees.  Sadly, such criteria are outside Coverity's ken.
Coverity has gotten better and better,
but understanding the complexity in Postgres, for memory usage,
is still far away.

best regards,
Ranier Vilela

Re: Silence resource leaks alerts

From
Ranier Vilela
Date:
Em sex., 11 de abr. de 2025 às 08:27, Ranier Vilela <ranier.vf@gmail.com> escreveu:
Thanks Michael, for looking at this.


Em sex., 11 de abr. de 2025 às 02:09, Michael Paquier <michael@paquier.xyz> escreveu:
On Thu, Apr 10, 2025 at 03:10:02PM -0300, Ranier Vilela wrote:
> While it is arguable that this is a false warning, there is a benefit in
> moving the initialization of the string buffer, silencing the warnings that
> are presented in this case.
>
> 1. pg_overexplain.c
> 2. ruleutils.c

These code paths are far from being critical and the two ones in
ruleutils.c are older, even if it is a practice that had better be
discouraged particularly as initStringInfo() can allocate some memory
for nothing.  So it could bloat the current memory context if these
code paths are repeatedly taken.
Yeah, it's a bit annoying to do unnecessary work.
Plus a small gain, by delaying memory allocation until when it is actually needed.
Attached a new example of moving stringinfo creation, per Coverity.

best regards,
Ranier Vilela
Attachment