On 2025-May-08, Tom Lane wrote:
> Alvaro seems to think CheckNNConstraintFetch is worth taking
> a second look at, and maybe he's right, but the amount of
> storage involved there seems unexciting too.
Yeah, I think the new issue here is that we're calling that function at
all; previously we would only call that function if the table had check
constraints, but now we call it if it has either that or not-null
constraints.
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:
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 68ff67de549..a5ae3d8de5b 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4598,10 +4598,11 @@ CheckNNConstraintFetch(Relation relation)
HeapTuple htup;
int found = 0;
- /* Allocate array with room for as many entries as expected */
- check = (ConstrCheck *)
- MemoryContextAllocZero(CacheMemoryContext,
- ncheck * sizeof(ConstrCheck));
+ /* Allocate array with room for as many entries as expected, if needed */
+ if (ncheck > 0)
+ check = (ConstrCheck *)
+ MemoryContextAllocZero(CacheMemoryContext,
+ ncheck * sizeof(ConstrCheck));
/* Search pg_constraint for relevant entries */
ScanKeyInit(&skey[0],
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.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
