Re: Silence resource leaks alerts

Em sex., 11 de abr. de 2025 às 02:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> Michael Paquier <michael(at)paquier(dot)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