Date: Fri, 4 Oct 2019 10:26:44 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
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
Message-ID: <20191004082644.wytixiukkm6ditlu@development>
References: 
 <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>
 <15151.1570163761@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <15151.1570163761@sss.pgh.pa.us>
User-Agent: NeoMutt/20180716
Precedence: bulk

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