Re: Valgrind - showing memory leaks

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre(at)kurilemu(dot)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