On 8/31/22 00:40, David Rowley wrote:
> On Wed, 31 Aug 2022 at 02:17, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>
>> I wrote:
>>> So maybe we should revisit the question. It'd be worth collecting some
>>> stats about how much extra space would be needed if we force there
>>> to be room for a sentinel.
>>
>> Actually, after ingesting more caffeine, the problem with this for aset.c
>> is that the only way to add space for a sentinel that didn't fit already
>> is to double the space allocation. That's a little daunting, especially
>> remembering how many places deliberately allocate power-of-2-sized
>> arrays.
>
> I decided to try and quantify that by logging the size, MAXALIGN(size)
> and the power of 2 size during AllocSetAlloc and GenerationAlloc. I
> made the pow2_size 0 in GenerationAlloc and in AlloocSetAlloc when
> size > allocChunkLimit.
>
> After running make installcheck, grabbing the records out the log and
> loading them into Postgres, I see that if we did double the pow2_size
> when there's no space for the sentinel byte then we'd go from
> allocating a total of 10.2GB all the way to 16.4GB (!) of
> non-dedicated block aset.c allocations.
>
> select
> round(sum(pow2_Size)::numeric/1024/1024/1024,3) as pow2_size,
> round(sum(case when maxalign_size=pow2_size then pow2_size*2 else
> pow2_size end)::numeric/1024/1024/1024,3) as method1,
> round(sum(case when maxalign_size=pow2_size then pow2_size+8 else
> pow2_size end)::numeric/1024/1024/1024,3) as method2
> from memstats
> where pow2_size > 0;
> pow2_size | method1 | method2
> -----------+---------+---------
> 10.194 | 16.382 | 10.463
>
> if we did just add on an extra 8 bytes (or or MAXALIGN(size+1) at
> least), then that would take the size up to 10.5GB.
>
I've been experimenting with this a bit too, and my results are similar,
but not exactly the same. I've logged all Alloc/Realloc calls for the
two memory contexts, and when I aggregated the results I get this:
f | size | pow2(size) | pow2(size+1)
-----------------+----------+------------+--------------
AllocSetAlloc | 23528 | 28778 | 31504
AllocSetRelloc | 761 | 824 | 1421
GenerationAlloc | 68 | 90 | 102
So the raw size (what we asked for) is ~23.5GB, but in practice we
allocate ~28.8GB because of the pow-of-2 logic. And by adding the extra
1B we end up allocating 31.5GB. That doesn't seem like a huge increase,
and it's far from the +60% you got.
I wonder where does the difference come - I did make installcheck too,
so how come you get 10/16GB, and I get 28/31GB? My patch is attached,
maybe I did something silly.
I also did a quick hack to see if always having the sentinel detects any
pre-existing issues, but that didn't happen. I guess valgrind would find
those, but not sure?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index b6eeb8abab..e6d86a54d7 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -674,6 +674,15 @@ AllocSetDelete(MemoryContext context)
free(set);
}
+static Size pow2_size(Size size)
+{
+ Size s = 1;
+ while (s < size)
+ s *= 2;
+
+ return s;
+}
+
/*
* AllocSetAlloc
* Returns pointer to allocated memory of given size or NULL if
@@ -699,13 +708,16 @@ AllocSetAlloc(MemoryContext context, Size size)
AssertArg(AllocSetIsValid(set));
+ fprintf(stderr, "AllocSetAlloc %ld %ld %ld\n", size, pow2_size(size), pow2_size(size+1));
+ fflush(stderr);
+
/*
* If requested size exceeds maximum for chunks, allocate an entire block
* for this request.
*/
- if (size > set->allocChunkLimit)
+ if (size + 1 > set->allocChunkLimit)
{
- chunk_size = MAXALIGN(size);
+ chunk_size = MAXALIGN(size + 1);
blksize = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
block = (AllocBlock) malloc(blksize);
if (block == NULL)
@@ -725,7 +737,16 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chunk_size)
+ {
set_sentinel(MemoryChunkGetPointer(chunk), size);
+ fprintf(stderr, "sentinel added\n");
+ fflush(stderr);
+ }
+ else
+ {
+ fprintf(stderr, "A sentinel not added\n");
+ fflush(stderr);
+ }
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* fill the allocated space with junk */
@@ -767,7 +788,7 @@ AllocSetAlloc(MemoryContext context, Size size)
* If one is found, remove it from the free list, make it again a member
* of the alloc set and return its data address.
*/
- fidx = AllocSetFreeIndex(size);
+ fidx = AllocSetFreeIndex(size+1);
chunk = set->freelist[fidx];
if (chunk != NULL)
{
@@ -784,7 +805,16 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < GetChunkSizeFromFreeListIdx(fidx))
+ {
set_sentinel(MemoryChunkGetPointer(chunk), size);
+ fprintf(stderr, "sentinel added\n");
+ fflush(stderr);
+ }
+ else
+ {
+ fprintf(stderr, "B sentinel not added\n");
+ fflush(stderr);
+ }
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* fill the allocated space with junk */
@@ -950,7 +980,16 @@ AllocSetAlloc(MemoryContext context, Size size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chunk_size)
+ {
set_sentinel(MemoryChunkGetPointer(chunk), size);
+ fprintf(stderr, "sentinel added\n");
+ fflush(stderr);
+ }
+ else
+ {
+ fprintf(stderr, "C sentinel not added\n");
+ fflush(stderr);
+ }
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* fill the allocated space with junk */
@@ -994,7 +1033,7 @@ AllocSetFree(void *pointer)
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < chunk_size)
if (!sentinel_ok(pointer, chunk->requested_size))
- elog(WARNING, "detected write past chunk end in %s %p",
+ elog(ERROR, "detected write past chunk end in %s %p",
set->header.name, chunk);
}
#endif
@@ -1034,7 +1073,7 @@ AllocSetFree(void *pointer)
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < GetChunkSizeFromFreeListIdx(fidx))
if (!sentinel_ok(pointer, chunk->requested_size))
- elog(WARNING, "detected write past chunk end in %s %p",
+ elog(ERROR, "detected write past chunk end in %s %p",
set->header.name, chunk);
#endif
@@ -1078,6 +1117,9 @@ AllocSetRealloc(void *pointer, Size size)
MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
Size oldsize;
+ fprintf(stderr, "AllocSetRelloc %ld %ld %ld\n", size, pow2_size(size), pow2_size(size+1));
+ fflush(stderr);
+
/* Allow access to private part of chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, ALLOCCHUNK_PRIVATE_LEN);
@@ -1100,7 +1142,7 @@ AllocSetRealloc(void *pointer, Size size)
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < oldsize)
if (!sentinel_ok(pointer, chunk->requested_size))
- elog(WARNING, "detected write past chunk end in %s %p",
+ elog(ERROR, "detected write past chunk end in %s %p",
set->header.name, chunk);
#endif
@@ -1111,7 +1153,7 @@ AllocSetRealloc(void *pointer, Size size)
if (block->freeptr != block->endptr)
elog(ERROR, "could not find block containing chunk %p", chunk);
- chksize = MAXALIGN(size);
+ chksize = MAXALIGN(size+1);
/* Do the realloc */
blksize = chksize + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
@@ -1163,7 +1205,16 @@ AllocSetRealloc(void *pointer, Size size)
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
if (size < chksize)
+ {
set_sentinel(pointer, size);
+ fprintf(stderr, "sentinel added\n");
+ fflush(stderr);
+ }
+ else
+ {
+ fprintf(stderr, "D sentinel not added\n");
+ fflush(stderr);
+ }
#else /* !MEMORY_CONTEXT_CHECKING */
/*
@@ -1191,7 +1242,7 @@ AllocSetRealloc(void *pointer, Size size)
/* Test for someone scribbling on unused space in chunk */
if (chunk->requested_size < oldsize)
if (!sentinel_ok(pointer, chunk->requested_size))
- elog(WARNING, "detected write past chunk end in %s %p",
+ elog(ERROR, "detected write past chunk end in %s %p",
set->header.name, chunk);
#endif
@@ -1200,7 +1251,7 @@ AllocSetRealloc(void *pointer, Size size)
* allocated area already is >= the new size. (In particular, we will
* fall out here if the requested size is a decrease.)
*/
- if (oldsize >= size)
+ if (oldsize >= size + 1)
{
#ifdef MEMORY_CONTEXT_CHECKING
Size oldrequest = chunk->requested_size;
@@ -1227,7 +1278,17 @@ AllocSetRealloc(void *pointer, Size size)
/* set mark to catch clobber of "unused" space */
if (size < oldsize)
+ {
set_sentinel(pointer, size);
+
+ fprintf(stderr, "sentinel added\n");
+ fflush(stderr);
+ }
+ else
+ {
+ fprintf(stderr, "E sentinel not added\n");
+ fflush(stderr);
+ }
#else /* !MEMORY_CONTEXT_CHECKING */
/*
@@ -1532,7 +1593,7 @@ AllocSetCheck(MemoryContext context)
*/
if (dsize != InvalidAllocSize && dsize < chsize &&
!sentinel_ok(chunk, ALLOC_CHUNKHDRSZ + dsize))
- elog(WARNING, "problem in alloc set %s: detected write past chunk end in block %p, chunk %p",
+ elog(ERROR, "problem in alloc set %s: detected write past chunk end in block %p, chunk %p",
name, block, chunk);
/*
diff --git a/src/backend/utils/mmgr/generation.c b/src/backend/utils/mmgr/generation.c
index b39894ec94..2f1363967d 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -323,6 +323,15 @@ GenerationDelete(MemoryContext context)
free(context);
}
+static Size pow2_size(Size size)
+{
+ Size s = 1;
+ while (s < size)
+ s *= 2;
+
+ return s;
+}
+
/*
* GenerationAlloc
* Returns pointer to allocated memory of given size or NULL if
@@ -345,6 +354,9 @@ GenerationAlloc(MemoryContext context, Size size)
Size chunk_size = MAXALIGN(size);
Size required_size = chunk_size + Generation_CHUNKHDRSZ;
+ fprintf(stderr, "GenerationAlloc %ld %ld %ld\n", size, pow2_size(size), pow2_size(size+1));
+ fflush(stderr);
+
/* is it an over-sized chunk? if yes, allocate special block */
if (chunk_size > set->allocChunkLimit)
{
@@ -709,6 +721,9 @@ GenerationRealloc(void *pointer, Size size)
GenerationPointer newPointer;
Size oldsize;
+ fprintf(stderr, "GenerationRealloc %ld %ld %ld\n", size, pow2_size(size), pow2_size(size+1));
+ fflush(stderr);
+
/* Allow access to private part of chunk header. */
VALGRIND_MAKE_MEM_DEFINED(chunk, GENERATIONCHUNK_PRIVATE_LEN);
