Re: Memory Accounting

On Fri, Oct 04, 2019 at 12:36:01AM -0400, Tom Lane wrote:
>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).
>

Yeah, I think that's an oversight. Maybe there's a reason why Jeff used
int64, but I can't think of any.

>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?)
>

Interesting failure mode (especially that it does *not* fail on some
32-bit machines).

>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.
>

I think so too, but I'll take a closer look in the afternoon, unless you
beat me to it.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services