Обсуждение: Avoid resource leak (src/test/modules/test_binaryheap/test_binaryheap.c)

Поиск
Список
Период
Сортировка

Avoid resource leak (src/test/modules/test_binaryheap/test_binaryheap.c)

От
Ranier Vilela
Дата:
Hi.

Per Coverity.

Coverity reports this resource leak in test_binaryheap module.
I think that is right.

Trivial patch attached.

best regards,
Ranier Vilela
Вложения

Re: Avoid resource leak (src/test/modules/test_binaryheap/test_binaryheap.c)

От
Robert Haas
Дата:
On Fri, Sep 12, 2025 at 1:54 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> Per Coverity.
>
> Coverity reports this resource leak in test_binaryheap module.
> I think that is right.
>
> Trivial patch attached.

If this were correct, we'd need to also free the memory in all the
error paths. But of course, in both error and non-error paths, we rely
on memory context cleanup to free memory for us, except in cases where
there's some specific reason to believe that's not good enough. I
doubt that there is any such reason in this case.

See src/backend/utils/mmgr/README

--
Robert Haas
EDB: http://www.enterprisedb.com



Re: Avoid resource leak (src/test/modules/test_binaryheap/test_binaryheap.c)

От
Tom Lane
Дата:
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Sep 12, 2025 at 1:54 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>> Coverity reports this resource leak in test_binaryheap module.
>> I think that is right.

> If this were correct, we'd need to also free the memory in all the
> error paths. But of course, in both error and non-error paths, we rely
> on memory context cleanup to free memory for us, except in cases where
> there's some specific reason to believe that's not good enough. I
> doubt that there is any such reason in this case.

I agree this isn't interesting from a resource-leak perspective.
However, is it interesting from a test-coverage perspective?
AFAICS, test_binaryheap doesn't presently exercise binaryheap_free,
which seems a little sad for what's supposed to be a unit-test
module.

Of course, binaryheap_free is quite trivial and we do already
have coverage of it elsewhere.  So I'm not super excited about
the omission.

            regards, tom lane