Re: Silence resource leaks alerts - Mailing list pgsql-hackers
| From | Ranier Vilela |
|---|---|
| Subject | Re: Silence resource leaks alerts |
| Date | |
| Msg-id | CAEudQArwhxB0CAD+uRd5ZGDYsNsRwteo+i6xecg3gMxrBTF7iw@mail.gmail.com Whole thread Raw |
| In response to | Re: Silence resource leaks alerts (Tom Lane <tgl@sss.pgh.pa.us>) |
| List | pgsql-hackers |
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
--- 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.
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
pgsql-hackers by date:
Previous
From: Álvaro HerreraDate:
Subject: Re: as per commit 643a1a61985bef2590496, move create/open dir code to function using switch case of pg_backup_directory.c file also
Next
From: Mahendra Singh ThalorDate:
Subject: add some more error checks into _fileExistsInDirectory function to report "too long name" error