pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Jeff Davis <pgsql@j-davis.com>
Cc: Melanie Plageman <melanieplageman@gmail.com>
Cc: Soumyadeep Chakraborty <sochakraborty@pivotal.io>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Cc: soumyadeep2007@gmail.com
Subject: Re: Memory Accounting
Date: Fri, 04 Oct 2019 00:36:01 -0400
Message-ID: <15151.1570163761@sss.pgh.pa.us> (raw)
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>

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





view thread (28+ messages)  latest in thread

Message-ID: <15151.1570163761@sss.pgh.pa.us>
Permalink:  ../15151.1570163761@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/15151.1570163761@sss.pgh.pa.us

 · 

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, tomas.vondra@2ndquadrant.com, pgsql@j-davis.com, melanieplageman@gmail.com, sochakraborty@pivotal.io, robertmhaas@gmail.com, soumyadeep2007@gmail.com
  Subject: Re: Memory Accounting
  In-Reply-To: <15151.1570163761@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox