Subject: pgsql: Introduce dynamic shared memory areas.

From: Robert Haas <[email-masked]>
Date: Fri, 02 Dec 2016 17:36:07 +0000

Introduce dynamic shared memory areas.

Programmers discovered decades ago that it was useful to have a simple
interface for allocating and freeing memory, which is why malloc() and
free() were invented.  Unfortunately, those handy tools don't work
with dynamic shared memory segments because those are specific to
PostgreSQL and are not necessarily mapped at the same address in every
cooperating process.  So invent our own allocator instead.  This makes
it possible for processes cooperating as part of parallel query
execution to allocate and free chunks of memory without having to
reserve them prior to the start of execution.  It could also be used
for longer lived objects; for example, we could consider storing data
for pg_stat_statements or the stats collector in shared memory using
these interfaces, rather than writing them to files.  Basically,
anything that needs shared memory but can't predict in advance how
much it's going to need might find this useful.

Thomas Munro and Robert Haas.  The original code (of mine) on which
Thomas based his work was actually designed to be a new backend-local
memory allocator for PostgreSQL, but that hasn't gone anywhere - or
not yet, anyway.  Thomas took that work and performed major
refactoring and extensive modifications to make it work with dynamic
shared memory, including the addition of appropriate locking.

Discussion: [email-masked]
Discussion: CAEepm=[email-masked]

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/13df76a537cca3b8884911d8fdf7c89a457a8dd3

Modified Files
--------------
src/backend/utils/mmgr/Makefile  |    2 +-
src/backend/utils/mmgr/dsa.c     | 2202 ++++++++++++++++++++++++++++++++++++++
src/include/utils/dsa.h          |  108 ++
src/tools/pgindent/typedefs.list |    6 +
4 files changed, 2317 insertions(+), 1 deletion(-)

-- 
Sent via pgsql-committers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

---

From: Michael Paquier <[email-masked]>
Date: Mon, 5 Dec 2016 04:18:55 -0800

> shared memory, including the addition of appropriate locking.

This commit is generating a warning when compiling on my Win7 dev box:
"C:\Users\ioltas\git\postgres\pgsql.sln" (default target) (1) ->
"C:\Users\ioltas\git\postgres\postgres.vcxproj" (default target) (2) ->
(ClCompile target) ->
  src/backend/utils/mmgr/dsa.c(1921): warning C4334: '<<' : result of 32-bit sh
ift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\Users\iolt
as\git\postgres\postgres.vcxproj]

    1 Warning(s)
    0 Error(s)
-- 
Michael

-- 
Sent via pgsql-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

---

From: Tom Lane <[email-masked]>
Date: Mon, 05 Dec 2016 10:09:05 -0500

> This commit is generating a warning when compiling on my Win7 dev box:

dromedary has this:

ccache gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -O2 -ansi -I../../../../src/include -DCOPY_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST   -c -o network_selfuncs.o network_selfuncs.c
dsa.c: In function 'dsa_dump':
dsa.c:1106: warning: format '%016lx' expects type 'long unsigned int', but argument 3 has type 'dsa_pointer'
dsa.c:1106: warning: format '%016lx' expects type 'long unsigned int', but argument 4 has type 'dsa_pointer'
dsa.c: In function 'make_new_segment':
dsa.c:2039: warning: left shift count >= width of type
dsa.c:2039: warning: left shift count >= width of type
dsa.c:2077: warning: left shift count >= width of type

The first two of those should be fixed by 670b3bc8f, but the shift
problems remain.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 5 Dec 2016 10:09:28 -0500

> as\git\postgres\postgres.vcxproj]

Hmm, I'm not sure I understand that warning.  I think the complaint is
about this line of code:

        Size        threshold = 1 << (bin - 1);

"bin" is declared as "Size", and threshold is also declared as "Size",
so what's the problem?  Is it unhappy that the "1" being shifted isn't
declared as 1L?  Is this a 32-bit system or a 64-bit system?  Or maybe
(Size) 1 << (bin - 1) would be safer?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Tom Lane <[email-masked]>
Date: Mon, 05 Dec 2016 10:17:04 -0500

> so what's the problem?

The shift operator does not coerce its operands to be the same size.
It just shifts the left operand in its native width, which here is
"int", which ain't enough.

> (Size) 1 << (bin - 1) would be safer?

Yes.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 5 Dec 2016 10:21:18 -0500

> The first two of those should be fixed by 670b3bc8f, but the shift

Thanks.  I think I see what's happening here.  DSA_MAX_SEGMENT_SIZE is
defined as ((size_t) 1 << DSA_OFFSET_WIDTH).  I'm not sure why that's
not (Size), but the issue is that any system with 64-bit atomics
support ends up selecting the 64-bit version of DSA even if Size is
32-bit.  So DSA_OFFSET_WIDTH ends up as 40, and then the wheels come
off.  I think I'll go adjust things so that we always pick the 32-bit
version of DSA if Size is 32-bits.  There's some theoretical loss
there since we are then limited to 32 DSA segments per DSA area and
hypothetically you could want more than that, but I don't think that's
much of a problem in practice because you probably would run out of
address space before you hit 32 segments anyway.

---

From: Tom Lane <[email-masked]>
Date: Mon, 05 Dec 2016 11:37:17 -0500

>> Introduce dynamic shared memory areas.

gaur finds another problem with dsa.c: it uses SIZE_MAX which AFAICS
is not required to exist by POSIX.

We could adopt the timezone code's workaround

#ifndef SIZE_MAX
#define SIZE_MAX ((size_t) -1)
#endif

but I don't find that particularly nice, and in any case I wonder
why we would want to use SIZE_MAX and not MaxAllocHugeSize.  The
overflow hazards that motivate that not being the max possible
value seem relevant here too.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 5 Dec 2016 12:18:35 -0500

> value seem relevant here too.

It's not quite the same thing, because control->max_total_segment_size
is a total of the memory used by all allocations plus the associated
bookkeeping overhead, not the amount of memory used by a single
allocation.  On 64-bit systems, MaxAllocHugeSize is clearly fine,
because it's some unreasonably large value that nobody's ever going to
hit anyway (or at least not until systems with multiple exabytes of
memory start to become common).  On 32-bit systems, IIUC,
MaxAllocHugeSize is just under 2GB.  It's pretty hard to imagine
allocating more than 2GB of storage in a DSA on a 32-bit system,
especially because with the current value of
DSA_NUM_SEGMENTS_AT_EACH_SIZE and the changes introduced by
88f626f8680fbe93444582317d1adb375111855f mean that such systems are
limited to about (4 * 1MB) + (4 * 2MB) + (4 * 4MB) + (4 * 8MB) + (4 *
16MB) + (4 * 32MB) + (4 * 64MB) + (8 * 128MB) = ~1.5GB of allocations
per DSA anyway.  That could be fixed, though, in several different
ways, and then the 2GB limit you're proposing to impose would become
relevant, at least if the system is using something like a 3GB/1GB
user/kernel split rather than 2GB/2GB.  It's probably still not a
practical limitation, though, and even if it is it may not be one we
want to care very much about.  I think the bigger issue is whether
there's any theoretical justification for using  MaxAllocHugeSize to
limit anything other than the size of an individual allocation; to me,
that looks arbitrary.

---

From: Tom Lane <[email-masked]>
Date: Mon, 05 Dec 2016 12:41:16 -0500

Robert Haas <[email-masked]> writes:
> allocation.

Really?  Why doesn't it start out at zero then?

Given your later argumentation, I wonder why we're trying to implement
any kind of limit at all, rather than just operating on the principle
that it's the kernel's problem to enforce a limit.  In short, maybe
removing max_total_segment_size would do fine.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 5 Dec 2016 13:00:45 -0500

> Really?  Why doesn't it start out at zero then?

It seems I misspoke.  It's an upper limit on the total amount of
memory that could be used, not the amount actually used.

> removing max_total_segment_size would do fine.

Well, if we did that, then we'd have to remove dsa_set_size_limit().
I don't want to do that, because I think it's useful for the calling
code to be able to ask this code to enforce a limit that may be less
than the point at which allocations would start failing.  We do that
sort of thing all the time (e.g. work_mem, max_locks_per_transaction)
for good reasons.  Let's not re-engineer this feature now on the
strength of "it produces a compiler warning".  I think the easiest
thing to do here is change SIZE_MAX to (Size) -1.  If there are deeper
problems that need to be addressed, we can consider those separately.

---

From: Tom Lane <[email-masked]>
Date: Mon, 05 Dec 2016 13:08:58 -0500

> problems that need to be addressed, we can consider those separately.

Well, that would solve my immediate problem, which is to be able to
finish the testing Heikki requested on gaur/pademelon.  But I am not
terribly impressed with the concept here.  For example, this bit:

	/*
	 * If the total size limit is already exceeded, then we exit early and
	 * avoid arithmetic wraparound in the unsigned expressions below.
	 */
	if (area->control->total_segment_size >=
		area->control->max_total_segment_size)
		return NULL;

is a no-op, because it's physically impossible to exceed SIZE_MAX, and
that leads me to wonder whether the claim that this test helps prevent
arithmetic overflow has any substance.  Also, given that the idea of
DSA seems to be to support a number of different use-cases, I'm not
sure that it's useful to have a knob that limits the total consumption
rather than individual areas.  IOW: who exactly is going to call
dsa_set_size_limit, and on what grounds would they compute the limit?

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 5 Dec 2016 14:06:29 -0500

On Mon, Dec 5, 2016 at 1:08 PM, Tom Lane <[email-masked]> wrote:
> arithmetic overflow has any substance.

Eh?  Sure, if no limit has been set via dsa_set_size_limit(), then
that test is a no-op.  But if one has been set, then it isn't a no-op.
Which is of course the point.

> Also, given that the idea of

The code that creates the DSA is going to call it.  For example,
suppose we change the stats collector or pg_stat_statements to use
this as a backing store.Or we modify the heavyweight lock manager to
use DSM so that the maximum size of the lock table can be changed
without restarting the server.  Or we implement a shared plan cache.
In any of those cases, there will (presumably) be a GUC that limits
how much memory the relevant facility is allowed to chew up, and that
could be implemented by calling dsa_set_size_limit() to establish a
limit matching the value of that GUC.  Now maybe we'll never do any of
those things or maybe we'll implement the limit entirely on the caller
side, but I don't care to presume that.

Also, we could use this facility to manage memory mapped at fixed
addresses within the main shared memory segment, if we wanted to be
able to allocate and free memory more flexibly in that arena.  If we
did that, we'd use dsa_set_size_limit() to prevent that DSA from
creating additional DSMs, so that it only managed the fixed chunk of
address space originally given to it.

---

From: Tom Lane <[email-masked]>
Date: Mon, 05 Dec 2016 14:32:40 -0500

> The code that creates the DSA is going to call it.

Ah, I misunderstood: from the name of the field and the way you'd
described it, I thought it was a limit on the total space used across
all DSAs.  A per-DSA limit does make sense.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 5 Dec 2016 15:14:01 -0500

> all DSAs.  A per-DSA limit does make sense.

OK, good.  I was rather puzzled as to how that didn't seem like a good
thing to have available.  I'm going to go change SIZE_MAX to (Size) -1
now and we can decide later if something else should be done.