From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
cc: Jeff Davis <pgsql@j-davis.com>,
        Melanie Plageman <melanieplageman@gmail.com>,
        Soumyadeep Chakraborty <sochakraborty@pivotal.io>,
        Robert Haas <robertmhaas@gmail.com>,
        Pg Hackers <pgsql-hackers@postgresql.org>, soumyadeep2007@gmail.com
Subject: Re: Memory Accounting
In-reply-to: <20191001021114.iimd77xgnstaww7j@development>
References: 
 <CAAKRu_aAtcpqFz0fJKAjrvgFUUmYp2mPbeXOPRKWNtJK_Fxa-g@mail.gmail.com>
 <20190724215228.tlyj74ie4l2jn7ji@development>
 <CADwEdor2jZitB2P94on2LUAhdi0iRHv2WNseV3LJmJPenuuNpw@mail.gmail.com>
 <67f66d2fa39e35aef90093c74531a38dcd5c0f73.camel@j-davis.com>
 <CAAKRu_b2Qxr=GzE7thk8_=JDgYRnOssYBuqWdZ39R2+Heo=oQw@mail.gmail.com>
 <20190926192244.mk5ru6nkofo7am3h@development>
 <57780817877d4b40106b248b63261e21a7a56261.camel@j-davis.com>
 <20190928221249.mo2jrtlakingnc5e@development>
 <20190928222206.thidzeygazl7axhg@development>
 <392e034c1b674a1a58c217dc440ea6197081ccfa.camel@j-davis.com>
 <20191001021114.iimd77xgnstaww7j@development>
Comments: In-reply-to Tomas Vondra <tomas.vondra@2ndquadrant.com>
	message dated "Tue, 01 Oct 2019 04:11:14 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <15150.1570163761.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 04 Oct 2019 00:36:01 -0400
Message-ID: <15151.1570163761@sss.pgh.pa.us>
Precedence: bulk

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: C=
REATE INDEX text_idx on test__int using gist ( a gist__int_ops );
TRAP: FailedAssertion("total_allocated =3D=3D context->mem_allocated", Fil=
e: "aset.c", Line: 1533)
2019-10-04 00:42:25.505 CEST [63836:11] LOG:  server process (PID 66916) w=
as terminated by signal 6: Abort trap
2019-10-04 00:42:25.505 CEST [63836:12] DETAIL:  Failed process was runnin=
g: 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 +=3D 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