Обсуждение: Re: [COMMITTERS] pgsql: Introduce dynamic shared memory areas.
On Fri, Dec 2, 2016 at 9:36 AM, Robert Haas <rhaas@postgresql.org> wrote: > 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. 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
Michael Paquier <michael.paquier@gmail.com> writes:
> 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
On Mon, Dec 5, 2016 at 7:18 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Dec 2, 2016 at 9:36 AM, Robert Haas <rhaas@postgresql.org> wrote:
>> 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.
>
> 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)
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
Robert Haas <robertmhaas@gmail.com> writes:
> 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?
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
On Mon, Dec 5, 2016 at 10:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> 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. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
>> 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
On Mon, Dec 5, 2016 at 11:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> 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. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> 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.
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
On Mon, Dec 5, 2016 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> 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. > > 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. > 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. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Dec 5, 2016 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.
> 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.
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
expressionsbelow. */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
On Mon, Dec 5, 2016 at 1:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Dec 5, 2016 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> 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. > >> 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. > > 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. 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 > 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? 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Dec 5, 2016 at 1:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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?
> 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
On Mon, Dec 5, 2016 at 2:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Dec 5, 2016 at 1:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> 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? > >> 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. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company