Silence resource leaks alerts

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

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

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

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

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