Postgres Pro
Downloads
Home   >   mailing lists

Thread: Re: Valgrind - showing memory leaks

Re: Valgrind - showing memory leaks

From
Tom Lane
Date:
=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> If the table doesn't have check constraints, we end up doing
> MemoryContextAllocZero() with size 0 in CacheMemoryContext, which isn't
> great (IIUC we innecessarily allocate a chunk of size 8 in this case).
> I think we should make the allocation conditional on nchecks not being
> zero, otherwise I think we're indeed leaking memory permanently in
> CacheMemoryContext, since that allocation is not recorded anywhere:

Uh ... yeah it is, down at the bottom of the function:

    /* Install array only after it's fully valid */
    relation->rd_att->constr->check = check;
    relation->rd_att->constr->num_check = found;

So it seems like valgrind is wrong here, or else we're leaking the
whole rd_att structure later on somehow.

In any case, you're right that asking for a zero-size chunk is
pretty pointless.  I'd support doing

+    if (ncheck > 0)
+        check = (ConstrCheck *)
+            MemoryContextAllocZero(CacheMemoryContext,
+                                   ncheck * sizeof(ConstrCheck));
+    else
+        check = NULL;

but I think we have to make sure it's null if we don't palloc it.

> On the other hand, the bug I was thinking about, is that if the table
> has an invalid not-null constraint, we leak during detoasting in
> extractNotNullColumn().  We purposefully made that function leak that
> memory, because it was only used in DDL code (so the leak didn't
> matter), and to simplify code; commit ff239c3bf4e8.  This uses the
> caller memory context, so it's not a permanent leak and I don't think we
> need any fixes.  But it's no longer so obvious that extractNotNullColumn
> is okay to leak those few bytes.

Given your description it still sounds fine to me.

            regards, tom lane

Re: Valgrind - showing memory leaks

From
Álvaro Herrera
Date:
On 2025-May-08, Tom Lane wrote:

> Uh ... yeah it is, down at the bottom of the function:
> 
>     /* Install array only after it's fully valid */
>     relation->rd_att->constr->check = check;
>     relation->rd_att->constr->num_check = found;
> 
> So it seems like valgrind is wrong here, or else we're leaking the
> whole rd_att structure later on somehow.

Well, the problem is that if num_check is zero, FreeTupleDesc() doesn't
free ->check.

> In any case, you're right that asking for a zero-size chunk is pretty
> pointless.  I'd support doing
> 
> +    if (ncheck > 0)
> +        check = (ConstrCheck *)
> +            MemoryContextAllocZero(CacheMemoryContext,
> +                                   ncheck * sizeof(ConstrCheck));
> +    else
> +        check = NULL;
> 
> but I think we have to make sure it's null if we don't palloc it.

Done that way, thanks.

> > On the other hand, the bug I was thinking about, is that if the table
> > has an invalid not-null constraint, we leak during detoasting in
> > extractNotNullColumn().  [...] But it's no longer so obvious that
> > extractNotNullColumn is okay to leak those few bytes.
> 
> Given your description it still sounds fine to me.

Cool, I left it alone.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)

Re: Valgrind - showing memory leaks

From
Tom Lane
Date:
=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> On 2025-May-08, Tom Lane wrote:
>> So it seems like valgrind is wrong here, or else we're leaking the
>> whole rd_att structure later on somehow.

> Well, the problem is that if num_check is zero, FreeTupleDesc() doesn't
> free ->check.

Ah-hah.

> Done that way, thanks.

Thanks! I've been putting together a list of fixes to make
Valgrind less noisy, and it just got one shorter.

            regards, tom lane