So ... why exactly did this patch define MemoryContextData.mem_allocated
as int64? That seems to me to be doubly wrong: it is not the right width
on 32-bit machines, and it is not the right signedness anywhere. I think
that field ought to be of type Size (a/k/a size_t, but memnodes.h always
calls it Size).
I let this pass when the patch went in, but now I'm on the warpath
about it, because since c477f3e449 went in, some of the 32-bit buildfarm
members are failing with
2019-10-04 00:41:56.569 CEST [66916:86] pg_regress/_int LOG: statement: CREATE
INDEX text_idx on test__int using gist ( a gist__int_ops );
TRAP: FailedAssertion("total_allocated == context->mem_allocated", File:
"aset.c", Line: 1533)
2019-10-04 00:42:25.505 CEST [63836:11] LOG: server process (PID 66916) was
terminated by signal 6: Abort trap
2019-10-04 00:42:25.505 CEST [63836:12] DETAIL: Failed process was running:
CREATE INDEX text_idx on test__int using gist ( a gist__int_ops );
What I think is happening is that c477f3e449 allowed this bit in
AllocSetRealloc:
context->mem_allocated += blksize - oldblksize;
to be executed in situations where blksize < oldblksize, where before
that was not possible. Of course blksize and oldblksize are of type
Size, hence unsigned, so the subtraction result underflows in this
case. If mem_allocated is of the same width as Size then this does
not matter because the final result wraps around to the proper value,
but if we're going to allow mem_allocated to be wider than Size then
we will need more logic here to add or subtract as appropriate.
(I'm not quite sure why we're not seeing this failure on *all* the
32-bit machines; maybe there's some other factor involved?)
I see no value in defining mem_allocated to be wider than Size.
Yes, the C standard contemplates the possibility that the total
available address space is larger than the largest chunk you can
ever ask malloc for; but nobody has built a platform like that in
this century, and they sucked to program on back in the dark ages
when they did exist. (I speak from experience.) I do not think
we need to design Postgres to allow for that.
Likewise, there's no evident value in allowing mem_allocated
to be negative.
I haven't chased down exactly what else would need to change.
It might be that s/int64/Size/g throughout the patch is
sufficient, but I haven't analyzed it.
regards, tom lane
