Re: Reducing the chunk header sizes on all memory context types
On Thu, Oct 20, 2022 at 1:55 AM Andres Freund wrote: > > Hi, > > On 2022-10-11 10:21:17 +0700, John Naylor wrote: > > On Tue, Oct 11, 2022 at 5:31 AM David Rowley wrote: > > > > > > The proposed patches in [1] do aim to make additional usages of the > > > slab allocator, and I have a feeling that we'll want to fix the > > > performance of slab.c before those. Perhaps the Asserts are a better > > > option if we're to get the proposed radix tree implementation. > > > > Going by [1], that use case is not actually a natural fit for slab because > > of memory fragmentation. > > > > [1] > > https://www.postgresql.org/message-id/20220704220038.at2ane5xkymzzssb%40awork3.anarazel.de > > Not so sure about that - IIRC I made one slab for each different size class, > which seemed to work well and suit slab well? If that's the case, then great! The linked message didn't give me that impression, but I won't worry about it. -- John Naylor EDB: http://www.enterprisedb.com
Re: Reducing the chunk header sizes on all memory context types
Hi, On 2022-10-11 10:21:17 +0700, John Naylor wrote: > On Tue, Oct 11, 2022 at 5:31 AM David Rowley wrote: > > > > The proposed patches in [1] do aim to make additional usages of the > > slab allocator, and I have a feeling that we'll want to fix the > > performance of slab.c before those. Perhaps the Asserts are a better > > option if we're to get the proposed radix tree implementation. > > Going by [1], that use case is not actually a natural fit for slab because > of memory fragmentation. > > [1] > https://www.postgresql.org/message-id/20220704220038.at2ane5xkymzzssb%40awork3.anarazel.de Not so sure about that - IIRC I made one slab for each different size class, which seemed to work well and suit slab well? Greetings, Andres Freund
Re: Reducing the chunk header sizes on all memory context types
On Tue, Oct 11, 2022 at 5:31 AM David Rowley wrote: > > The proposed patches in [1] do aim to make additional usages of the > slab allocator, and I have a feeling that we'll want to fix the > performance of slab.c before those. Perhaps the Asserts are a better > option if we're to get the proposed radix tree implementation. Going by [1], that use case is not actually a natural fit for slab because of memory fragmentation. The motivation to use slab there was that the allocation sizes are just over a power of two, leading to a lot of wasted space for aset. FWIW, I have proposed in that thread a scheme to squeeze things into power-of-two sizes without wasting quite as much space. That's not a done deal, of course, but it could work today without adding memory management code. [1] https://www.postgresql.org/message-id/20220704220038.at2ane5xkymzzssb%40awork3.anarazel.de -- John Naylor EDB: http://www.enterprisedb.com
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > The main reason I brought it up was that only yesterday I was looking > into fixing the slowness of the Slab allocator. It's currently quite > far behind the performance of both generation.c and aset.c and it > would be very nice to bring it up to at least be on-par with those. Really!? That's pretty sad, because surely it should be handling a simpler case. Anyway, I'm about to push this with an Assert in SlabFree and run-time test in SlabRealloc. That should be enough to assuage my safety concerns, and then we can think about better performance. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 11 Oct 2022 at 10:07, Tom Lane wrote: > Yeah, slab.c hasn't any distinction between large and small chunks, > so we have to just pick one policy or the other. I'd hoped to get > away with the more robust runtime test on the basis that slab allocation > is not used so much that this'd result in any noticeable performance > change. SlabRealloc, at least, is not used *at all* per the code > coverage tests, and if we're there at all we should be highly suspicious > that something is wrong. However, I could be wrong about SlabFree, > and if you're going to hold my feet to the fire then I'll change it > rather than try to produce some performance evidence. The main reason I brought it up was that only yesterday I was looking into fixing the slowness of the Slab allocator. It's currently quite far behind the performance of both generation.c and aset.c and it would be very nice to bring it up to at least be on-par with those. Ideally there would be some performance advantages of the fixed-sized chunks. I'd just rather not have any additional things go in to make that goal harder to reach. The proposed patches in [1] do aim to make additional usages of the slab allocator, and I have a feeling that we'll want to fix the performance of slab.c before those. Perhaps the Asserts are a better option if we're to get the proposed radix tree implementation. David [1] https://postgr.es/m/cad21aod3w76wers_lq7_ua6+gtaooerpji+yz8ac6aui4jw...@mail.gmail.com
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > Looking at your changes to SlabFree(), I don't really think that > change is well aligned to the newly proposed policy. My understanding > of the rationale behind this policy is that large chunks get malloced > and will be slower anyway, so the elog(ERROR) is less overhead for > those. In SlabFree(), we're most likely not doing any free()s, so I > don't quite understand why you've added the elog rather than the > Assert for this case. The slab allocator *should* be very fast. Yeah, slab.c hasn't any distinction between large and small chunks, so we have to just pick one policy or the other. I'd hoped to get away with the more robust runtime test on the basis that slab allocation is not used so much that this'd result in any noticeable performance change. SlabRealloc, at least, is not used *at all* per the code coverage tests, and if we're there at all we should be highly suspicious that something is wrong. However, I could be wrong about SlabFree, and if you're going to hold my feet to the fire then I'll change it rather than try to produce some performance evidence. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 11 Oct 2022 at 08:35, Tom Lane wrote: > Hearing no comments on that, I decided that a good policy would be > to use Asserts in the paths dealing with small chunks but test-and-elog > in the paths dealing with large chunks. This seems like a good policy. I think it's good to get at least the Asserts in there. If we have any troubles in the future then we can revisit this and reconsider if we need to elog them instead. > Hence v2 attached, which cleans things up a tad in aset.c and then > extends similar policy to generation.c and slab.c. Looking at your changes to SlabFree(), I don't really think that change is well aligned to the newly proposed policy. My understanding of the rationale behind this policy is that large chunks get malloced and will be slower anyway, so the elog(ERROR) is less overhead for those. In SlabFree(), we're most likely not doing any free()s, so I don't quite understand why you've added the elog rather than the Assert for this case. The slab allocator *should* be very fast. I don't have any issue with any of the other changes. David
Re: Reducing the chunk header sizes on all memory context types
I wrote:
> What I am mainly wondering about at this point is whether Asserts
> are good enough or we should use actual test-and-elog checks for
> these things.
Hearing no comments on that, I decided that a good policy would be
to use Asserts in the paths dealing with small chunks but test-and-elog
in the paths dealing with large chunks. We already had test-and-elog
sanity checks in the latter paths, at least in aset.c, which the new
checks can reasonably be combined with. It's unlikely that the
large-chunk case is at all performance-critical, too. But adding
runtime checks in the small-chunk paths would probably risk losing
some performance in production builds, and I'm not currently prepared
to argue that the problem is big enough to justify that.
Hence v2 attached, which cleans things up a tad in aset.c and then
extends similar policy to generation.c and slab.c. Of note is
that slab.c was doing things like this:
SlabContext *slab = castNode(SlabContext, context);
Assert(slab);
which has about the same effect as what I'm proposing with
AllocSetIsValid, but (a) it's randomly different from the
other allocators, and (b) it's probably a hair less efficient,
because I doubt the compiler can optimize away castNode's
special handling of NULL. So I made these bits follow the
style of aset.c.
regards, tom lane
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec423375ae..db402e3a41 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -132,6 +132,10 @@ typedef struct AllocFreeListLink
#define GetFreeListLink(chkptr) \
(AllocFreeListLink *) ((char *) (chkptr) + ALLOC_CHUNKHDRSZ)
+/* Validate a freelist index retrieved from a chunk header */
+#define FreeListIdxIsValid(fidx) \
+ ((fidx) >= 0 && (fidx) < ALLOCSET_NUM_FREELISTS)
+
/* Determine the size of the chunk based on the freelist index */
#define GetChunkSizeFromFreeListIdx(fidx) \
Size) 1) << ALLOC_MINBITS) << (fidx))
@@ -202,7 +206,15 @@ typedef struct AllocBlockData
* AllocSetIsValid
* True iff set is valid allocation set.
*/
-#define AllocSetIsValid(set) PointerIsValid(set)
+#define AllocSetIsValid(set) \
+ (PointerIsValid(set) && IsA(set, AllocSetContext))
+
+/*
+ * AllocBlockIsValid
+ * True iff block is valid block of allocation set.
+ */
+#define AllocBlockIsValid(block) \
+ (PointerIsValid(block) && AllocSetIsValid((block)->aset))
/*
* We always store external chunks on a dedicated block. This makes fetching
@@ -530,8 +542,7 @@ AllocSetReset(MemoryContext context)
{
AllocSet set = (AllocSet) context;
AllocBlock block;
- Size keepersize PG_USED_FOR_ASSERTS_ONLY
- = set->keeper->endptr - ((char *) set);
+ Size keepersize PG_USED_FOR_ASSERTS_ONLY;
AssertArg(AllocSetIsValid(set));
@@ -540,6 +551,9 @@ AllocSetReset(MemoryContext context)
AllocSetCheck(context);
#endif
+ /* Remember keeper block size for Assert below */
+ keepersize = set->keeper->endptr - ((char *) set);
+
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -598,8 +612,7 @@ AllocSetDelete(MemoryContext context)
{
AllocSet set = (AllocSet) context;
AllocBlock block = set->blocks;
- Size keepersize PG_USED_FOR_ASSERTS_ONLY
- = set->keeper->endptr - ((char *) set);
+ Size keepersize PG_USED_FOR_ASSERTS_ONLY;
AssertArg(AllocSetIsValid(set));
@@ -608,6 +621,9 @@ AllocSetDelete(MemoryContext context)
AllocSetCheck(context);
#endif
+ /* Remember keeper block size for Assert below */
+ keepersize = set->keeper->endptr - ((char *) set);
+
/*
* If the context is a candidate for a freelist, put it into that freelist
* instead of destroying it.
@@ -994,9 +1010,16 @@ AllocSetFree(void *pointer)
if (MemoryChunkIsExternal(chunk))
{
-
+ /* Release single-chunk block. */
AllocBlock block = ExternalChunkGetBlock(chunk);
+ /*
+ * Try to verify that we have a sane block pointer: the block header
+ * should reference an aset and the freeptr should match the endptr.
+ */
+ if (!AllocBlockIsValid(block) || block->freeptr != block->endptr)
+ elog(ERROR, "could not find block containing chunk %p", chunk);
+
set = block->aset;
#ifdef MEMORY_CONTEXT_CHECKING
@@ -1011,14 +1034,6 @@ AllocSetFree(void *pointer)
}
#endif
-
- /*
- * Try to verify that we have a sane block pointer, the freeptr should
- * match the endptr.
- */
- if (block->freeptr != block->endptr)
- elog(ERROR, "could not find block containing chunk %p", chunk);
-
/* OK, remove block from aset's list and free it */
if (block->prev)
block->prev->next = block->next;
@@ -1036,12 +1051,23 @@ AllocSetFree(void *pointer)
}
else
{
- int fidx = MemoryChunkGetValue(chunk);
AllocBlock block = MemoryChunkGetBlock(chunk);
- AllocFreeListLink *link = GetFreeListLink(chunk);
+ int fidx;
+ AllocFreeListLink *link;
+ /*
+ * In this path, for speed reasons we just Assert that
Re: Reducing the chunk header sizes on all memory context types
So I pushed that, but I don't feel that we're out of the woods yet.
As I mentioned at [1], while testing this stuff I hit a case where
aset.c will try to wipe_mem practically the entire address space after
being asked to pfree() an invalid pointer. The specific reproducer
isn't too interesting because it depends on the pre-80ef92675 state of
the code, but what I take away from this is that aset.c is still far
too fragile as it stands.
One problem is that aset.c generally isn't too careful about whether
a putative chunk is actually one of its chunks. That was okay before
c6e0fe1f2 because we would never get to AllocSetFree etc unless the
word before the chunk data pointed at a moderately-sane AllocSet.
Now, we can arrive at that code on the strength of three random bits,
so it's got to be more careful. In the attached patch, I make
AllocSetIsValid do an IsA() test, and make sure to apply it to the
aset we think we have found from the chunk header. This is not in
any way a new check: what it is doing is replacing the IsA check done
by the "AssertArg(MemoryContextIsValid(context))" that was performed
by GetMemoryChunkContext in the old code, and that we've completely
lost in the code as it stands.
The other problem, which is what is leading to wipe_mem-the-universe,
is that aset.c figures the size of a non-external chunk essentially
as "1 << MemoryChunkGetValue(chunk)", where the "value" field is 30
bits wide and has undergone exactly zero validation before
AllocSetFree uses the size in memset. That's far, far too trusting.
In the attached I put in some asserts to verify that the value field
is in the valid range for a freelist index, which should usually
trigger if we have a garbage value, or at the very least constrain
the damage.
What I am mainly wondering about at this point is whether Asserts
are good enough or we should use actual test-and-elog checks for
these things. The precedent of the old GetMemoryChunkContext
implementation suggests that assertions are good enough for the
AllocSetIsValid tests. On the other hand, there are existing
cross-checks like
if (block->freeptr != block->endptr)
elog(ERROR, "could not find block containing chunk %p", chunk);
so at least in some code paths we've thought it is worth expending
such tests in production builds. Any opinions?
I imagine generation.c and slab.c need similar bulletproofing
measures, but I didn't look at them yet.
regards, tom lane
[1] https://www.postgresql.org/message-id/3436789.1665187055%40sss.pgh.pa.us
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec423375ae..76a9f02bd8 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -132,6 +132,10 @@ typedef struct AllocFreeListLink
#define GetFreeListLink(chkptr) \
(AllocFreeListLink *) ((char *) (chkptr) + ALLOC_CHUNKHDRSZ)
+/* Validate a freelist index retrieved from a chunk header */
+#define FreeListIdxIsValid(fidx) \
+ ((fidx) >= 0 && (fidx) < ALLOCSET_NUM_FREELISTS)
+
/* Determine the size of the chunk based on the freelist index */
#define GetChunkSizeFromFreeListIdx(fidx) \
Size) 1) << ALLOC_MINBITS) << (fidx))
@@ -202,7 +206,15 @@ typedef struct AllocBlockData
* AllocSetIsValid
* True iff set is valid allocation set.
*/
-#define AllocSetIsValid(set) PointerIsValid(set)
+#define AllocSetIsValid(set) \
+ (PointerIsValid(set) && IsA(set, AllocSetContext))
+
+/*
+ * AllocBlockIsValid
+ * True iff block is valid block of allocation set.
+ */
+#define AllocBlockIsValid(block) \
+ (PointerIsValid(block) && AllocSetIsValid((block)->aset))
/*
* We always store external chunks on a dedicated block. This makes fetching
@@ -530,8 +542,7 @@ AllocSetReset(MemoryContext context)
{
AllocSet set = (AllocSet) context;
AllocBlock block;
- Size keepersize PG_USED_FOR_ASSERTS_ONLY
- = set->keeper->endptr - ((char *) set);
+ Size keepersize PG_USED_FOR_ASSERTS_ONLY;
AssertArg(AllocSetIsValid(set));
@@ -540,6 +551,9 @@ AllocSetReset(MemoryContext context)
AllocSetCheck(context);
#endif
+ /* Remember keeper block size for Assert below */
+ keepersize = set->keeper->endptr - ((char *) set);
+
/* Clear chunk freelists */
MemSetAligned(set->freelist, 0, sizeof(set->freelist));
@@ -598,8 +612,7 @@ AllocSetDelete(MemoryContext context)
{
AllocSet set = (AllocSet) context;
AllocBlock block = set->blocks;
- Size keepersize PG_USED_FOR_ASSERTS_ONLY
- = set->keeper->endptr - ((char *) set);
+ Size keepersize PG_USED_FOR_ASSERTS_ONLY;
AssertArg(AllocSetIsValid(set));
@@ -608,6 +621,9 @@ AllocSetDelete(MemoryContext context)
AllocSetCheck(context);
#endif
+ /* Remember keeper block size for Assert below */
+ keepersize = set->keeper->endptr - ((char *) set);
+
/*
* If the context is a candidate for a freelist, put it into that freelist
* instead of destroying it.
@@ -994,9 +1010,10 @@ AllocSetFree(void *pointer)
if
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > On Fri, 7 Oct 2022 at 12:35, Tom Lane wrote: >> Which leaves me with the attached proposed wording. > No objections here. Cool, I'll push in a little bit. > With these comments I'd be using slot MCTX_UNUSED4_ID first, then I'd > probably be looking at MCTX_UNUSED5_ID after adjusting wipe_mem to do > something other than setting bytes to 0x7F. Well, the only way that you could free up a bitpattern that way is to make wipe_mem use something ending in 000 or 001. I'd be against using 000 because then wiped memory might appear to contain valid (aligned) pointers. But perhaps 001 would be ok. > I'd then use > MCTX_UNUSED3_ID since that pattern is only used for larger chunks with > glibc (per your findings). After that, I'd probably start looking > into making more than 3 bits available. If that wasn't possible, I'd > be using MCTX_UNUSED2_ID and at last resort MCTX_UNUSED1_ID. If we get to having three-quarters or seven-eighths of the bitpatterns being valid IDs, we'll have precious little ability to detect garbage. So personally I'd put "find a fourth bit" higher on the priority list. In any case, we needn't invest more effort here until someone comes with a fifth context method ... and I don't recall hearing discussions of even a fourth one yet. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Fri, 7 Oct 2022 at 12:35, Tom Lane wrote: > Which leaves me with the attached proposed wording. No objections here. With these comments I'd be using slot MCTX_UNUSED4_ID first, then I'd probably be looking at MCTX_UNUSED5_ID after adjusting wipe_mem to do something other than setting bytes to 0x7F. I'd then use MCTX_UNUSED3_ID since that pattern is only used for larger chunks with glibc (per your findings). After that, I'd probably start looking into making more than 3 bits available. If that wasn't possible, I'd be using MCTX_UNUSED2_ID and at last resort MCTX_UNUSED1_ID. David
Re: Reducing the chunk header sizes on all memory context types
I wrote:
> So I'm still inclined to leave 001 and 010 both unused, but the
> reason why is different than I thought before.
Which leaves me with the attached proposed wording.
regards, tom lane
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index c80236d285..b1a3c74830 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -32,6 +32,11 @@
#include "utils/memutils_internal.h"
+static void BogusFree(void *pointer);
+static void *BogusRealloc(void *pointer, Size size);
+static MemoryContext BogusGetChunkContext(void *pointer);
+static Size BogusGetChunkSpace(void *pointer);
+
/*
* GLOBAL MEMORY *
*/
@@ -74,10 +79,42 @@ static const MemoryContextMethods mcxt_methods[] = {
[MCTX_SLAB_ID].get_chunk_context = SlabGetChunkContext,
[MCTX_SLAB_ID].get_chunk_space = SlabGetChunkSpace,
[MCTX_SLAB_ID].is_empty = SlabIsEmpty,
- [MCTX_SLAB_ID].stats = SlabStats
+ [MCTX_SLAB_ID].stats = SlabStats,
#ifdef MEMORY_CONTEXT_CHECKING
- ,[MCTX_SLAB_ID].check = SlabCheck
+ [MCTX_SLAB_ID].check = SlabCheck,
#endif
+
+ /*
+ * Unused (as yet) IDs should have dummy entries here. This allows us to
+ * fail cleanly if a bogus pointer is passed to pfree or the like. It
+ * seems sufficient to provide routines for the methods that might get
+ * invoked from inspection of a chunk (see MCXT_METHOD calls below).
+ */
+
+ [MCTX_UNUSED1_ID].free_p = BogusFree,
+ [MCTX_UNUSED1_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED1_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED1_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED2_ID].free_p = BogusFree,
+ [MCTX_UNUSED2_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED2_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED2_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED3_ID].free_p = BogusFree,
+ [MCTX_UNUSED3_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED3_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED3_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED4_ID].free_p = BogusFree,
+ [MCTX_UNUSED4_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED4_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED4_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED5_ID].free_p = BogusFree,
+ [MCTX_UNUSED5_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED5_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED5_ID].get_chunk_space = BogusGetChunkSpace,
};
/*
@@ -125,6 +162,77 @@ static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
#define MCXT_METHOD(pointer, method) \
mcxt_methods[GetMemoryChunkMethodID(pointer)].method
+/*
+ * GetMemoryChunkMethodID
+ * Return the MemoryContextMethodID from the uint64 chunk header which
+ * directly precedes 'pointer'.
+ */
+static inline MemoryContextMethodID
+GetMemoryChunkMethodID(const void *pointer)
+{
+ uint64 header;
+
+ /*
+ * Try to detect bogus pointers handed to us, poorly though we can.
+ * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
+ * allocated chunk.
+ */
+ Assert(pointer == (const void *) MAXALIGN(pointer));
+
+ header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+
+ return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
+}
+
+/*
+ * GetMemoryChunkHeader
+ * Return the uint64 chunk header which directly precedes 'pointer'.
+ *
+ * This is only used after GetMemoryChunkMethodID, so no need for error checks.
+ */
+static inline uint64
+GetMemoryChunkHeader(const void *pointer)
+{
+ return *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+}
+
+/*
+ * Support routines to trap use of invalid memory context method IDs
+ * (from calling pfree or the like on a bogus pointer). As a possible
+ * aid in debugging, we report the header word along with the pointer
+ * address (if we got here, there must be an accessible header word).
+ */
+static void
+BogusFree(void *pointer)
+{
+ elog(ERROR, "pfree called with invalid pointer %p (header 0x%016llx)",
+ pointer, (long long) GetMemoryChunkHeader(pointer));
+}
+
+static void *
+BogusRealloc(void *pointer, Size size)
+{
+ elog(ERROR, "repalloc called with invalid pointer %p (header 0x%016llx)",
+ pointer, (long long) GetMemoryChunkHeader(pointer));
+ return NULL;/* keep compiler quiet */
+}
+
+static MemoryContext
+BogusGetChunkContext(void *pointer)
+{
+ elog(ERROR, "GetMemoryChunkContext called with invalid pointer %p (header 0x%016llx)",
+ pointer, (long long) GetMemoryChunkHeader(pointer));
+ return NULL;/* keep compiler quiet */
+}
+
+static Size
+BogusGetChunkSpace(void *pointer)
+{
+ elog(ERROR, "GetMemoryChunkSpace called with invalid pointer %p (header 0x%016llx)",
+ pointer, (long long) GetMemoryChunkHeader(pointer));
+ return 0; /* keep compiler quiet */
+}
+
Re: Reducing the chunk header sizes on all memory context types
I wrote: > FreeBSD 13.0, arm64: Usually the low-order nibble is or , > but for some smaller values of N it sometimes comes up as 0010. > NetBSD 9.2, amd64: results similar to FreeBSD. I looked into NetBSD's malloc.c, and what I discovered is that their implementation doesn't have any chunk headers: chunks of the same size are allocated consecutively within pages, and all the bookkeeping data is somewhere else. Presumably FreeBSD is the same. So the apparent special case with 0010 is an illusion, even though I saw it on two different machines (maybe it's a specific value that we're allocating??) The most likely case is due to the immediately previous word having never been used (note that like palloc, they round chunk sizes up to powers of two, so unused space at the end of a chunk is common). I'm not sure whether the cases I saw with are chance artifacts or reflect some real mechanism, but probably the former. I thought for a bit that that might be the effects of wipe_mem on the previous chunk, but palloc'd storage would never share the same page as malloc'd storage under this allocator, because we grab it from malloc in larger-than-page chunks. However ... after looking into glib's malloc.c, I find that it does use a chunk header, and very conveniently the three bits that we care about are flag bits (at least on 64-bit machines): chunk-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | Size of previous chunk, if unallocated (P clear) | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | Size of chunk, in bytes |A|M|P| mem-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | User data starts here... . The A bit is only used when threading, and hence should always be zero in our usage. The M bit only gets set in chunks large enough to be separately mmap'd, so when it is set P must be 0. If M is not set then P seems to usually be 1, although it could be 0. So the three possibilities for what we can see under glibc are 000, 001, 010 (the last only occuring for chunks larger than 128K). This squares with experimental results on my machine --- I'd not thought to try sizes above 100K before. So I'm still inclined to leave 001 and 010 both unused, but the reason why is different than I thought before. Going forward, we could commandeer 010 if we need to without losing very much debuggability, since malloc'ing more than 128K in a chunk won't happen often. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Fri, 7 Oct 2022 at 10:57, Tom Lane wrote:
> I poked at this some more by creating a function that intentionally
> does pfree(malloc(N)) for various values of N.
>
> RHEL8, x86_64: the low-order nibble of the header is consistently 0001.
>
> macOS 12.6, arm64: the low-order nibble is consistently .
>
> FreeBSD 13.0, arm64: Usually the low-order nibble is or ,
> but for some smaller values of N it sometimes comes up as 0010.
>
> NetBSD 9.2, amd64: results similar to FreeBSD.
Out of curiosity I tried using the attached on a Windows machine and got:
0: 130951
1: 131061
2: 133110
3: 129053
4: 131061
5: 131067
6: 131070
7: 131203
So it seems pretty much entirely inconsistent on that platform.
Also, on an Ubuntu machine I didn't get the consistent 0001 as you got
on your RHEL machine. There were a very small number of 010's there
too:
0: 0
1: 1048569
2: 7
3: 0
4: 0
5: 0
6: 0
7: 0
Despite Windows not being very consistent here, I think it's a useful
change as if our most common platform (Linux/glibc) is fairly
consistent, then that'll give us wide coverage to track down any buggy
code.
In anycase, even on Windows (assuming it's completely random) we'll
have a 5 out of 8 chance of getting a nice error message if there are
any bad pointers being passed.
David
#include
#include
typedef unsigned long long uint64;
int main(void)
{
size_t count[8] = {0};
size_t n;
for (n = 0; n < 1024*1024; n++)
{
void *ptr = (void *) malloc(n);
uint64 header;
if (!ptr)
{
fprintf(stderr, "OOM\n");
return -1;
}
header = *((uint64 *) ((char *) ptr - sizeof(uint64)));
count[header & 7]++;
free(ptr);
}
for (n = 0; n <= 7; n++)
printf("%zd: %zd\n", n, count[n]);
return 0;
}
Re: Reducing the chunk header sizes on all memory context types
I wrote: > I also avoided using 001: based on my work with converting guc.c to use > palloc [1], it seems that pfree'ing a malloc-provided pointer is likely > to see 001 a lot, at least on 64-bit glibc platforms. I poked at this some more by creating a function that intentionally does pfree(malloc(N)) for various values of N. RHEL8, x86_64: the low-order nibble of the header is consistently 0001. macOS 12.6, arm64: the low-order nibble is consistently . FreeBSD 13.0, arm64: Usually the low-order nibble is or , but for some smaller values of N it sometimes comes up as 0010. NetBSD 9.2, amd64: results similar to FreeBSD. I still haven't looked into anybody's source code, but based on these results I'm inclined to leave both 001 and 010 IDs unused for now. That'll help the GUC malloc -> palloc transition tremendously, because people will get fairly clear errors rather than weird assertions and/or memory corruption. That'll leave us in a situation where only one more context ID can be assigned without risk of reducing our error detection ability, but I'm content with that for now. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > However, maybe you've left it this way as you feel it's a decision > that must be made in the future, perhaps based on how difficult it > would be to free up another bit? Yeah, pretty much. I think it'll be a long time before we run out of memory context IDs, and it's hard to anticipate the tradeoffs that will matter at that time. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Fri, 7 Oct 2022 at 09:05, Tom Lane wrote: > > Here's a v2 incorporating discussed changes. > > In reordering enum MemoryContextMethodID, I arranged to avoid using > 000 and 111 as valid IDs, since those bit patterns will appear in > zeroed and wipe_mem'd memory respectively. Those should probably be > more-or-less-permanent exclusions, so I added comments about it. I'm just considering some future developer here that is writing a new MemoryContext type and there's no more space left and she or he needs to either use 000 or 111. I think if that was me, I might be unsure if I should be looking to expand the bit-space to make room. I might think that based on the word "avoid" in: > + MCTX_UNUSED1_ID, /* avoid: 000 occurs in never-used memory */ > + MCTX_UNUSED5_ID /* avoid: 111 occurs in wipe_mem'd memory */ but the final sentence in: > + * dummy entries for unused IDs in the mcxt_methods[] array. We also try > + * to avoid using bit-patterns as valid IDs if they are likely to occur in > + * garbage data. leads me to believe we're just *trying* to avoid using these bit-patterns. Also, the comment in mcxt_methods[] might make me believe that it's ok for me to use them if I really need to. > + * Unused (as yet) IDs should have dummy entries here. This allows us to Based on these comments, I'm not quite sure if I should be completely avoiding using 000 and 111 or I should just use those last when there are no other free slots in the array. It might be quite a long time before someone is in this situation, so should we be more clear? However, maybe you've left it this way as you feel it's a decision that must be made in the future, perhaps based on how difficult it would be to free up another bit? David
Re: Reducing the chunk header sizes on all memory context types
Here's a v2 incorporating discussed changes.
In reordering enum MemoryContextMethodID, I arranged to avoid using
000 and 111 as valid IDs, since those bit patterns will appear in
zeroed and wipe_mem'd memory respectively. Those should probably be
more-or-less-permanent exclusions, so I added comments about it.
I also avoided using 001: based on my work with converting guc.c to use
palloc [1], it seems that pfree'ing a malloc-provided pointer is likely
to see 001 a lot, at least on 64-bit glibc platforms. I've not stuck
my nose into the glibc sources to see how consistent that might be,
but it definitely recurred several times while I was chasing down
places needing adjustment in that patch.
I'm not sure if there are any platform-dependent reasons to avoid
other bit-patterns, but we do still have a little bit of flexibility
left here if anyone has candidates.
regards, tom lane
[1] https://www.postgresql.org/message-id/2982579.1662416866%40sss.pgh.pa.us
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index c80236d285..b1a3c74830 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -32,6 +32,11 @@
#include "utils/memutils_internal.h"
+static void BogusFree(void *pointer);
+static void *BogusRealloc(void *pointer, Size size);
+static MemoryContext BogusGetChunkContext(void *pointer);
+static Size BogusGetChunkSpace(void *pointer);
+
/*
* GLOBAL MEMORY *
*/
@@ -74,10 +79,42 @@ static const MemoryContextMethods mcxt_methods[] = {
[MCTX_SLAB_ID].get_chunk_context = SlabGetChunkContext,
[MCTX_SLAB_ID].get_chunk_space = SlabGetChunkSpace,
[MCTX_SLAB_ID].is_empty = SlabIsEmpty,
- [MCTX_SLAB_ID].stats = SlabStats
+ [MCTX_SLAB_ID].stats = SlabStats,
#ifdef MEMORY_CONTEXT_CHECKING
- ,[MCTX_SLAB_ID].check = SlabCheck
+ [MCTX_SLAB_ID].check = SlabCheck,
#endif
+
+ /*
+ * Unused (as yet) IDs should have dummy entries here. This allows us to
+ * fail cleanly if a bogus pointer is passed to pfree or the like. It
+ * seems sufficient to provide routines for the methods that might get
+ * invoked from inspection of a chunk (see MCXT_METHOD calls below).
+ */
+
+ [MCTX_UNUSED1_ID].free_p = BogusFree,
+ [MCTX_UNUSED1_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED1_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED1_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED2_ID].free_p = BogusFree,
+ [MCTX_UNUSED2_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED2_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED2_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED3_ID].free_p = BogusFree,
+ [MCTX_UNUSED3_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED3_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED3_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED4_ID].free_p = BogusFree,
+ [MCTX_UNUSED4_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED4_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED4_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED5_ID].free_p = BogusFree,
+ [MCTX_UNUSED5_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED5_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED5_ID].get_chunk_space = BogusGetChunkSpace,
};
/*
@@ -125,6 +162,77 @@ static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
#define MCXT_METHOD(pointer, method) \
mcxt_methods[GetMemoryChunkMethodID(pointer)].method
+/*
+ * GetMemoryChunkMethodID
+ * Return the MemoryContextMethodID from the uint64 chunk header which
+ * directly precedes 'pointer'.
+ */
+static inline MemoryContextMethodID
+GetMemoryChunkMethodID(const void *pointer)
+{
+ uint64 header;
+
+ /*
+ * Try to detect bogus pointers handed to us, poorly though we can.
+ * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
+ * allocated chunk.
+ */
+ Assert(pointer == (const void *) MAXALIGN(pointer));
+
+ header = *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+
+ return (MemoryContextMethodID) (header & MEMORY_CONTEXT_METHODID_MASK);
+}
+
+/*
+ * GetMemoryChunkHeader
+ * Return the uint64 chunk header which directly precedes 'pointer'.
+ *
+ * This is only used after GetMemoryChunkMethodID, so no need for error checks.
+ */
+static inline uint64
+GetMemoryChunkHeader(const void *pointer)
+{
+ return *((const uint64 *) ((const char *) pointer - sizeof(uint64)));
+}
+
+/*
+ * Support routines to trap use of invalid memory context method IDs
+ * (from calling pfree or the like on a bogus pointer). As a possible
+ * aid in debugging, we report the header word along with the pointer
+ * address (if we got here, there must be an accessible header word).
+ */
+static void
+BogusFree(void *pointer)
+{
+ elog(ERROR, "pfree called with invalid pointer %p (header 0x%016llx)",
+ pointer, (long long)
Re: Reducing the chunk header sizes on all memory context types
Andres Freund writes: > On 2022-10-06 15:10:44 -0400, Tom Lane wrote: >> Andres Freund writes: >>> Maybe worth printing the method ID as well? >> I doubt it'd be useful. > I was thinking it could be useful to see whether the bits are likely to be the > result of wipe_mem(). But I guess for that we should print the whole byte, > rather than just the method. Perhaps not worth it. I think printing the whole int64 header word would be appropriate if we were hoping for something like that. Still not sure if it's useful. On the other hand, if control gets there then you are probably in need of debugging help ... regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
Hi, On 2022-10-06 15:10:44 -0400, Tom Lane wrote: > Andres Freund writes: > >> + elog(ERROR, "pfree called with invalid pointer %p", pointer); > > > Maybe worth printing the method ID as well? > > I doubt it'd be useful. I was thinking it could be useful to see whether the bits are likely to be the result of wipe_mem(). But I guess for that we should print the whole byte, rather than just the method. Perhaps not worth it. Greetings, Andres Freund
Re: Reducing the chunk header sizes on all memory context types
Andres Freund writes: > Yea, that makes sense. I wouldn't get rid of the MAXALIGN Assert though - it's > not replaced by the the unused mcxt stuff afaics. OK. >> +elog(ERROR, "pfree called with invalid pointer %p", pointer); > Maybe worth printing the method ID as well? I doubt it'd be useful. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
Hi,
On 2022-10-06 14:19:21 -0400, Tom Lane wrote:
> One more thing: based on what I saw in working with my pending guc.c
> changes, the assertions in GetMemoryChunkMethodID are largely useless
> for detecting bogus pointers. I think we should do something more
> like the attached, which will result in a clean failure if the method
> ID bits are invalid.
Yea, that makes sense. I wouldn't get rid of the MAXALIGN Assert though - it's
not replaced by the the unused mcxt stuff afaics.
> I'm a little tempted also to rearrange the MemoryContextMethodID enum
> so that likely bit patterns like 000 are not valid IDs.
Yea, I was suggesting that during a review as well. We can still relax it
later if we run out of bits.
> +/*
> + * Support routines to trap use of invalid memory context method IDs
> + * (from calling pfree or the like on a bogus pointer).
> + */
> +static void
> +BogusFree(void *pointer)
> +{
> + elog(ERROR, "pfree called with invalid pointer %p", pointer);
> +}
Maybe worth printing the method ID as well?
Greetings,
Andres Freund
Re: Reducing the chunk header sizes on all memory context types
One more thing: based on what I saw in working with my pending guc.c
changes, the assertions in GetMemoryChunkMethodID are largely useless
for detecting bogus pointers. I think we should do something more
like the attached, which will result in a clean failure if the method
ID bits are invalid.
I'm a little tempted also to rearrange the MemoryContextMethodID enum
so that likely bit patterns like 000 are not valid IDs.
While I didn't change it here, I wonder why GetMemoryChunkMethodID is
publicly exposed at all. AFAICS it could be static inside mcxt.c.
regards, tom lane
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index c80236d285..e82d9b21ba 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -32,6 +32,11 @@
#include "utils/memutils_internal.h"
+static void BogusFree(void *pointer);
+static void *BogusRealloc(void *pointer, Size size);
+static MemoryContext BogusGetChunkContext(void *pointer);
+static Size BogusGetChunkSpace(void *pointer);
+
/*
* GLOBAL MEMORY *
*/
@@ -74,10 +79,42 @@ static const MemoryContextMethods mcxt_methods[] = {
[MCTX_SLAB_ID].get_chunk_context = SlabGetChunkContext,
[MCTX_SLAB_ID].get_chunk_space = SlabGetChunkSpace,
[MCTX_SLAB_ID].is_empty = SlabIsEmpty,
- [MCTX_SLAB_ID].stats = SlabStats
+ [MCTX_SLAB_ID].stats = SlabStats,
#ifdef MEMORY_CONTEXT_CHECKING
- ,[MCTX_SLAB_ID].check = SlabCheck
+ [MCTX_SLAB_ID].check = SlabCheck,
#endif
+
+ /*
+ * Unused (as yet) IDs should have dummy entries here. This allows us to
+ * fail cleanly if a bogus pointer is passed to pfree or the like. It
+ * seems sufficient to provide routines for the methods that might get
+ * invoked from inspection of a chunk (see MCXT_METHOD calls below).
+ */
+
+ [MCTX_UNUSED3_ID].free_p = BogusFree,
+ [MCTX_UNUSED3_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED3_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED3_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED4_ID].free_p = BogusFree,
+ [MCTX_UNUSED4_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED4_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED4_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED5_ID].free_p = BogusFree,
+ [MCTX_UNUSED5_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED5_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED5_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED6_ID].free_p = BogusFree,
+ [MCTX_UNUSED6_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED6_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED6_ID].get_chunk_space = BogusGetChunkSpace,
+
+ [MCTX_UNUSED7_ID].free_p = BogusFree,
+ [MCTX_UNUSED7_ID].realloc = BogusRealloc,
+ [MCTX_UNUSED7_ID].get_chunk_context = BogusGetChunkContext,
+ [MCTX_UNUSED7_ID].get_chunk_space = BogusGetChunkSpace,
};
/*
@@ -125,6 +162,34 @@ static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
#define MCXT_METHOD(pointer, method) \
mcxt_methods[GetMemoryChunkMethodID(pointer)].method
+/*
+ * Support routines to trap use of invalid memory context method IDs
+ * (from calling pfree or the like on a bogus pointer).
+ */
+static void
+BogusFree(void *pointer)
+{
+ elog(ERROR, "pfree called with invalid pointer %p", pointer);
+}
+
+static void *
+BogusRealloc(void *pointer, Size size)
+{
+ elog(ERROR, "repalloc called with invalid pointer %p", pointer);
+}
+
+static MemoryContext
+BogusGetChunkContext(void *pointer)
+{
+ elog(ERROR, "GetMemoryChunkContext called with invalid pointer %p", pointer);
+}
+
+static Size
+BogusGetChunkSpace(void *pointer)
+{
+ elog(ERROR, "GetMemoryChunkSpace called with invalid pointer %p", pointer);
+}
+
/*
* EXPORTED ROUTINES *
diff --git a/src/include/utils/memutils_internal.h b/src/include/utils/memutils_internal.h
index 377cee7a84..797409ee94 100644
--- a/src/include/utils/memutils_internal.h
+++ b/src/include/utils/memutils_internal.h
@@ -77,12 +77,20 @@ extern void SlabCheck(MemoryContext context);
* The maximum value for this enum is constrained by
* MEMORY_CONTEXT_METHODID_MASK. If an enum value higher than that is
* required then MEMORY_CONTEXT_METHODID_BITS will need to be increased.
+ * For robustness, ensure that MemoryContextMethodID has a value for
+ * each possible bit-pattern of MEMORY_CONTEXT_METHODID_MASK, and make
+ * dummy entries for unused IDs in the mcxt_methods[] array.
*/
typedef enum MemoryContextMethodID
{
MCTX_ASET_ID,
MCTX_GENERATION_ID,
MCTX_SLAB_ID,
+ MCTX_UNUSED3_ID,
+ MCTX_UNUSED4_ID,
+ MCTX_UNUSED5_ID,
+ MCTX_UNUSED6_ID,
+ MCTX_UNUSED7_ID
} MemoryContextMethodID;
/*
@@ -110,20 +118,11 @@ extern void MemoryContextCreate(MemoryContext node,
* directly precedes
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > I did see that PostGIS does use > MemoryContextContains(), though I didn't look at their code to figure > out if they're always passing it a pointer to an allocated chunk. As far as I can tell from a cursory look, they should be able to use the GetMemoryChunkContext workaround, because they are just doing this with SHARED_GSERIALIZED objects, which seem to always be palloc'd. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > On Wed, 5 Oct 2022 at 04:55, Tom Lane wrote: >> After studying the existing usages of MemoryContextContains, I think >> there is a better answer, which is to just nuke them. > I was under the impression you wanted to keep that function around in > cassert builds for some of the guc.c changes you were making. I would've liked to have it, but for that purpose an unreliable version of MemoryContextContains is probably little help. In any case, as you mention, I can do something with GetMemoryChunkContext() instead. >> As far as I can tell, the datumCopy steps associated with aggregate >> finalfns are basically useless. They only serve to prevent >> returning a pointer directly to the aggregate's transition value >> (or, perhaps, to a portion thereof). But what's wrong with that? >> It'll last as long as we need it to. Maybe there was a reason >> back before we required finalfns to not scribble on the transition >> values, but those days are gone. > Yeah, I wondered the same thing. I couldn't see a situation where the > aggregate context would disappear. I have a feeling that we might once have aggressively reset the aggregate context ... but we don't anymore. >> The one place where we actually need the conditional datumCopy is >> with window functions, and even there I don't think we need it >> in simple cases with only one window function. The case that is >> hazardous is where multiple window functions are sharing a >> WindowObject. So I'm content to optimize the single-window-function >> case and just always copy if there's more than one. (Sadly, there >> is no existing regression test that catches this, so I added one.) > I was unsure what window functions might exist out in the wild, so I'd > added some code to pass along the return type information so that any > extensions which need to make a copy can do so. However, maybe it's > better just to wait to see if anyone complains about that before we go > to the trouble. I'd originally feared that a window function might return a pointer into the WindowObject's tuplestore, which we manipulate immediately after the window function returns. However, AFAICS the APIs we provide don't have any such hazard. The actual hazard is that we might get a pointer into one of the temp slots, which are independent storage because we tell them to copy the source tuple. (Maybe a comment about that would be appropriate.) > I've looked at your patches and don't see any problems. Our findings > seem to be roughly the same. i.e the datumCopy is mostly useless. Cool, I'll push in a little bit. > Maybe it's worth doing; > #define MemoryContextContains(c, p) (GetMemoryChunkContext(p) == (c)) > in memutils.h? or are we better to force extension authors to > re-evaluate their code in case anyone is passing memory that's not > pointing directly to a palloc'd chunk? I think the latter. The fact that MemoryContextContains was (mostly) safe on arbitrary pointers was an important part of its API IMO. I'm okay with giving up that property to reduce chunk overhead, but we'll do nobody any service by pretending we still have it. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Wed, 5 Oct 2022 at 04:55, Tom Lane wrote: > After studying the existing usages of MemoryContextContains, I think > there is a better answer, which is to just nuke them. I was under the impression you wanted to keep that function around in cassert builds for some of the guc.c changes you were making. > As far as I can tell, the datumCopy steps associated with aggregate > finalfns are basically useless. They only serve to prevent > returning a pointer directly to the aggregate's transition value > (or, perhaps, to a portion thereof). But what's wrong with that? > It'll last as long as we need it to. Maybe there was a reason > back before we required finalfns to not scribble on the transition > values, but those days are gone. Yeah, I wondered the same thing. I couldn't see a situation where the aggregate context would disappear. > The same goes for aggregate serialfns --- although there, I can't > avoid the feeling that the datumCopy step was just cargo-culted in. > I don't think there can exist a serialfn that doesn't return a > freshly-palloced bytea. Most likely. I probably copied that as I wouldn't have understood why we did any copying when calling the finalfn. I still don't understand why. Seems there's no good reason if we're both in favour of removing it. > The one place where we actually need the conditional datumCopy is > with window functions, and even there I don't think we need it > in simple cases with only one window function. The case that is > hazardous is where multiple window functions are sharing a > WindowObject. So I'm content to optimize the single-window-function > case and just always copy if there's more than one. (Sadly, there > is no existing regression test that catches this, so I added one.) I was unsure what window functions might exist out in the wild, so I'd added some code to pass along the return type information so that any extensions which need to make a copy can do so. However, maybe it's better just to wait to see if anyone complains about that before we go to the trouble. I've looked at your patches and don't see any problems. Our findings seem to be roughly the same. i.e the datumCopy is mostly useless. However, you've noticed the requirement to datumCopy when there are multiple window functions using the same window along with yours containing the call to MakeExpandedObjectReadOnly() where I missed that. This should also slightly improve the performance of LEAD and LAG with byref types, which seems like a good side-effect. I guess the commit message for 0002 should mention that for pointers to allocated chunks that GetMemoryChunkContext() can be used in place of MemoryContextContains(). I did see that PostGIS does use MemoryContextContains(), though I didn't look at their code to figure out if they're always passing it a pointer to an allocated chunk. Maybe it's worth doing; #define MemoryContextContains(c, p) (GetMemoryChunkContext(p) == (c)) in memutils.h? or are we better to force extension authors to re-evaluate their code in case anyone is passing memory that's not pointing directly to a palloc'd chunk? David
Re: Reducing the chunk header sizes on all memory context types
I wrote:
> I think what we should look at is extending the aggregate/window
> function APIs so that such functions can report where they put their
> output, and then we can nuke MemoryContextContains(), with the
> code code set up to assume that it has to copy if the called function
> didn't report anything. The existing FunctionCallInfo.resultinfo
> mechanism (cf. ReturnSetInfo for SRFs) is probably the right thing
> to pass the flag through.
After studying the existing usages of MemoryContextContains, I think
there is a better answer, which is to just nuke them.
As far as I can tell, the datumCopy steps associated with aggregate
finalfns are basically useless. They only serve to prevent
returning a pointer directly to the aggregate's transition value
(or, perhaps, to a portion thereof). But what's wrong with that?
It'll last as long as we need it to. Maybe there was a reason
back before we required finalfns to not scribble on the transition
values, but those days are gone.
The same goes for aggregate serialfns --- although there, I can't
avoid the feeling that the datumCopy step was just cargo-culted in.
I don't think there can exist a serialfn that doesn't return a
freshly-palloced bytea.
The one place where we actually need the conditional datumCopy is
with window functions, and even there I don't think we need it
in simple cases with only one window function. The case that is
hazardous is where multiple window functions are sharing a
WindowObject. So I'm content to optimize the single-window-function
case and just always copy if there's more than one. (Sadly, there
is no existing regression test that catches this, so I added one.)
See attached.
regards, tom lane
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index fe74e49814..45fcd37253 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1028,6 +1028,8 @@ process_ordered_aggregate_multi(AggState *aggstate,
*
* The finalfn will be run, and the result delivered, in the
* output-tuple context; caller's CurrentMemoryContext does not matter.
+ * (But note that in some cases, such as when there is no finalfn, the
+ * result might be a pointer to or into the agg's transition value.)
*
* The finalfn uses the state as set in the transno. This also might be
* being used by another aggregate function, so it's important that we do
@@ -1112,21 +1114,13 @@ finalize_aggregate(AggState *aggstate,
}
else
{
- /* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
- *resultVal = pergroupstate->transValue;
+ *resultVal =
+ MakeExpandedObjectReadOnly(pergroupstate->transValue,
+ pergroupstate->transValueIsNull,
+ pertrans->transtypeLen);
*resultIsNull = pergroupstate->transValueIsNull;
}
- /*
- * If result is pass-by-ref, make sure it is in the right context.
- */
- if (!peragg->resulttypeByVal && !*resultIsNull &&
- !MemoryContextContains(CurrentMemoryContext,
- DatumGetPointer(*resultVal)))
- *resultVal = datumCopy(*resultVal,
- peragg->resulttypeByVal,
- peragg->resulttypeLen);
-
MemoryContextSwitchTo(oldContext);
}
@@ -1176,19 +1170,13 @@ finalize_partialaggregate(AggState *aggstate,
}
else
{
- /* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
- *resultVal = pergroupstate->transValue;
+ *resultVal =
+ MakeExpandedObjectReadOnly(pergroupstate->transValue,
+ pergroupstate->transValueIsNull,
+ pertrans->transtypeLen);
*resultIsNull = pergroupstate->transValueIsNull;
}
- /* If result is pass-by-ref, make sure it is in the right context. */
- if (!peragg->resulttypeByVal && !*resultIsNull &&
- !MemoryContextContains(CurrentMemoryContext,
- DatumGetPointer(*resultVal)))
- *resultVal = datumCopy(*resultVal,
- peragg->resulttypeByVal,
- peragg->resulttypeLen);
-
MemoryContextSwitchTo(oldContext);
}
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 8b0858e9f5..ce860bceeb 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -630,20 +630,13 @@ finalize_windowaggregate(WindowAggState *winstate,
}
else
{
- /* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
- *result = peraggstate->transValue;
+ *result =
+ MakeExpandedObjectReadOnly(peraggstate->transValue,
+ peraggstate->transValueIsNull,
+ peraggstate->transtypeLen);
*isnull = peraggstate->transValueIsNull;
}
- /*
- * If result is pass-by-ref, make sure it is in the right context.
- */
- if (!peraggstate->resulttypeByVal && !*isnull &&
- !MemoryContextContains(CurrentMemoryContext,
- DatumGetPointer(*result)))
- *result = datumCopy(*result,
- peraggstate->resulttypeByVal,
- peraggstate->resulttypeLen);
MemoryContextSwitchTo(oldContext);
}
@@ -1057,13 +1050,14 @@
Re: Reducing the chunk header sizes on all memory context types
Em ter., 4 de out. de 2022 Ã s 08:29, Ranier Vilela escreveu: > Em ter., 4 de out. de 2022 Ã s 05:36, David Rowley > escreveu: > >> On Tue, 4 Oct 2022 at 13:35, Ranier Vilela wrote: >> > Revisiting my work on reducing memory consumption, I found this patch >> left out. >> > I'm not sure I can help. >> > But basically I was able to write and read the block size, in the chunk. >> > Could it be the case of writing and reading the context pointer in the >> same way? >> > Sure this leads to some performance loss, but would it make it possible >> to get the context pointer from the chunk? >> > In other words, would it be possible to save the context pointer and >> compare it later in MemoryContextContains? >> >> I'm not really sure I understand the intention of the patch. The >> header size for all our memory contexts was already reduced in >> c6e0fe1f2. The patch you sent seems to pre-date that commit. >> > There is zero intention to commit this. It's just an experiment I did. > > As it is in the patch, it is possible to save the context pointer outside > the header chunk. > Making it possible to retrieve it in MemoryContextContains. > Never mind, it's a waste of time. regards, Ranier Vilela
Re: Reducing the chunk header sizes on all memory context types
Em ter., 4 de out. de 2022 Ã s 05:36, David Rowley escreveu: > On Tue, 4 Oct 2022 at 13:35, Ranier Vilela wrote: > > Revisiting my work on reducing memory consumption, I found this patch > left out. > > I'm not sure I can help. > > But basically I was able to write and read the block size, in the chunk. > > Could it be the case of writing and reading the context pointer in the > same way? > > Sure this leads to some performance loss, but would it make it possible > to get the context pointer from the chunk? > > In other words, would it be possible to save the context pointer and > compare it later in MemoryContextContains? > > I'm not really sure I understand the intention of the patch. The > header size for all our memory contexts was already reduced in > c6e0fe1f2. The patch you sent seems to pre-date that commit. > There is zero intention to commit this. It's just an experiment I did. As it is in the patch, it is possible to save the context pointer outside the header chunk. Making it possible to retrieve it in MemoryContextContains. It's just an idea. regards, Ranier Vilela
Re: Reducing the chunk header sizes on all memory context types
On Tue, 4 Oct 2022 at 13:35, Ranier Vilela wrote: > Revisiting my work on reducing memory consumption, I found this patch left > out. > I'm not sure I can help. > But basically I was able to write and read the block size, in the chunk. > Could it be the case of writing and reading the context pointer in the same > way? > Sure this leads to some performance loss, but would it make it possible to > get the context pointer from the chunk? > In other words, would it be possible to save the context pointer and compare > it later in MemoryContextContains? I'm not really sure I understand the intention of the patch. The header size for all our memory contexts was already reduced in c6e0fe1f2. The patch you sent seems to pre-date that commit. David
Re: Reducing the chunk header sizes on all memory context types
On Thu, 29 Sept 2022 at 18:30, David Rowley wrote: > Does anyone have any opinions on this? Hi, Revisiting my work on reducing memory consumption, I found this patch left out. I'm not sure I can help. But basically I was able to write and read the block size, in the chunk. Could it be the case of writing and reading the context pointer in the same way? Sure this leads to some performance loss, but would it make it possible to get the context pointer from the chunk? In other words, would it be possible to save the context pointer and compare it later in MemoryContextContains? regards, Ranier Vilela 001-reduces-memory-consumption.patch Description: Binary data
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > Andres did mention to me off-list about perhaps adding a boolean field > to FunctionCallInfoBaseData to indicate if the return value can be > assumed to be in CurrentMemoryContext. I feel like that might be > quite a bit of work to go and change all functions to ensure that > that's properly populated. That seems like the right basic answer, but wrong in detail. We have only a tiny number of places that care about this --- aggregates and window functions basically --- and those already have a bunch of special calling conventions. So changing the generic fmgr APIs has side-effects far beyond what's justified. I think what we should look at is extending the aggregate/window function APIs so that such functions can report where they put their output, and then we can nuke MemoryContextContains(), with the code code set up to assume that it has to copy if the called function didn't report anything. The existing FunctionCallInfo.resultinfo mechanism (cf. ReturnSetInfo for SRFs) is probably the right thing to pass the flag through. I can take a look at this once the release dust settles a little. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Thu, 29 Sept 2022 at 18:30, David Rowley wrote: > Does anyone have any opinions on this? I by no means think I've nailed the fix in other_ideas_to_fix_MemoryContextContains.patch, so it would be good to see if anyone else has any new ideas on how to solve this issue. Andres did mention to me off-list about perhaps adding a boolean field to FunctionCallInfoBaseData to indicate if the return value can be assumed to be in CurrentMemoryContext. I feel like that might be quite a bit of work to go and change all functions to ensure that that's properly populated. For example, look at split_part() in varlena.c, it's going to be a little tedious to ensure we set that field correctly there as that function sometimes returns it's input, sometimes returns a string constant and sometimes allocates new memory. I feel fixing it this way will be error-prone and cause lots of future bugs. I'm also aware that the change made in b76fb6c2a becomes less temporary with each day that passes, so I really would like to find a solution to the MemoryContextContains issue. I'll reiterate that I don't think reverting c6e0fe1f2 fixes MemoryContextContains. That would just put back the behaviour of it returning true based on the owning MemoryContext and/or the direction that the wind is coming from on the particular day the function is called. Although I do think other_ideas_to_fix_MemoryContextContains.patch does fix the problem. I also fear a few people would be reaching for their pitchforks if I was to go and commit it. However, as of now, I'm starting to look more favourably at it as more time passes. David
Re: Reducing the chunk header sizes on all memory context types
On Tue, 27 Sept 2022 at 11:28, David Rowley wrote:
> Maybe we could remove the datumCopy() from eval_windowfunction() and
> also document that a window function when returning a non-byval type,
> must allocate the Datum in either ps_ExprContext's
> ecxt_per_tuple_memory or ecxt_per_query_memory. We could ensure any
> extension which has its own window functions get the memo about the
> API change by adding an Assert to ensure that the return value (for
> byref types) is in the current context by calling the
> loop-over-the-blocks version of MemoryContextContains().
I did some work on this and it turned out that the value returned by
any of lead(), lag(), first_value(), last_value() and nth_value()
could also be in MessageContext or some child context to
CacheMemoryContext. The reason for the latter two is that cases like
LAG(col, 1, 'default value') will return the Const in the 3rd arg when
the offset value is outside of the window frame. That means
MessageContext for normal queries and it means it'll be cached in a
child context of CacheMemoryContext for PREPAREd queries.
This means the Assert that I wanted to add to eval_windowfunction
became quite complex. Namely:
Assert(perfuncstate->resulttypeByVal || fcinfo->isnull ||
MemoryContextContains(winstate->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
(void *) *result) ||
MemoryContextContains(winstate->ss.ps.ps_ExprContext->ecxt_per_query_memory,
(void *) *result) ||
MemoryContextContains(MessageContext, (void *) *result) ||
MemoryContextOrChildOfContains(CacheMemoryContext, (void *) *result));
Notice the invention of MemoryContextOrChildOfContains() to
recursively search the CacheMemoryContext children. It does not seem
so great as CacheMemoryContext tends to have many children and
searching through them all could make that Assert a bit slow.
I think I am fairly happy that all the 4 message contexts I mentioned
in the Assert will be around long enough for the result value to not
be freed. It's just that the whole thing feels a bit wrong and that
the context the return value is in should be a bit more predictable.
Does anyone have any opinions on this?
David
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index fe74e49814..bc7eeaf84d 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1107,6 +1107,11 @@ finalize_aggregate(AggState *aggstate,
{
*resultVal = FunctionCallInvoke(fcinfo);
*resultIsNull = fcinfo->isnull;
+
+ /* ensure by-ref types are allocated in
CurrentMemoryContext */
+ Assert(peragg->resulttypeByVal || *resultIsNull ||
+ MemoryContextContains(CurrentMemoryContext,
(void *) *resultVal));
+
}
aggstate->curperagg = NULL;
}
@@ -1115,17 +1120,13 @@ finalize_aggregate(AggState *aggstate,
/* Don't need MakeExpandedObjectReadOnly; datumCopy will copy
it */
*resultVal = pergroupstate->transValue;
*resultIsNull = pergroupstate->transValueIsNull;
- }
- /*
-* If result is pass-by-ref, make sure it is in the right context.
-*/
- if (!peragg->resulttypeByVal && !*resultIsNull &&
- !MemoryContextContains(CurrentMemoryContext,
-
DatumGetPointer(*resultVal)))
- *resultVal = datumCopy(*resultVal,
-
peragg->resulttypeByVal,
-
peragg->resulttypeLen);
+ /* if result is pass-by-ref, copy into the CurrentMemoryContext
*/
+ if (!peragg->resulttypeByVal && !*resultIsNull)
+ *resultVal = datumCopy(*resultVal,
+
peragg->resulttypeByVal,
+
peragg->resulttypeLen);
+ }
MemoryContextSwitchTo(oldContext);
}
@@ -1172,6 +1173,11 @@ finalize_partialaggregate(AggState *aggstate,
*resultVal = FunctionCallInvoke(fcinfo);
*resultIsNull = fcinfo->isnull;
+
+ /* ensure by-ref types are allocated in
CurrentMemoryContext */
+ Assert(peragg->resulttypeByVal || *resultIsNull ||
+ MemoryContextContains(CurrentMemoryContext,
(void *) *resultVal));
+
}
}
else
@@ -1179,15 +1185,14 @@ finalize_partialaggregate(AggState *aggstate,
/* Don't need MakeExpandedObjectReadOnly; datumCopy will copy
it */
*resultVal = pergroupstate->transValue;
*resultIsNull = pergroupstate->transValueIsNull;
- }
- /* If result is pass-by-ref, make sure it
Re: Reducing the chunk header sizes on all memory context types
On Tue, 20 Sept 2022 at 13:23, Tom Lane wrote: > > David Rowley writes: > > Aside from that, I don't have any ideas on how to get rid of the > > possible additional datumCopy() from non-Var arguments to these window > > functions. Should we just suffer it? It's quite likely that most > > arguments to these functions are plain Vars anyway. > > No, we shouldn't. I'm pretty sure that we have various window > functions that are deliberately designed to take advantage of the > no-copy behavior, and that they have taken a significant speed > hit from your having disabled that optimization. I don't say > that this is enough to justify reverting the chunk header changes > altogether ... but I'm completely not satisfied with the current > situation in HEAD. Looking more closely at window_gettupleslot(), it always allocates the tuple in ecxt_per_query_memory, so any column we fetch out of that tuple will be in that memory context. window_gettupleslot() is used in lead(), lag(), first_value(), last_value() and nth_value() to fetch the Nth tuple out of the partition window. The other window functions all return BIGINT, FLOAT8 or INT which are byval on 64-bit, and on 32-bit these functions return a freshly palloc'd Datum in the CurrentMemoryContext. Maybe we could remove the datumCopy() from eval_windowfunction() and also document that a window function when returning a non-byval type, must allocate the Datum in either ps_ExprContext's ecxt_per_tuple_memory or ecxt_per_query_memory. We could ensure any extension which has its own window functions get the memo about the API change by adding an Assert to ensure that the return value (for byref types) is in the current context by calling the loop-over-the-blocks version of MemoryContextContains(). This would mean that wfuncs like lead(column_name) would no longer do that extra datumCopy and the likes of lead(col || 'some OpExpr') would save a little as we'd no longer call MemoryContextContains on non-Assert builds. David
Re: Reducing the chunk header sizes on all memory context types
On Tue, 20 Sept 2022 at 17:23, Tom Lane wrote: > "Broken" is a strong claim. There's reason to think it could fail > in the back branches, but little evidence that it actually has failed > in the field. I've posted some code to the security list that shows that I can get MemoryContextContains() to return true when it should return false. This results in the datumCopy() in eval_windowfunction() being skipped and the result of the window function being left in the incorrect memory context. I was unable to get this to produce a crash, but if there's some way to have the result point into a shared buffer page and that page is evicted and replaced with something else before the value is used then we'd have issues. > So yeah, we have work to do --- which is the exact > opposite of your apparent stand that we can walk away from the > problem. My problem is that I'm unable to think of a way to fix something I see as an existing bug. I've given it a week and nobody else has come forward with any proposals on how to fix this. I'm very open to finding some way to allow us to keep this optimisation, but so far I've been unable to. We have reverted broken optimisations before. Also, reverting c6e0fe1f2a does not seem that appealing to me as it just returns MemoryContextContains() back into a state where it can return false positives again. David
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > On Tue, 20 Sept 2022 at 13:23, Tom Lane wrote: >> ... but I'm completely not satisfied with the current >> situation in HEAD. > Maybe you've forgotten that MemoryContextContains() is broken in the > back branches or just don't think it is broken? "Broken" is a strong claim. There's reason to think it could fail in the back branches, but little evidence that it actually has failed in the field. So yeah, we have work to do --- which is the exact opposite of your apparent stand that we can walk away from the problem. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 20 Sept 2022 at 13:23, Tom Lane wrote: > > David Rowley writes: > > Aside from that, I don't have any ideas on how to get rid of the > > possible additional datumCopy() from non-Var arguments to these window > > functions. Should we just suffer it? It's quite likely that most > > arguments to these functions are plain Vars anyway. > > No, we shouldn't. I'm pretty sure that we have various window > functions that are deliberately designed to take advantage of the > no-copy behavior, and that they have taken a significant speed > hit from your having disabled that optimization. I don't say > that this is enough to justify reverting the chunk header changes > altogether ... but I'm completely not satisfied with the current > situation in HEAD. Maybe you've forgotten that MemoryContextContains() is broken in the back branches or just don't think it is broken? David
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > Aside from that, I don't have any ideas on how to get rid of the > possible additional datumCopy() from non-Var arguments to these window > functions. Should we just suffer it? It's quite likely that most > arguments to these functions are plain Vars anyway. No, we shouldn't. I'm pretty sure that we have various window functions that are deliberately designed to take advantage of the no-copy behavior, and that they have taken a significant speed hit from your having disabled that optimization. I don't say that this is enough to justify reverting the chunk header changes altogether ... but I'm completely not satisfied with the current situation in HEAD. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 13 Sept 2022 at 20:27, David Rowley wrote: > I see that one of the drawbacks from not using MemoryContextContains() > is that window functions such as lead(), lag(), first_value(), > last_value() and nth_value() may now do the datumCopy() when it's not > needed. For example, with a window function call such as > lead(byref_col ), the expression evaluation code in > WinGetFuncArgInPartition() will just return the address in the > tuplestore tuple for "byref_col". The datumCopy() is needed for that. > However, if the function call was lead(byref_col || 'something') then > we'd have ended up with a new allocation in CurrentMemoryContext to > concatenate the two values. We'll now do a datumCopy() where we > previously wouldn't have. I don't really see any way around that > without doing some highly invasive surgery to the expression > evaluation code. It feels like a terrible idea, but I wondered if we could look at the WindowFunc->args and make a decision if we should do the datumCopy() based on the type of the argument. Vars would need to be copied as they will point into the tuple's memory, but an OpExpr likely would not need to be copied. Aside from that, I don't have any ideas on how to get rid of the possible additional datumCopy() from non-Var arguments to these window functions. Should we just suffer it? It's quite likely that most arguments to these functions are plain Vars anyway. David
Re: Reducing the chunk header sizes on all memory context types
On Fri, 9 Sept 2022 at 11:33, David Rowley wrote:
> I really think the Assert only form of MemoryContextContains() is the
> best move, and if it's doing Assert only, then we can do the
> loop-over-the-blocks idea as you described and I drafted in [1].
>
> If the need comes up that we're certain we always have a pointer to
> some allocated chunk, but need to know if it's in some memory context,
> then the proper form of expressing that, I think, should be:
>
> if (GetMemoryChunkContext(pointer) == somecontext)
>
> If we're worried about getting that wrong, we can beef up the
> MemoryChunk struct with a magic_number field in
> MEMORY_CONTEXT_CHECKING builds to ensure we catch any code which
> passes invalid pointers.
I've attached a patch series which is my proposal on what we should do
about MemoryContextContains. In summary, this basically changes
MemoryContextContains() so it's only available in
MEMORY_CONTEXT_CHECKING builds and removes 4 current usages of the
function.
0001: Makes MemoryContextContains work again with the
loop-over-the-blocks method mentioned by Tom.
0002: Adds a new "chunk_magic" field to MemoryChunk. My thoughts are
that it might be a good idea to do this so that we get Assert failures
if we use functions like GetMemoryChunkContext() on a pointer that's
not a MemoryChunk.
0003: This adjusts aggregate final functions and window functions so
that any byref Datum they return is allocated in CurrentMemoryContext
0004: Makes MemoryContextContains only available in
MEMORY_CONTEXT_CHECKING builds and adjusts our usages of the function
to use GetMemoryChunkContext() instead.
An alternative to 0004, would be more along the lines of what was
mentioned by Andres and just Assert that the returned value is in the
memory context that we expect. I don't think we need to do anything
special with aggregates that take an internal state. I think the rule
is just as simple as; all final functions and window functions must
return any byref values in CurrentMemoryContext. For aggregates
without a finalfn, we can just datumCopy() the returned byref value.
There's no chance for those to be in CurrentMemoryContext anyway. The
return value must be in the aggregate state's context. The attached
assert.patch shows that this holds true in make check after applying
each of the other patches.
I see that one of the drawbacks from not using MemoryContextContains()
is that window functions such as lead(), lag(), first_value(),
last_value() and nth_value() may now do the datumCopy() when it's not
needed. For example, with a window function call such as
lead(byref_col ), the expression evaluation code in
WinGetFuncArgInPartition() will just return the address in the
tuplestore tuple for "byref_col". The datumCopy() is needed for that.
However, if the function call was lead(byref_col || 'something') then
we'd have ended up with a new allocation in CurrentMemoryContext to
concatenate the two values. We'll now do a datumCopy() where we
previously wouldn't have. I don't really see any way around that
without doing some highly invasive surgery to the expression
evaluation code.
None of the attached patches are polished. I can do that once we agree
on the best way to fix the issue.
David
From a9fcf078bc8a87742860abee527199f772f39f5b Mon Sep 17 00:00:00 2001
From: David Rowley
Date: Thu, 8 Sep 2022 23:32:03 +1200
Subject: [PATCH v1 1/4] Reinstate MemoryContextContains to work with arbitrary
pointers
---
src/backend/utils/mmgr/aset.c| 42 ++
src/backend/utils/mmgr/generation.c | 45 +++
src/backend/utils/mmgr/mcxt.c| 56 ++--
src/backend/utils/mmgr/slab.c| 44 +++
src/include/nodes/memnodes.h | 4 +-
src/include/utils/memutils_internal.h| 3 ++
src/include/utils/memutils_memorychunk.h | 21 +
7 files changed, 191 insertions(+), 24 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec423375ae..24ab7399a3 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1333,6 +1333,48 @@ AllocSetGetChunkContext(void *pointer)
return >header;
}
+/*
+ * AllocSetContains
+ * Attempt to determine if 'pointer' is memory which was allocated
by
+ * 'context'. Return true if it is, otherwise false.
+ */
+bool
+AllocSetContains(MemoryContext context, void *pointer)
+{
+ MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
+ AllocBlock block;
+ AllocSetset = (AllocSet) context;
+
+ /*
+* We must use MemoryChunkIsExternalUnSafePointer() instead of
+* MemoryChunkIsExternal() here as 'chunk' may not be a MemoryChunk if
+* 'pointer' is not pointing to palloced memory. Below we're careful
+* never to dereference 'block' as it could point to memory not owned by
+* this process.
+*/
+ if
Re: Reducing the chunk header sizes on all memory context types
On Fri, 9 Sept 2022 at 10:53, Tom Lane wrote: > I hate to give up MemoryContextContains altogether. The assertions > that David nuked in b76fb6c2a had some value I think, Those can be put back if we decide to keep MemoryContextContains. Those newly added Asserts just temporarily had to go due to b76fb6c2a making MemoryContextContains temporarily always return false. > The implementation I suggested upthread would reliably distinguish > malloc from palloc, and while it is potentially a tad expensive > I don't think it's too much so for Assert checks. I don't have an > objection to trying to get to a place where we only use it in > Assert, though. I really think the Assert only form of MemoryContextContains() is the best move, and if it's doing Assert only, then we can do the loop-over-the-blocks idea as you described and I drafted in [1]. If the need comes up that we're certain we always have a pointer to some allocated chunk, but need to know if it's in some memory context, then the proper form of expressing that, I think, should be: if (GetMemoryChunkContext(pointer) == somecontext) If we're worried about getting that wrong, we can beef up the MemoryChunk struct with a magic_number field in MEMORY_CONTEXT_CHECKING builds to ensure we catch any code which passes invalid pointers. David [1] https://www.postgresql.org/message-id/CAApHDvoKjOmPQeokicwDuO-_Edh=tKp23-=jskycykfw5qu...@mail.gmail.com
Re: Reducing the chunk header sizes on all memory context types
Andres Freund writes: > On 2022-09-08 14:10:36 -0400, Tom Lane wrote: >> No, I don't think we can get away with that. See int8inc() for a >> counterexample. > What I was suggesting a bit below the bit quoted above, was that we'd copy > whenever there's no finalfunc or if the finalfunc doesn't take an internal > parameter. Hmm, OK, I was confusing this with the optimization for transition functions; but that one is looking for pointer equality rather than checking MemoryContextContains. So maybe this'd work. > This business with interpreting random memory as a palloc'd chunk seems like a > fundamentally wrong approach worth incurring some pain to fix. I hate to give up MemoryContextContains altogether. The assertions that David nuked in b76fb6c2a had some value I think, and I was hoping to address your concerns in [1] by adding Assert(MemoryContextContains()) to guc_free. But I'm not sure how much that'll help to diagnose you- malloced-instead-of-pallocing if the result is not an assertion failure but a segfault in a not-obviously-related place. The failure at guc_free is already going to be some distance from the scene of the crime. The implementation I suggested upthread would reliably distinguish malloc from palloc, and while it is potentially a tad expensive I don't think it's too much so for Assert checks. I don't have an objection to trying to get to a place where we only use it in Assert, though. regards, tom lane [1] https://www.postgresql.org/message-id/20220905233233.jhcu5jqsrtosmgh5%40awork3.anarazel.de
Re: Reducing the chunk header sizes on all memory context types
Hi, On 2022-09-08 14:10:36 -0400, Tom Lane wrote: > Andres Freund writes: > > If there is a finalfunc, they're typically going to return data from within > > the current memory context, but could legitimately also return part of the > > data from curaggcontext. Perhaps we could forbid that? > > No, I don't think we can get away with that. See int8inc() for a > counterexample. What I was suggesting a bit below the bit quoted above, was that we'd copy whenever there's no finalfunc or if the finalfunc doesn't take an internal parameter. And that finalfuncs returning byref with an internal parameter can be expected to return memory allocated in the right context (which we of course could check with an assert). It's not super satisfying - but I don't think it'd have the problem you describe above. Alternatively we could add a column to pg_aggregate denoting this. That'd only be permissible to set for a superuser presumably. This business with interpreting random memory as a palloc'd chunk seems like a fundamentally wrong approach worth incurring some pain to fix. Greetings, Andres Freund
Re: Reducing the chunk header sizes on all memory context types
Andres Freund writes: > If there is a finalfunc, they're typically going to return data from within > the current memory context, but could legitimately also return part of the > data from curaggcontext. Perhaps we could forbid that? No, I don't think we can get away with that. See int8inc() for a counterexample. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
Hi, On 2022-09-07 11:08:27 -0400, Tom Lane wrote: > > I'll need to think about how best to fix this. In the meantime, I > > think the other 32-bit animals are probably not going to like this > > either :-( > > Yeah. The basic problem here is that we've greatly reduced the amount > of redundancy in chunk headers. Even with the prior amount of redundancy it's quite scary. It's one thing if the only consequence is a bit of added overhead - but if we *don't* copy the tuple due to a false positive we're in trouble. Afaict the user would have some control over the memory contents and so an attacker could work on triggering this issue. MemoryContextContains() may be ok for an assertion or such, but relying on it for correctness seems a bad idea. I wonder if we can get away from needing these checks, without unnecessarily copying datums every time: If there is no finalfunc, we know that the tuple ought to be in curaggcontext or such, and we need to copy. If there is a finalfunc, they're typically going to return data from within the current memory context, but could legitimately also return part of the data from curaggcontext. Perhaps we could forbid that? Our docs already say the following for serialization funcs: The result of the deserialization function should simply be allocated in the current memory context, as unlike the combine function's result, it is not long-lived. Perhaps we could institute a similar rule for finalfuncs? The argument against that is that we can use arbitrary functions as finalfuncs currently. Perhaps we could treat taking an internal argument as opting into the requirement and default to copying otherwise? Greetings, Andres Freund
Re: Reducing the chunk header sizes on all memory context types
On Thu, 8 Sept 2022 at 09:32, David Rowley wrote:
>
> On Thu, 8 Sept 2022 at 03:08, Tom Lane wrote:
> > Step 4 is annoyingly expensive, but perhaps not too awful given
> > the way we step up alloc block sizes. We should make sure that
> > any context we want to use MemoryContextContains with is allowed
> > to let its blocks grow large, so that there can't be too many
> > of them.
>
> I'll go code up your idea and see if doing that triggers any other ideas.
I've attached a very much draft grade patch for this. I have a couple
of thoughts:
1. I should remove all the Assert(MemoryContextContains(context,
ret)); I littered around mcxt.c. This function is not as cheap as it
once was and I'm expecting that Assert to be a bit too expensive now.
2. I changed the header comment in MemoryContextContains again, but I
removed the part about false positives since I don't believe that is
possible now. What I do think is just as possible as it was before is
a segfault. We're still accessing the 8 bytes prior to the given
pointer and there's a decent chance that would segfault when working
with a pointer which was returned by malloc. I imagine I'm not the
only C programmer around that dislikes writing comments along the
lines of "this might segfault, but..."
3. For external chunks, I'd coded MemoryChunk to put a magic number in
the 60 free bits of the hdrmask. Since we still need to call
MemoryChunkIsExternal on the given pointer, that function will Assert
that the magic number matches if the external chunk bit is set. We
can't expect that magic number check to pass when the external bit
just happens to be on because it's not a MemoryChunk we're looking at.
For now I commented out those Asserts to make the tests pass. Not sure
what's best there, maybe another version of MemoryChunkIsExternal or
export the underlying macro. I'm currently more focused on what I
wrote in #2.
David
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index ec423375ae..841d6cda5d 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1333,6 +1333,46 @@ AllocSetGetChunkContext(void *pointer)
return >header;
}
+/*
+ * AllocSetContains
+ * Attempt to determine if 'pointer' is memory which was allocated
by
+ * 'context'. Return true if it is, otherwise false.
+ */
+bool
+AllocSetContains(MemoryContext context, void *pointer)
+{
+ MemoryChunk *chunk = PointerGetMemoryChunk(pointer);
+ AllocBlock block;
+ AllocSetset = (AllocSet) context;
+
+ /*
+* Careful not to dereference anything in 'block' as if 'pointer' is not
+* a pointer to an allocated chunk then 'block' could be pointing to
about
+* anything.
+*/
+ if (MemoryChunkIsExternal(chunk))
+ block = ExternalChunkGetBlock(chunk);
+ else
+ block = (AllocBlock) MemoryChunkGetBlock(chunk);
+
+ for (AllocBlock blk = set->blocks; blk != NULL; blk = blk->next)
+ {
+ if (block == blk)
+ {
+ /*
+* The block matches. Now check if 'pointer' falls
within the
+* block's memory. We don't detect if the pointer is
pointing to
+* an allocated chunk as that would require looping
over the
+* freelist for this chunk's size.
+*/
+ if ((char *) pointer >= (char *) blk + ALLOC_BLOCKHDRSZ
+ ALLOC_CHUNKHDRSZ &&
+ (char *) pointer < blk->endptr)
+ return true;
+ }
+ }
+ return false;
+}
+
/*
* AllocSetGetChunkSpace
* Given a currently-allocated chunk, determine the total space
diff --git a/src/backend/utils/mmgr/generation.c
b/src/backend/utils/mmgr/generation.c
index c743b24fa7..653fa08064 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -848,6 +848,49 @@ GenerationGetChunkContext(void *pointer)
return >context->header;
}
+/*
+ * GenerationContains
+ * Attempt to determine if 'pointer' is memory which was allocated
by
+ * 'context'. Return true if it is, otherwise false.
+ */
+bool
+GenerationContains(MemoryContext context, void *pointer)
+{
+ MemoryChunk*chunk = PointerGetMemoryChunk(pointer);
+ GenerationBlock*block;
+ GenerationContext *set = (GenerationContext *) context;
+ dlist_iter iter;
+
+ /*
+* Careful not to dereference anything in 'block' as if 'pointer' is not
+* a pointer to an allocated chunk then 'block' could be pointing to
about
+* anything.
+*/
+ if (MemoryChunkIsExternal(chunk))
+ block = ExternalChunkGetBlock(chunk);
+ else
+ block = (GenerationBlock *)
Re: Reducing the chunk header sizes on all memory context types
On Thu, 8 Sept 2022 at 03:08, Tom Lane wrote: > 4. For aset.c, I'd be inclined to have it compute the AllocBlock > address implied by the putative chunk header, and then run through > the context's alloc blocks and see if any of them match. If we > do find a match, and the chunk address is within the allocated > length of the block, call it good. Probably the same could be done > for the other two methods. > > Step 4 is annoyingly expensive, but perhaps not too awful given > the way we step up alloc block sizes. We should make sure that > any context we want to use MemoryContextContains with is allowed > to let its blocks grow large, so that there can't be too many > of them. Thanks for the idea. I've not come up with anything better other than remove the calls to MemoryContextContains and just copy the Datum each time. That doesn't fix the problems with function, however. I'll go code up your idea and see if doing that triggers any other ideas. David
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > Looks like my analysis wasn't that good in nodeWindowAgg.c. The > reason it's crashing is due to int2int4_sum() returning > Int64GetDatumFast(transdata->sum). Ugh. I thought for a bit about whether we could define that as wrong, but it's returning a portion of its input, which seems kosher enough (not much different from array subscripting, for instance). > I'll need to think about how best to fix this. In the meantime, I > think the other 32-bit animals are probably not going to like this > either :-( Yeah. The basic problem here is that we've greatly reduced the amount of redundancy in chunk headers. Perhaps we need to proceed about like this: 1. Check the provided pointer is non-null and maxaligned (if not, return false). 2. Pull out the mcxt type bits and check that they match the type of the provided context. 3. If 1 and 2 pass, it's safe enough to call a context-specific check routine. 4. For aset.c, I'd be inclined to have it compute the AllocBlock address implied by the putative chunk header, and then run through the context's alloc blocks and see if any of them match. If we do find a match, and the chunk address is within the allocated length of the block, call it good. Probably the same could be done for the other two methods. Step 4 is annoyingly expensive, but perhaps not too awful given the way we step up alloc block sizes. We should make sure that any context we want to use MemoryContextContains with is allowed to let its blocks grow large, so that there can't be too many of them. I don't see a way to do better if we're afraid to dereference the computed AllocBlock address. BTW, if we do it this way, what we'd actually be guaranteeing is that the address is within some alloc block belonging to the context; it wouldn't quite prove that the address corresponds to a currently-allocated chunk. That'd be good enough probably for the important use-cases. In particular it'd be 100% correct at rejecting chunks of other contexts and chunks gotten from raw malloc(). regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Thu, 8 Sept 2022 at 01:22, David Rowley wrote:
>
> On Thu, 8 Sept 2022 at 01:05, Julien Rouhaud wrote:
> > FYI lapwing isn't happy with this patch:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing=2022-09-07%2012%3A40%3A16.
>
> I'll look into it further.
Looks like my analysis wasn't that good in nodeWindowAgg.c. The
reason it's crashing is due to int2int4_sum() returning
Int64GetDatumFast(transdata->sum). For 64-bit machines,
Int64GetDatumFast() translates to Int64GetDatum() and and that's
byval, so the MemoryContextContains() call is not triggered, but on
32-bit machines that's PointerGetDatum() and a byref type, and we're
returning a pointer to transdata->sum, which is part way into an
allocation.
Funnily, the struct looks like:
typedef struct Int8TransTypeData
{
int64 count;
int64 sum;
} Int8TransTypeData;
so the previous version of MemoryContextContains() would have
subtracted sizeof(void *) from >sum which, on this 32-bit
machine would have pointed halfway up the "count" field. That count
field seems like it would be a good candidate for the "false positive"
that the previous comment in MemoryContextContains mentioned about. So
it looks like it had about a 1 in 2^32 odds of doing the wrong thing
before.
Had the fields in that struct happened to be in the opposite order,
then I don't think it would have crashed, but that's certainly no fix.
I'll need to think about how best to fix this. In the meantime, I
think the other 32-bit animals are probably not going to like this
either :-(
David
Re: Reducing the chunk header sizes on all memory context types
On Thu, 8 Sept 2022 at 01:05, Julien Rouhaud wrote: > FYI lapwing isn't happy with this patch: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing=2022-09-07%2012%3A40%3A16. Thanks. It does seem to be because of the nodeWindowAgg.c usage of MemoryContextContains. I'll look into it further. David
Re: Reducing the chunk header sizes on all memory context types
Hi, On Thu, Sep 08, 2022 at 12:29:22AM +1200, David Rowley wrote: > > I spent some time looking at our existing usages of > MemoryContextContains() to satisfy myself that we'll only ever pass in > a pointer to memory allocated by a MemoryContext and pushed this > patch. FYI lapwing isn't happy with this patch: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing=2022-09-07%2012%3A40%3A16.
Re: Reducing the chunk header sizes on all memory context types
On Tue, 6 Sept 2022 at 15:17, David Rowley wrote: > > On Tue, 6 Sept 2022 at 14:43, Tom Lane wrote: > > I think MemoryContextContains' charter is to return > > > > GetMemoryChunkContext(pointer) == context > > > > *except* that instead of asserting what GetMemoryChunkContext asserts, > > it should treat those cases as reasons to return false. So if you > > can still do GetMemoryChunkContext then you can still do > > MemoryContextContains. The point of having the separate function > > is to be as forgiving as we can of bogus pointers. > > Ok. I've readded the Asserts that c6e0fe1f2 mistakenly removed from > GetMemoryChunkContext() and changed MemoryContextContains() to do > those same pre-checks before calling GetMemoryChunkContext(). > > I've also boosted the Assert in mcxt.c to > Assert(MemoryContextContains(context, ret)) in each place we call the > context's callback function to obtain a newly allocated pointer. I > think this should cover the testing. > > I felt the need to keep the adjustments I made to the header comment > in MemoryContextContains() to ward off anyone who thinks it's ok to > pass this any random pointer and have it do something sane. It's much > more prone to misbehaving/segfaulting now given the extra dereferences > that c6e0fe1f2 added to obtain a pointer to the owning context. I spent some time looking at our existing usages of MemoryContextContains() to satisfy myself that we'll only ever pass in a pointer to memory allocated by a MemoryContext and pushed this patch. I put some notes in the commit message about it being unsafe now to pass in arbitrary pointers to MemoryContextContains(). Just a note to the archives that I'd personally feel much better if we just removed this function in favour of using GetMemoryChunkContext() instead. That would force extension authors using MemoryContextContains() to rewrite and revalidate their code. I feel that it's unlikely anyone will notice until something crashes otherwise. Hopefully that'll happen before their extension is released. David
Re: Reducing the chunk header sizes on all memory context types
В Ср, 07/09/2022 в 16:13 +1200, David Rowley пишет: > On Tue, 6 Sept 2022 at 01:41, David Rowley wrote: > > > > On Fri, 2 Sept 2022 at 20:11, David Rowley wrote: > > > > > > On Thu, 1 Sept 2022 at 12:46, Tom Lane wrote: > > > > > > > > David Rowley writes: > > > > > Maybe we should just consider always making room for a sentinel for > > > > > chunks that are on dedicated blocks. At most that's an extra 8 bytes > > > > > in some allocation that's either over 1024 or 8192 (depending on > > > > > maxBlockSize). > > > > > > > > Agreed, if we're not doing that already then we should. > > > > > > Here's a patch to that effect. > > > > If there are no objections, then I plan to push that patch soon. > > I've now pushed the patch which adds the sentinel space in more cases. > > The final analysis I did on the stats gathered during make > installcheck show that we'll now allocate about 19MBs more over the > entire installcheck run out of about 26GBs total allocations. > > That analysis looks something like: > > Before: > > SELECT CASE > WHEN pow2_size > 0 >  AND pow2_size = size THEN 'No' > WHEN pow2_size = 0 >  AND size = maxalign_size THEN 'No' > ELSE 'Yes' >   END   AS has_sentinel, >   Count(*)  AS n_allocations, >   Sum(CASE > WHEN pow2_size > 0 THEN pow2_size > ELSE maxalign_size >   END) / 1024 / 1024 mega_bytes_alloc > FROM  memstats > GROUP BY 1; > has_sentinel | n_allocations | mega_bytes_alloc > --+---+-- >  No  | 26445855 |   21556 >  Yes | 37602052 | 5044 > > After: > > SELECT CASE > WHEN pow2_size > 0 >  AND pow2_size = size THEN 'No' > WHEN pow2_size = 0 >  AND size = maxalign_size THEN 'Yes' -- this part changed > ELSE 'Yes' >   END   AS has_sentinel, >   Count(*)  AS n_allocations, >   Sum(CASE > WHEN pow2_size > 0 THEN pow2_size > WHEN size = maxalign_size THEN maxalign_size + 8 > ELSE maxalign_size >   END) / 1024 / 1024 mega_bytes_alloc > FROM  memstats > GROUP BY 1; > has_sentinel | n_allocations | mega_bytes_alloc > --+---+-- >  No  | 23980527 | 2177 >  Yes | 40067380 |   24442 > > That amounts to previously having about 58.7% of allocations having a > sentinel up to 62.6% currently, during the installcheck run. > > It seems a pretty large portion of allocation request sizes are > power-of-2 sized and use AllocSet. 19MB over 26GB is almost nothing. If it is only for enable-casserts builds, then it is perfectly acceptable. regards Yura
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > It seems a pretty large portion of allocation request sizes are > power-of-2 sized and use AllocSet. No surprise there, we've been programming with aset.c's behavior in mind for ~20 years ... regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 6 Sept 2022 at 01:41, David Rowley wrote: > > On Fri, 2 Sept 2022 at 20:11, David Rowley wrote: > > > > On Thu, 1 Sept 2022 at 12:46, Tom Lane wrote: > > > > > > David Rowley writes: > > > > Maybe we should just consider always making room for a sentinel for > > > > chunks that are on dedicated blocks. At most that's an extra 8 bytes > > > > in some allocation that's either over 1024 or 8192 (depending on > > > > maxBlockSize). > > > > > > Agreed, if we're not doing that already then we should. > > > > Here's a patch to that effect. > > If there are no objections, then I plan to push that patch soon. I've now pushed the patch which adds the sentinel space in more cases. The final analysis I did on the stats gathered during make installcheck show that we'll now allocate about 19MBs more over the entire installcheck run out of about 26GBs total allocations. That analysis looks something like: Before: SELECT CASE WHEN pow2_size > 0 AND pow2_size = size THEN 'No' WHEN pow2_size = 0 AND size = maxalign_size THEN 'No' ELSE 'Yes' ENDAS has_sentinel, Count(*) AS n_allocations, Sum(CASE WHEN pow2_size > 0 THEN pow2_size ELSE maxalign_size END) / 1024 / 1024 mega_bytes_alloc FROM memstats GROUP BY 1; has_sentinel | n_allocations | mega_bytes_alloc --+---+-- No | 26445855 |21556 Yes | 37602052 | 5044 After: SELECT CASE WHEN pow2_size > 0 AND pow2_size = size THEN 'No' WHEN pow2_size = 0 AND size = maxalign_size THEN 'Yes' -- this part changed ELSE 'Yes' ENDAS has_sentinel, Count(*) AS n_allocations, Sum(CASE WHEN pow2_size > 0 THEN pow2_size WHEN size = maxalign_size THEN maxalign_size + 8 ELSE maxalign_size END) / 1024 / 1024 mega_bytes_alloc FROM memstats GROUP BY 1; has_sentinel | n_allocations | mega_bytes_alloc --+---+-- No | 23980527 | 2177 Yes | 40067380 |24442 That amounts to previously having about 58.7% of allocations having a sentinel up to 62.6% currently, during the installcheck run. It seems a pretty large portion of allocation request sizes are power-of-2 sized and use AllocSet. David
Re: Reducing the chunk header sizes on all memory context types
On Tue, 6 Sept 2022 at 14:43, Tom Lane wrote:
> I think MemoryContextContains' charter is to return
>
> GetMemoryChunkContext(pointer) == context
>
> *except* that instead of asserting what GetMemoryChunkContext asserts,
> it should treat those cases as reasons to return false. So if you
> can still do GetMemoryChunkContext then you can still do
> MemoryContextContains. The point of having the separate function
> is to be as forgiving as we can of bogus pointers.
Ok. I've readded the Asserts that c6e0fe1f2 mistakenly removed from
GetMemoryChunkContext() and changed MemoryContextContains() to do
those same pre-checks before calling GetMemoryChunkContext().
I've also boosted the Assert in mcxt.c to
Assert(MemoryContextContains(context, ret)) in each place we call the
context's callback function to obtain a newly allocated pointer. I
think this should cover the testing.
I felt the need to keep the adjustments I made to the header comment
in MemoryContextContains() to ward off anyone who thinks it's ok to
pass this any random pointer and have it do something sane. It's much
more prone to misbehaving/segfaulting now given the extra dereferences
that c6e0fe1f2 added to obtain a pointer to the owning context.
David
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 64bcc7ef32..3d80abbfae 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -482,6 +482,15 @@ MemoryContextAllowInCriticalSection(MemoryContext context,
bool allow)
MemoryContext
GetMemoryChunkContext(void *pointer)
{
+ /*
+* Try to detect bogus pointers handed to us, poorly though we can.
+* Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
+* allocated chunk.
+*/
+ Assert(pointer != NULL);
+ Assert(pointer == (void *) MAXALIGN(pointer));
+ /* adding further Asserts here? See pre-checks in MemoryContextContains
*/
+
return MCXT_METHOD(pointer, get_chunk_context) (pointer);
}
@@ -809,11 +818,10 @@ MemoryContextCheck(MemoryContext context)
* Detect whether an allocated chunk of memory belongs to a given
* context or not.
*
- * Caution: this test is reliable as long as 'pointer' does point to
- * a chunk of memory allocated from *some* context. If 'pointer' points
- * at memory obtained in some other way, there is a small chance of a
- * false-positive result, since the bits right before it might look like
- * a valid chunk header by chance.
+ * Caution: 'pointer' must point to a pointer which was allocated by a
+ * MemoryContext. It's not safe or valid to use this function on arbitrary
+ * pointers as obtaining the MemoryContext which 'pointer' belongs to requires
+ * possibly several pointer dereferences.
*/
bool
MemoryContextContains(MemoryContext context, void *pointer)
@@ -821,9 +829,8 @@ MemoryContextContains(MemoryContext context, void *pointer)
MemoryContext ptr_context;
/*
-* NB: Can't use GetMemoryChunkContext() here - that performs assertions
-* that aren't acceptable here since we might be passed memory not
-* allocated by any memory context.
+* NB: We must perform run-time checks here which
GetMemoryChunkContext()
+* does as assertions before calling GetMemoryChunkContext().
*
* Try to detect bogus pointers handed to us, poorly though we can.
* Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
@@ -835,7 +842,7 @@ MemoryContextContains(MemoryContext context, void *pointer)
/*
* OK, it's probably safe to look at the context.
*/
- ptr_context = *(MemoryContext *) (((char *) pointer) - sizeof(void *));
+ ptr_context = GetMemoryChunkContext(pointer);
return ptr_context == context;
}
@@ -953,6 +960,8 @@ MemoryContextAlloc(MemoryContext context, Size size)
VALGRIND_MEMPOOL_ALLOC(context, ret, size);
+ Assert(MemoryContextContains(context, ret));
+
return ret;
}
@@ -991,6 +1000,8 @@ MemoryContextAllocZero(MemoryContext context, Size size)
MemSetAligned(ret, 0, size);
+ Assert(MemoryContextContains(context, ret));
+
return ret;
}
@@ -1029,6 +1040,8 @@ MemoryContextAllocZeroAligned(MemoryContext context, Size
size)
MemSetLoop(ret, 0, size);
+ Assert(MemoryContextContains(context, ret));
+
return ret;
}
@@ -1070,6 +1083,8 @@ MemoryContextAllocExtended(MemoryContext context, Size
size, int flags)
if ((flags & MCXT_ALLOC_ZERO) != 0)
MemSetAligned(ret, 0, size);
+ Assert(MemoryContextContains(context, ret));
+
return ret;
}
@@ -1153,6 +1168,8 @@ palloc(Size size)
VALGRIND_MEMPOOL_ALLOC(context, ret, size);
+ Assert(MemoryContextContains(context, ret));
+
return ret;
}
@@ -1186,6 +1203,8 @@ palloc0(Size size)
MemSetAligned(ret, 0, size);
+
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > I think the fix is harder than I thought, or perhaps impossible to do > given how we now determine the owning MemoryContext of a pointer. > There's a comment in MemoryContextContains which says: > * NB: Can't use GetMemoryChunkContext() here - that performs assertions > * that aren't acceptable here since we might be passed memory not > * allocated by any memory context. I think MemoryContextContains' charter is to return GetMemoryChunkContext(pointer) == context *except* that instead of asserting what GetMemoryChunkContext asserts, it should treat those cases as reasons to return false. So if you can still do GetMemoryChunkContext then you can still do MemoryContextContains. The point of having the separate function is to be as forgiving as we can of bogus pointers. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 6 Sept 2022 at 14:32, David Rowley wrote: > I wonder if there are many usages of MemoryContextContains in > extensions. If there's not, I'd be much happier if we got rid of this > function and used GetMemoryChunkContext() in nodeAgg.c and > nodeWindowAgg.c. I see postgis is one user of it, per [1]. The other extensions mentioned there just seem to be copying code and not using it. David [1] https://codesearch.debian.net/search?q=MemoryContextContains=1
Re: Reducing the chunk header sizes on all memory context types
On Tue, 6 Sept 2022 at 12:27, Tom Lane wrote: > > David Rowley writes: > > On Tue, 6 Sept 2022 at 11:09, Andres Freund wrote: > >> I was looking at > >> MemoryContextContains(). Unless I am missing something, the patch omitted > >> adjusting that? We'll probably always return false right now. > > > Oops. Yes. I'll push a fix a bit later. I think the fix is harder than I thought, or perhaps impossible to do given how we now determine the owning MemoryContext of a pointer. There's a comment in MemoryContextContains which says: * NB: Can't use GetMemoryChunkContext() here - that performs assertions * that aren't acceptable here since we might be passed memory not * allocated by any memory context. That seems to indicate that we should be able to handle any random pointer given to us (!). That comment seems more confident that'll work than the function's header comment does: * Caution: this test is reliable as long as 'pointer' does point to * a chunk of memory allocated from *some* context. If 'pointer' points * at memory obtained in some other way, there is a small chance of a * false-positive result, since the bits right before it might look like * a valid chunk header by chance. Here that's just claiming the test might not be reliable and could return false-positive results. I find this entire function pretty scary as even before the context changes that function seems to think it's fine to subtract sizeof(void *) from the given pointer and dereference that memory. That could very well segfault. I wonder if there are many usages of MemoryContextContains in extensions. If there's not, I'd be much happier if we got rid of this function and used GetMemoryChunkContext() in nodeAgg.c and nodeWindowAgg.c. > +1 for adding something to regress.c that verifies that this > works properly for all three allocators. I suggest making > three contexts and cross-checking the correct results for > all combinations of chunk A vs context B. I went as far as adding an Assert to palloc(). I'm not quite sure what you have in mind in regress.c Attached is a draft patch. I just don't like this function one bit. David diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index 64bcc7ef32..a6b2d02b75 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -809,11 +809,10 @@ MemoryContextCheck(MemoryContext context) * Detect whether an allocated chunk of memory belongs to a given * context or not. * - * Caution: this test is reliable as long as 'pointer' does point to - * a chunk of memory allocated from *some* context. If 'pointer' points - * at memory obtained in some other way, there is a small chance of a - * false-positive result, since the bits right before it might look like - * a valid chunk header by chance. + * Caution: 'pointer' must point to a pointer which was allocated by a + * MemoryContext. It's not safe or valid to use this function on arbitrary + * pointers as obtaining the MemoryContext which 'pointer' belongs to requires + * possibly several pointer dereferences. */ bool MemoryContextContains(MemoryContext context, void *pointer) @@ -821,10 +820,6 @@ MemoryContextContains(MemoryContext context, void *pointer) MemoryContext ptr_context; /* -* NB: Can't use GetMemoryChunkContext() here - that performs assertions -* that aren't acceptable here since we might be passed memory not -* allocated by any memory context. -* * Try to detect bogus pointers handed to us, poorly though we can. * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an * allocated chunk. @@ -835,7 +830,7 @@ MemoryContextContains(MemoryContext context, void *pointer) /* * OK, it's probably safe to look at the context. */ - ptr_context = *(MemoryContext *) (((char *) pointer) - sizeof(void *)); + ptr_context = GetMemoryChunkContext(pointer); return ptr_context == context; } @@ -1151,6 +1146,8 @@ palloc(Size size) size, context->name))); } + Assert(MemoryContextContains(context, ret)); + VALGRIND_MEMPOOL_ALLOC(context, ret, size); return ret;
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > On Tue, 6 Sept 2022 at 11:09, Andres Freund wrote: >> I was looking at >> MemoryContextContains(). Unless I am missing something, the patch omitted >> adjusting that? We'll probably always return false right now. > Oops. Yes. I'll push a fix a bit later. The existing uses in nodeAgg and nodeWindowAgg failed to expose this because an incorrect false result just causes them to do extra work (ie, a useless datumCopy). I think there might be a memory leak too, but the regression tests wouldn't run an aggregation long enough to make that obvious either. +1 for adding something to regress.c that verifies that this works properly for all three allocators. I suggest making three contexts and cross-checking the correct results for all combinations of chunk A vs context B. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 6 Sept 2022 at 11:09, Andres Freund wrote: > I was looking at > MemoryContextContains(). Unless I am missing something, the patch omitted > adjusting that? We'll probably always return false right now. Oops. Yes. I'll push a fix a bit later. David
Re: Reducing the chunk header sizes on all memory context types
Hi, On 2022-08-29 17:26:29 +1200, David Rowley wrote: > On Mon, 29 Aug 2022 at 10:39, David Rowley wrote: > > One more try to make CFbot happy. > > After a bit more revision, mostly updating outdated comments and > naming adjustments, I've pushed this. Responding to Tom's email about guc.c changes [1], I was looking at MemoryContextContains(). Unless I am missing something, the patch omitted adjusting that? We'll probably always return false right now. Probably should have something that tests that MemoryContextContains() works at least to some degree. Perhaps a test in regress.c? Greetings, Andres Freund [1] https://postgr.es/m/2982579.1662416866%40sss.pgh.pa.us
Re: Reducing the chunk header sizes on all memory context types
On Fri, 2 Sept 2022 at 20:11, David Rowley wrote: > > On Thu, 1 Sept 2022 at 12:46, Tom Lane wrote: > > > > David Rowley writes: > > > Maybe we should just consider always making room for a sentinel for > > > chunks that are on dedicated blocks. At most that's an extra 8 bytes > > > in some allocation that's either over 1024 or 8192 (depending on > > > maxBlockSize). > > > > Agreed, if we're not doing that already then we should. > > Here's a patch to that effect. If there are no objections, then I plan to push that patch soon. David
Re: Reducing the chunk header sizes on all memory context types
On Thu, 1 Sept 2022 at 12:46, Tom Lane wrote:
>
> David Rowley writes:
> > Maybe we should just consider always making room for a sentinel for
> > chunks that are on dedicated blocks. At most that's an extra 8 bytes
> > in some allocation that's either over 1024 or 8192 (depending on
> > maxBlockSize).
>
> Agreed, if we're not doing that already then we should.
Here's a patch to that effect.
I've made it so that there's always space for the sentinel for all
generation.c and slab.c allocations. There is no power of 2 rounding
with those, so no concern about doubling the memory for power-of-2
sized allocations.
With aset.c, I'm only adding sentinel space when size >
allocChunkLimit, aka external chunks.
David
From a61c1646987996192ef9e917ca0fabbef51e34c9 Mon Sep 17 00:00:00 2001
From: David Rowley
Date: Fri, 2 Sep 2022 13:30:06 +1200
Subject: [PATCH v1] Make more effort to have a sentinel byte in memory
contexts
---
src/backend/utils/mmgr/aset.c | 41 +++
src/backend/utils/mmgr/generation.c | 43 +
src/backend/utils/mmgr/slab.c | 37 +
3 files changed, 74 insertions(+), 47 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index b6eeb8abab..ec423375ae 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -705,7 +705,13 @@ AllocSetAlloc(MemoryContext context, Size size)
*/
if (size > set->allocChunkLimit)
{
+#ifdef MEMORY_CONTEXT_CHECKING
+ /* ensure there's always space for the sentinel byte */
+ chunk_size = MAXALIGN(size + 1);
+#else
chunk_size = MAXALIGN(size);
+#endif
+
blksize = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
block = (AllocBlock) malloc(blksize);
if (block == NULL)
@@ -724,8 +730,8 @@ AllocSetAlloc(MemoryContext context, Size size)
#ifdef MEMORY_CONTEXT_CHECKING
chunk->requested_size = size;
/* set mark to catch clobber of "unused" space */
- if (size < chunk_size)
- set_sentinel(MemoryChunkGetPointer(chunk), size);
+ Assert(size < chunk_size);
+ set_sentinel(MemoryChunkGetPointer(chunk), size);
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* fill the allocated space with junk */
@@ -766,6 +772,12 @@ AllocSetAlloc(MemoryContext context, Size size)
* corresponding free list to see if there is a free chunk we could
reuse.
* If one is found, remove it from the free list, make it again a member
* of the alloc set and return its data address.
+*
+* Note that we don't attempt to ensure there's space for the sentinel
+* byte here. We expect a large proportion of allocations to be for
sizes
+* which are already a power of 2. If we were to always make space for
a
+* sentinel byte in MEMORY_CONTEXT_CHECKING builds, then we'd end up
+* doubling the memory requirements for such allocations.
*/
fidx = AllocSetFreeIndex(size);
chunk = set->freelist[fidx];
@@ -992,10 +1004,10 @@ AllocSetFree(void *pointer)
Sizechunk_size = block->endptr - (char *)
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",
-set->header.name, chunk);
+ Assert(chunk->requested_size < chunk_size);
+ if (!sentinel_ok(pointer, chunk->requested_size))
+ elog(WARNING, "detected write past chunk end in
%s %p",
+set->header.name, chunk);
}
#endif
@@ -1098,10 +1110,10 @@ AllocSetRealloc(void *pointer, Size size)
#ifdef MEMORY_CONTEXT_CHECKING
/* 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",
-set->header.name, chunk);
+ Assert(chunk->requested_size < oldsize);
+ if (!sentinel_ok(pointer, chunk->requested_size))
+ elog(WARNING, "detected write past chunk end in %s %p",
+set->header.name, chunk);
#endif
/*
@@ -,7 +1123,12 @@ AllocSetRealloc(void *pointer, Size size)
if (block->freeptr != block->endptr)
elog(ERROR, "could not find
Re: Reducing the chunk header sizes on all memory context types
Hi,
Excuse me for posting on this thread.
Coverity has a complaints about aset.c
CID 1497225 (#1 of 2): Out-of-bounds write (OVERRUN)3. overrun-local:
Overrunning
array set->freelist of 11 8-byte elements at element index 1073741823 (byte
offset 8589934591) using index fidx (which evaluates to 1073741823).
CID 1497225 (#2 of 2): Out-of-bounds write (OVERRUN)3. overrun-local:
Overrunning
array set->freelist of 11 8-byte elements at element index 1073741823 (byte
offset 8589934591) using index fidx (which evaluates to 1073741823).
I think that this is an oversight.
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index b6eeb8abab..8f709514b2 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -1024,7 +1024,7 @@ AllocSetFree(void *pointer)
}
else
{
- int fidx = MemoryChunkGetValue(chunk);
+ Size fidx = MemoryChunkGetValue(chunk);
AllocBlock block = MemoryChunkGetBlock(chunk);
AllocFreeListLink *link = GetFreeListLink(chunk);
MemoryChunkGetValue return Size not int.
Not sure if this fix is enough.
regards,
Ranier Vilela
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > On Thu, 1 Sept 2022 at 16:06, Tom Lane wrote: >> It doesn't seem to be fixing any live bug in the back branches, >> but by the same token it's harmless. > I considered that an extension might use the Slab allocator with a > non-MAXALIGNED chunksize and might run into some troubles during > SlabCheck(). Oh, yeah, the sentinel_ok() change is a live bug. Extensions have no control over sizeof(SlabBlock) though. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Thu, 1 Sept 2022 at 16:06, Tom Lane wrote: > > David Rowley writes: > > Does anyone have any objections to d5ee4db0e in its entirety being > > backpatched? > > It doesn't seem to be fixing any live bug in the back branches, > but by the same token it's harmless. I considered that an extension might use the Slab allocator with a non-MAXALIGNED chunksize and might run into some troubles during SlabCheck(). David
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > Does anyone have any objections to d5ee4db0e in its entirety being > backpatched? It doesn't seem to be fixing any live bug in the back branches, but by the same token it's harmless. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 30 Aug 2022 at 13:16, David Rowley wrote: > I'm also wondering if this should also be backpatched back to v10, > providing the build farm likes it well enough on master. Does anyone have any objections to d5ee4db0e in its entirety being backpatched? David
Re: Reducing the chunk header sizes on all memory context types
Tomas Vondra writes: > You're probably right we'll notice the clobber cases due to corruption > of the next chunk header. The annoying thing is having a corrupted > header only tells you there's a corruption somewhere, but it may be hard > to know which part of the code caused it. Same's true of a sentinel, though. > OTOH we have platforms where valgrind is either not supported or no one > runs tests with (e.g. on rpi4 it'd take insane amounts of code). According to https://valgrind.org/info/platforms.html valgrind supports a pretty respectable set of platforms. It might be too slow to be useful on ancient hardware, of course. I've had some success in identifying clobber perpetrators by putting a hardware watchpoint on the clobbered word, which IIRC does work on recent ARM hardware. It's tedious and far more manual than valgrind, but it's possible. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On 9/1/22 02:23, Tom Lane wrote: > Tomas Vondra writes: >> Focusing on the aset, vast majority of allocations (60M out of 64M) is >> small enough to use power-of-2 logic, and we go from 6.3GB to 8.2GB, so >> ~30%. Not great, not terrible. > > Not sure why this escaped me before, but I remembered another argument > for not forcibly adding space for a sentinel: if you don't have room, > that means the chunk end is up against the header for the next chunk, > which means that any buffer overrun will clobber that header. So we'll > detect the problem anyway if we validate the headers to a reasonable > extent. > > The hole in this argument is that the very last chunk allocated in a > block might have no following chunk to validate. But we could probably > special-case that somehow, maybe by laying down a sentinel in the free > space, where it will get overwritten by the next chunk when that does > get allocated. > > 30% memory bloat seems like a high price to pay if it's adding negligible > detection ability, which it seems is true if this argument is valid. > Is there reason to think we can't validate headers enough to catch > clobbers? > I'm not quite convinced the 30% figure is correct - it might be if you ignore cases exceeding allocChunkLimit, but that also makes it pretty bogus (because large allocations represent ~2x as much space). You're probably right we'll notice the clobber cases due to corruption of the next chunk header. The annoying thing is having a corrupted header only tells you there's a corruption somewhere, but it may be hard to know which part of the code caused it. I was hoping the sentinel would make it easier, because we mark it as NOACCESS for valgrind. But now I see we mark the first part of a MemoryChunk too, so maybe that's enough. OTOH we have platforms where valgrind is either not supported or no one runs tests with (e.g. on rpi4 it'd take insane amounts of code). In that case the sentinel might be helpful, especially considering alignment on those platforms can cause funny memory issues, as evidenced by this thread. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > Maybe we should just consider always making room for a sentinel for > chunks that are on dedicated blocks. At most that's an extra 8 bytes > in some allocation that's either over 1024 or 8192 (depending on > maxBlockSize). Agreed, if we're not doing that already then we should. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Thu, 1 Sept 2022 at 12:23, Tom Lane wrote: > Is there reason to think we can't validate headers enough to catch > clobbers? For non-sentinel chunks, the next byte after the end of the chunk will be storing the block offset for the following chunk. I think: if (block != MemoryChunkGetBlock(chunk)) elog(WARNING, "problem in alloc set %s: bad block offset for chunk %p in block %p", name, chunk, block); should catch those. Maybe we should just consider always making room for a sentinel for chunks that are on dedicated blocks. At most that's an extra 8 bytes in some allocation that's either over 1024 or 8192 (depending on maxBlockSize). David
Re: Reducing the chunk header sizes on all memory context types
Tomas Vondra writes: > Focusing on the aset, vast majority of allocations (60M out of 64M) is > small enough to use power-of-2 logic, and we go from 6.3GB to 8.2GB, so > ~30%. Not great, not terrible. Not sure why this escaped me before, but I remembered another argument for not forcibly adding space for a sentinel: if you don't have room, that means the chunk end is up against the header for the next chunk, which means that any buffer overrun will clobber that header. So we'll detect the problem anyway if we validate the headers to a reasonable extent. The hole in this argument is that the very last chunk allocated in a block might have no following chunk to validate. But we could probably special-case that somehow, maybe by laying down a sentinel in the free space, where it will get overwritten by the next chunk when that does get allocated. 30% memory bloat seems like a high price to pay if it's adding negligible detection ability, which it seems is true if this argument is valid. Is there reason to think we can't validate headers enough to catch clobbers? regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On 8/31/22 23:46, David Rowley wrote:
> On Thu, 1 Sept 2022 at 08:53, Tomas Vondra
> wrote:
>> 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.
>
> The reason my reported results were lower is because I ignored size >
> allocChunkLimit allocations. These are not raised to the next power of
> 2, so I didn't think they should be included.
If I differentiate the large chunks allocated separately (v2 patch
attached), I get this:
f|t | count | s1 | s2 | s3
-+--+--+--+--+--
AllocSetAlloc | normal | 60714914 | 4982 | 6288 | 8185
AllocSetAlloc | separate | 824390 |18245 |18245 |18251
AllocSetRelloc | normal | 182070 | 763 | 826 | 1423
GenerationAlloc | normal | 2118115 | 68 | 90 | 102
GenerationAlloc | separate | 28 |0 |0 |0
(5 rows)
Where s1 is the sum of requested sizes, s2 is the sum of allocated
chunks, and s3 is chunks allocated with 1B sentinel.
Focusing on the aset, vast majority of allocations (60M out of 64M) is
small enough to use power-of-2 logic, and we go from 6.3GB to 8.2GB, so
~30%. Not great, not terrible.
For the large allocations, there's almost no increase - in the last
query I used the power-of-2 logic in the calculations, but that was
incorrect, of course.
>
> I'm not sure why you're seeing only a 3GB additional overhead. I
> noticed a logic error in my query where I was checking
> maxaligned_size=pow2_size and doubling that to give sentinel space.
> That really should have been "case size=pow2_size then pow2_size * 2
> else pow2_size end", However, after adjusting the query, it does not
> seem to change the results much:
>
> postgres=# select
> postgres-# round(sum(pow2_Size)::numeric/1024/1024/1024,3) as pow2_size,
> postgres-# round(sum(case when size=pow2_size then pow2_size*2 else
> pow2_size end)::numeric/1024/1024/1024,3) as method1,
> postgres-# round(sum(case when size=pow2_size then pow2_size+8 else
> pow2_size end)::numeric/1024/1024/1024,3) as method2
> postgres-# from memstats
> postgres-# where pow2_size > 0;
> pow2_size | method1 | method2
> ---+-+-
> 10.269 | 16.322 | 10.476
>
> I've attached the crude patch I came up with for this. For some
> reason it was crashing on Linux, but it ran ok on Windows, so I used
> the results from that instead. Maybe that accounts for some
> differences as e.g sizeof(long) == 4 on 64-bit windows. I'd be
> surprised if that accounted for so many GBs though.
>
I tried to use that patch, but "make installcheck" never completes for
me, for some reason. It seems to get stuck in infinite_recurse.sql, but
I haven't looked into the details.
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..b215940062 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
@@ -703,9 +712,12 @@ AllocSetAlloc(MemoryContext context, Size size)
* 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);
+ fprintf(stderr, "AllocSetAlloc separate %ld %ld %ld\n", size, MAXALIGN(size), MAXALIGN(size+1));
+ fflush(stderr);
+
+ chunk_size = MAXALIGN(size+1);
blksize = chunk_size + ALLOC_BLOCKHDRSZ + ALLOC_CHUNKHDRSZ;
block = (AllocBlock) malloc(blksize);
if (block == NULL)
@@ -726,6 +738,11 @@ AllocSetAlloc(MemoryContext context, Size size)
/* set mark to catch clobber of "unused" space */
if (size < chunk_size)
set_sentinel(MemoryChunkGetPointer(chunk), size);
+ else
+ {
+ fprintf(stderr, "sentinel not added\n");
+ fflush(stderr);
+ }
#endif
#ifdef RANDOMIZE_ALLOCATED_MEMORY
/* fill the allocated space with junk */
@@ -761,13 +778,16 @@ AllocSetAlloc(MemoryContext context, Size size)
return MemoryChunkGetPointer(chunk);
}
+ fprintf(stderr, "AllocSetAlloc normal %ld %ld %ld\n", size, pow2_size(size), pow2_size(size+1));
+ fflush(stderr);
+
/*
* Request is small enough to be treated as a chunk. Look in the
* corresponding
Re: Reducing the chunk header sizes on all memory context types
On Thu, 1 Sept 2022 at 08:53, Tomas Vondra
wrote:
> 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.
The reason my reported results were lower is because I ignored size >
allocChunkLimit allocations. These are not raised to the next power of
2, so I didn't think they should be included.
I'm not sure why you're seeing only a 3GB additional overhead. I
noticed a logic error in my query where I was checking
maxaligned_size=pow2_size and doubling that to give sentinel space.
That really should have been "case size=pow2_size then pow2_size * 2
else pow2_size end", However, after adjusting the query, it does not
seem to change the results much:
postgres=# select
postgres-# round(sum(pow2_Size)::numeric/1024/1024/1024,3) as pow2_size,
postgres-# round(sum(case when size=pow2_size then pow2_size*2 else
pow2_size end)::numeric/1024/1024/1024,3) as method1,
postgres-# round(sum(case when size=pow2_size then pow2_size+8 else
pow2_size end)::numeric/1024/1024/1024,3) as method2
postgres-# from memstats
postgres-# where pow2_size > 0;
pow2_size | method1 | method2
---+-+-
10.269 | 16.322 | 10.476
I've attached the crude patch I came up with for this. For some
reason it was crashing on Linux, but it ran ok on Windows, so I used
the results from that instead. Maybe that accounts for some
differences as e.g sizeof(long) == 4 on 64-bit windows. I'd be
surprised if that accounted for so many GBs though.
I also forgot to add code to GenerationRealloc and AllocSetRealloc
David
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index b6eeb8abab..f4977f9bcc 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -46,6 +46,7 @@
#include "postgres.h"
+#include "miscadmin.h"
#include "port/pg_bitutils.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -696,9 +697,22 @@ AllocSetAlloc(MemoryContext context, Size size)
int fidx;
Sizechunk_size;
Sizeblksize;
+ static int rlevel = 1;
AssertArg(AllocSetIsValid(set));
+ if (rlevel == 1 && GetProcessingMode() == NormalProcessing)
+ {
+ rlevel++;
+ elog(LOG, "AllocSetAlloc,%s,%zu,%zu,%zu,%zu",
+context->name,
+size,
+MAXALIGN(size),
+size > set->allocChunkLimit ? 0 :
GetChunkSizeFromFreeListIdx(AllocSetFreeIndex(size)),
+set->allocChunkLimit);
+ rlevel--;
+ }
+
/*
* If requested size exceeds maximum for chunks, allocate an entire
block
* for this request.
diff --git a/src/backend/utils/mmgr/generation.c
b/src/backend/utils/mmgr/generation.c
index b39894ec94..9c5ff3c095 100644
--- a/src/backend/utils/mmgr/generation.c
+++ b/src/backend/utils/mmgr/generation.c
@@ -36,6 +36,7 @@
#include "postgres.h"
#include "lib/ilist.h"
+#include "miscadmin.h"
#include "port/pg_bitutils.h"
#include "utils/memdebug.h"
#include "utils/memutils.h"
@@ -344,6 +345,18 @@ GenerationAlloc(MemoryContext context, Size size)
MemoryChunk *chunk;
Sizechunk_size = MAXALIGN(size);
Sizerequired_size = chunk_size + Generation_CHUNKHDRSZ;
+ static int rlevel = 1;
+
+ if (rlevel == 1 && GetProcessingMode() == NormalProcessing)
+ {
+ rlevel++;
+ elog(LOG, "GenerationAlloc,%s,%zu,%zu,0,%zu",
+context->name,
+size,
+MAXALIGN(size),
+set->allocChunkLimit);
+ rlevel--;
+ }
/* is it an over-sized chunk? if yes, allocate special block */
if (chunk_size > set->allocChunkLimit)
Re: Reducing the chunk header sizes on all memory context types
On 8/31/22 00:40, David Rowley wrote:
> On Wed, 31 Aug 2022 at 02:17, Tom Lane 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))
+ {
Re: Reducing the chunk header sizes on all memory context types
On Wed, 31 Aug 2022 at 02:17, Tom Lane 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. David
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > Here's a patch which adds a comment to MemoryContextMethodID to Robert's > patch. OK, but while looking at that I noticed the adjacent #define MEMORY_CONTEXT_METHODID_MASK \ UINT64CONST((1 << MEMORY_CONTEXT_METHODID_BITS) - 1) I'm rather astonished that that compiles; UINT64CONST was only ever meant to be applied to *literals*. I think what it's expanding to is ((1 << MEMORY_CONTEXT_METHODID_BITS) - 1UL) (or on some machines 1ULL) which only accidentally does approximately what you want. It'd be all right perhaps to write ((UINT64CONST(1) << MEMORY_CONTEXT_METHODID_BITS) - 1) but you might as well avoid the Postgres-ism and just write ((uint64) ((1 << MEMORY_CONTEXT_METHODID_BITS) - 1)) Nobody's ever going to make MEMORY_CONTEXT_METHODID_BITS large enough for the shift to overflow in int arithmetic. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, Aug 30, 2022 at 11:39 AM David Rowley wrote: > On Wed, 31 Aug 2022 at 03:31, David Rowley wrote: > > I've no objections to adding a comment to the enum to > > explain to future devs. My vote would be for that and add the (int) > > cast as proposed by Robert. > > Here's a patch which adds a comment to MemoryContextMethodID to Robert's > patch. LGTM. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Reducing the chunk header sizes on all memory context types
On Wed, 31 Aug 2022 at 03:31, David Rowley wrote:
> I've no objections to adding a comment to the enum to
> explain to future devs. My vote would be for that and add the (int)
> cast as proposed by Robert.
Here's a patch which adds a comment to MemoryContextMethodID to Robert's patch.
David
diff --git a/src/include/utils/memutils_internal.h
b/src/include/utils/memutils_internal.h
index 9a9f52ef16..f348282a07 100644
--- a/src/include/utils/memutils_internal.h
+++ b/src/include/utils/memutils_internal.h
@@ -74,6 +74,9 @@ extern void SlabCheck(MemoryContext context);
* MemoryContextMethodID
* A unique identifier for each MemoryContext implementation which
* indicates the index into the mcxt_methods[] array. See mcxt.c.
+ * The maximum value for this enum is constrained by
+ * MEMORY_CONTEXT_METHODID_MASK. If an enum value higher than
that is
+ * required then MEMORY_CONTEXT_METHODID_BITS will need to be
increased.
*/
typedef enum MemoryContextMethodID
{
diff --git a/src/include/utils/memutils_memorychunk.h
b/src/include/utils/memutils_memorychunk.h
index 685c177b68..2eefc138e3 100644
--- a/src/include/utils/memutils_memorychunk.h
+++ b/src/include/utils/memutils_memorychunk.h
@@ -159,7 +159,7 @@ MemoryChunkSetHdrMask(MemoryChunk *chunk, void *block,
Assert((char *) chunk > (char *) block);
Assert(blockoffset <= MEMORYCHUNK_MAX_BLOCKOFFSET);
Assert(value <= MEMORYCHUNK_MAX_VALUE);
- Assert(methodid <= MEMORY_CONTEXT_METHODID_MASK);
+ Assert((int) methodid <= MEMORY_CONTEXT_METHODID_MASK);
chunk->hdrmask = (((uint64) blockoffset) <<
MEMORYCHUNK_BLOCKOFFSET_BASEBIT) |
(((uint64) value) << MEMORYCHUNK_VALUE_BASEBIT) |
@@ -175,7 +175,7 @@ static inline void
MemoryChunkSetHdrMaskExternal(MemoryChunk *chunk,
MemoryContextMethodID
methodid)
{
- Assert(methodid <= MEMORY_CONTEXT_METHODID_MASK);
+ Assert((int) methodid <= MEMORY_CONTEXT_METHODID_MASK);
chunk->hdrmask = MEMORYCHUNK_MAGIC | (((uint64) 1) <<
MEMORYCHUNK_EXTERNAL_BASEBIT) |
methodid;
Re: Reducing the chunk header sizes on all memory context types
On Wed, 31 Aug 2022 at 01:36, Tom Lane wrote: > > David Rowley writes: > > I think the Assert is useful as if we were ever to add an enum member > > with the value of 8 and forgot to adjust MEMORY_CONTEXT_METHODID_BITS > > then bad things would happen inside MemoryChunkSetHdrMask() and > > MemoryChunkSetHdrMaskExternal(). I think it's unlikely we'll ever get > > that many MemoryContext types, but I don't know for sure and would > > rather the person who adds the 9th one get alerted to the lack of bit > > space in MemoryChunk as soon as possible. > > I think that's a weak argument, so I don't mind dropping this Assert. > What would be far more useful is a comment inside the > MemoryContextMethodID enum pointing out that we can support at most > 8 values because XYZ. I'd just sleep better knowing that we have some test coverage to ensure that MemoryChunkSetHdrMask() and MemoryChunkSetHdrMaskExternal() have some verification that we don't end up with future code that will cause the hdrmask to be invalid. I tried to make those functions as lightweight as possible. Without the Assert, I just feel that there's a bit too much trust that none of the bits overlap. I've no objections to adding a comment to the enum to explain to future devs. My vote would be for that and add the (int) cast as proposed by Robert. David
Re: Reducing the chunk header sizes on all memory context types
On Wed, 31 Aug 2022 at 03:00, Robert Haas wrote: > > On Tue, Aug 30, 2022 at 3:14 AM David Rowley wrote: > > I think the Assert is useful as if we were ever to add an enum member > > with the value of 8 and forgot to adjust MEMORY_CONTEXT_METHODID_BITS > > then bad things would happen inside MemoryChunkSetHdrMask() and > > MemoryChunkSetHdrMaskExternal(). I think it's unlikely we'll ever get > > that many MemoryContext types, but I don't know for sure and would > > rather the person who adds the 9th one get alerted to the lack of bit > > space in MemoryChunk as soon as possible. > > Well I don't have a problem with that, but I think we should try to do > it without causing compiler warnings. The attached patch fixes it for > me. I'm fine with adding the int cast. Seems like a good idea. > > As much as I'm not a fan of adding new warnings for compiler options > > that are not part of our standard set, I feel like if there are > > warning flags out there that are as giving us false warnings such as > > this one, then we shouldn't trouble ourselves trying to get rid of > > them, especially so when they force us to remove something which might > > catch a future bug. > > For me the point is that, at least on the compiler that I'm using, the > warning suggests that the compiler will optimize the test away > completely, and therefore it wouldn't catch a future bug. Could there > be compilers where no warning is generated but the assertion is still > optimized away? I'd not considered that the compiler might optimise it away. My suspicions had been more along the lines of that clang removed the enum out of range warnings because they were annoying and wrong as it's pretty easy to set an enum variable to something out of range of the defined enum values. Looking at [1], it seems like 5.0.2 is producing the correct code and it's just producing a warning. The 2nd compiler window has -Werror and shows that it does fail to compile. If I change that to use clang 6.0.0 then it works. It seems to fail all the way back to clang 3.1. clang 3.0.0 works. David [1] https://godbolt.org/z/Gx388z5Ej
Re: Reducing the chunk header sizes on all memory context types
On Tue, Aug 30, 2022 at 3:14 AM David Rowley wrote: > I'm not really sure either. I tried compiling with clang 12.0.1 with > -Wtautological-constant-out-of-range-compare and don't get this > warning. I have a much older clang version, it seems. clang -v reports 5.0.2. I use -Wall and -Werror as a matter of habit. It looks like 5.0.2 was released in May 2018, installed by me in November of 2019, and I just haven't had a reason to upgrade. > I think the Assert is useful as if we were ever to add an enum member > with the value of 8 and forgot to adjust MEMORY_CONTEXT_METHODID_BITS > then bad things would happen inside MemoryChunkSetHdrMask() and > MemoryChunkSetHdrMaskExternal(). I think it's unlikely we'll ever get > that many MemoryContext types, but I don't know for sure and would > rather the person who adds the 9th one get alerted to the lack of bit > space in MemoryChunk as soon as possible. Well I don't have a problem with that, but I think we should try to do it without causing compiler warnings. The attached patch fixes it for me. > As much as I'm not a fan of adding new warnings for compiler options > that are not part of our standard set, I feel like if there are > warning flags out there that are as giving us false warnings such as > this one, then we shouldn't trouble ourselves trying to get rid of > them, especially so when they force us to remove something which might > catch a future bug. For me the point is that, at least on the compiler that I'm using, the warning suggests that the compiler will optimize the test away completely, and therefore it wouldn't catch a future bug. Could there be compilers where no warning is generated but the assertion is still optimized away? I don't know, but I don't think a 4-year-old compiler is such a fossil that we shouldn't care whether it produces warnings. We worry about operating systems and PostgreSQL versions that are almost extinct in the wild, so saying we're not going to worry about failing to update the compiler regularly enough within the lifetime of one off-the-shelf MacBook does not really make sense to me. -- Robert Haas EDB: http://www.enterprisedb.com fix-memorychunk-warnings.patch Description: Binary data
Re: Reducing the chunk header sizes on all memory context types
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. You could imagine deciding that the space classifications are not power-of-2 but power-of-2-plus-one, or something like that. But that would be very invasive to the logic, and I doubt it's a good idea. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > On Tue, 30 Aug 2022 at 03:16, Robert Haas wrote: >> ../../../../src/include/utils/memutils_memorychunk.h:186:18: error: >> comparison of constant 7 with expression of type >> 'MemoryContextMethodID' (aka 'enum MemoryContextMethodID') is always >> true [-Werror,-Wtautological-constant-out-of-range-compare] >> Assert(methodid <= MEMORY_CONTEXT_METHODID_MASK); >> ^~~~ > I think the Assert is useful as if we were ever to add an enum member > with the value of 8 and forgot to adjust MEMORY_CONTEXT_METHODID_BITS > then bad things would happen inside MemoryChunkSetHdrMask() and > MemoryChunkSetHdrMaskExternal(). I think it's unlikely we'll ever get > that many MemoryContext types, but I don't know for sure and would > rather the person who adds the 9th one get alerted to the lack of bit > space in MemoryChunk as soon as possible. I think that's a weak argument, so I don't mind dropping this Assert. What would be far more useful is a comment inside the MemoryContextMethodID enum pointing out that we can support at most 8 values because XYZ. However, I'm still wondering why Robert sees this when the rest of us don't. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
Tomas Vondra writes: > I guess the idea was to add a sentinel only when there already is space > for it, but perhaps that's a bad tradeoff limiting the benefits. Either > we add the sentinel fairly often (and then why not just add it all the > time - it'll need a bit more space), or we do it only very rarely (and > then it's a matter of luck if it catches an issue). I'm fairly sure that when we made that decision originally, a top-of-mind case was ListCells, which are plentiful, small, power-of-2-sized, and not used in a way likely to have buffer overruns. But since the List rewrite a couple years back we no longer palloc individual ListCells. 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. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On 8/30/22 04:31, David Rowley wrote: > On Tue, 30 Aug 2022 at 13:55, Tom Lane wrote: >> I wonder if slab ought to artificially bump up such requests when >> MEMORY_CONTEXT_CHECKING is enabled, so there's room for a sentinel. >> I think it's okay for aset.c to not do that, because its power-of-2 >> behavior means there usually is room for a sentinel; but slab's >> policy makes it much more likely that there won't be. > > I think it's fairly likely that small allocations are a power of 2, > and I think most of our allocates are small, so I imagine that if we > didn't do that for aset.c, we'd miss out on most of the benefits. > Yeah. I think we have a fair number of "larger" allocations (once you get to ~100B it probably won't be a 2^N), but we may easily miss whole sections of allocations. I guess the idea was to add a sentinel only when there already is space for it, but perhaps that's a bad tradeoff limiting the benefits. Either we add the sentinel fairly often (and then why not just add it all the time - it'll need a bit more space), or we do it only very rarely (and then it's a matter of luck if it catches an issue). Considering we only do this with asserts, I doubt the extra bytes / CPU is a major issue, and a (more) reliable detection of issues seems worth it. But maybe I underestimate the costs. The only alternative seems to be valgrind, and that's way costlier, though. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Reducing the chunk header sizes on all memory context types
On 8/30/22 03:04, David Rowley wrote: > On Tue, 30 Aug 2022 at 12:22, Tomas Vondra > wrote: >> I also suggested doing a similar check in MemoryChunkGetPointer, so that >> we catch the issue earlier - right after we allocate the chunk. Any >> opinion on that? > > I think it's probably a good idea. However, I'm not yet sure if we can > keep it as a macro or if it would need to become a static inline > function to do that. > I'd bet it can be done in the macro. See VARATT_EXTERNAL_GET_POINTER for example of a "do" block with an Assert. > What I'd really have wished for is a macro like AssertPointersEqual() > that spat out the two pointer values. That would probably have saved > more time on this issue. > Hmm, maybe. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Reducing the chunk header sizes on all memory context types
On Tue, 30 Aug 2022 at 03:16, Robert Haas wrote: > ../../../../src/include/utils/memutils_memorychunk.h:186:18: error: > comparison of constant 7 with expression of type > 'MemoryContextMethodID' (aka 'enum MemoryContextMethodID') is always > true [-Werror,-Wtautological-constant-out-of-range-compare] > Assert(methodid <= MEMORY_CONTEXT_METHODID_MASK); > ^~~~ > ../../../../src/include/c.h:827:9: note: expanded from macro 'Assert' > if (!(condition)) \ > ^ > > I'm not sure what to do about that, but every file that includes > memutils_memorychunk.h produces those warnings (which become errors > due to -Werror). I'm not really sure either. I tried compiling with clang 12.0.1 with -Wtautological-constant-out-of-range-compare and don't get this warning. I think the Assert is useful as if we were ever to add an enum member with the value of 8 and forgot to adjust MEMORY_CONTEXT_METHODID_BITS then bad things would happen inside MemoryChunkSetHdrMask() and MemoryChunkSetHdrMaskExternal(). I think it's unlikely we'll ever get that many MemoryContext types, but I don't know for sure and would rather the person who adds the 9th one get alerted to the lack of bit space in MemoryChunk as soon as possible. As much as I'm not a fan of adding new warnings for compiler options that are not part of our standard set, I feel like if there are warning flags out there that are as giving us false warnings such as this one, then we shouldn't trouble ourselves trying to get rid of them, especially so when they force us to remove something which might catch a future bug. David
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > I can revert df0f4feef, but would prefer just to get the green light > for d5ee4db0e from those 32-bit arm animals before doing so. I have a check-world pass on my RPI3 (Fedora 30 armv7l image). PPC test still running, but I don't doubt it will pass; it's finished contrib/test_decoding. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 30 Aug 2022 at 15:15, Tom Lane wrote: > AFAICS that could only happen if "double" has 8-byte alignment > requirement but int64 does not. I recall some discussion about > that possibility a month or two back, but I think we concluded > that we weren't going to support it. ok > I guess what I mostly don't like about df0f4feef is the hardwired "8" > constants. Yeah, it's hard to see how sizeof(uint64) isn't 8, but > it's not very readable like this IMO. Yeah, that was just down to lack of any SIZEOF_* macro to tell me uint64 was 8 bytes. I can revert df0f4feef, but would prefer just to get the green light for d5ee4db0e from those 32-bit arm animals before doing so. David
Re: Reducing the chunk header sizes on all memory context types
David Rowley writes: > On Tue, 30 Aug 2022 at 03:39, Tom Lane wrote: >> I'd suggest reverting df0f4feef as it seems to be >> a red herring. > I think it's useless providing that a 64-bit variable will always be > aligned to 8 bytes on all of our supported 32-bit platforms as, > without the padding, the uint64 hdrmask in MemoryChunk will always be > aligned to 8 bytes meaning the memory following that will be aligned > too. If we have a platform where a uint64 isn't aligned to 8 bytes > then we might need the padding. It's not so much "8 bytes". The question is this: is there any platform on which uint64 has less than MAXALIGN alignment requirement? If it is maxaligned then the compiler will insert any required padding automatically, so the patch accomplishes little. AFAICS that could only happen if "double" has 8-byte alignment requirement but int64 does not. I recall some discussion about that possibility a month or two back, but I think we concluded that we weren't going to support it. I guess what I mostly don't like about df0f4feef is the hardwired "8" constants. Yeah, it's hard to see how sizeof(uint64) isn't 8, but it's not very readable like this IMO. regards, tom lane
Re: Reducing the chunk header sizes on all memory context types
On Tue, 30 Aug 2022 at 03:39, Tom Lane wrote:
> I'd suggest reverting df0f4feef as it seems to be
> a red herring.
I think it's useless providing that a 64-bit variable will always be
aligned to 8 bytes on all of our supported 32-bit platforms as,
without the padding, the uint64 hdrmask in MemoryChunk will always be
aligned to 8 bytes meaning the memory following that will be aligned
too. If we have a platform where a uint64 isn't aligned to 8 bytes
then we might need the padding.
long long seems to align to 8 bytes on my 32-bit Rasberry PI going the
struct being 16 bytes rather than 12.
drowley@raspberrypi:~ $ cat struct.c
#include
typedef struct test
{
int a;
long long b;
} test;
int main(void)
{
printf("%d\n", sizeof(test));
return 0;
}
drowley@raspberrypi:~ $ gcc struct.c -o struct
drowley@raspberrypi:~ $ ./struct
16
drowley@raspberrypi:~ $ uname -m
armv7l
Is that the case for your 32-bit PPC too?
David
Re: Reducing the chunk header sizes on all memory context types
On Tue, 30 Aug 2022 at 13:58, Tomas Vondra wrote: > > On 8/30/22 03:16, David Rowley wrote: > > Any chance you could run make check-world on your 32-bit Raspberry PI? > > > > Will do, but I think the sentinel fix should be Thank you. I think Tom is also running make check-world. I now have an old RPI3 setup with a 32-bit OS, which is currently busy compiling Postgres. I'll kick off a make check-world run once that's done. >if (!sentinel_ok(chunk, Slab_CHUNKHDRSZ + slab->chunkSize)) Agreed. I've changed the patch to do it that way. Since the 32-bit ARM animals are already broken and per what Tom mentioned in [1], I've pushed the patch. Thanks again for taking the time to look at this. David [1] https://www.postgresql.org/message-id/3455754.1661823...@sss.pgh.pa.us
Re: Reducing the chunk header sizes on all memory context types
On Tue, 30 Aug 2022 at 13:55, Tom Lane wrote: > I wonder if slab ought to artificially bump up such requests when > MEMORY_CONTEXT_CHECKING is enabled, so there's room for a sentinel. > I think it's okay for aset.c to not do that, because its power-of-2 > behavior means there usually is room for a sentinel; but slab's > policy makes it much more likely that there won't be. I think it's fairly likely that small allocations are a power of 2, and I think most of our allocates are small, so I imagine that if we didn't do that for aset.c, we'd miss out on most of the benefits. David
Re: Reducing the chunk header sizes on all memory context types
On Tue, 30 Aug 2022 at 13:58, Tomas Vondra wrote: > armv7l (32-bit rpi4) > > +WARNING: chunkSize 216 fullChunkSize 232 header 16 > +WARNING: chunkSize 64 fullChunkSize 80 header 16 > > aarch64 (64-bit rpi4) > > +WARNING: chunkSize 304 fullChunkSize 320 header 16 > +WARNING: chunkSize 80 fullChunkSize 96 header 16 > > So indeed, those are *perfect* matches and thus the sentinel_ok() never > executed. So no failures until now. On x86-64 I get the same thing as on > aarch64. I guess that explains why it never failed. Seems like a pretty > amazing coincidence ... hmm, I'm not so sure I agree that it's an amazing coincidence. Isn't it quite likely that the chunksize being given to SlabContextCreate() is the same as MAXALIGN(chunksize)? Isn't that all it would take? David
Re: Reducing the chunk header sizes on all memory context types
On 8/30/22 03:55, Tom Lane wrote: > Tomas Vondra writes: >> On 8/30/22 02:45, David Rowley wrote: >>> I think the existing sentinel check looks wrong: >>> if (!sentinel_ok(chunk, slab->chunkSize)) >>> shouldn't that be passing the pointer rather than the chunk? > >> I agree the check in SlabCheck() looks wrong, as it's ignoring the chunk >> header (unlike the other contexts). > >> But yeah, I ran "make check-world" and it passed just fine, so my only >> explanation is that the check never actually executes because there's no >> space for the sentinel thanks to alignment, and the tweak you did breaks >> that. Strange ... > > A quick code-coverage check confirms that the sentinel_ok() line > is not reached in core or test_decoding tests as of HEAD > (on a 64-bit machine anyway). So we just happen to be using > only allocation requests that are already maxaligned. > > I wonder if slab ought to artificially bump up such requests when > MEMORY_CONTEXT_CHECKING is enabled, so there's room for a sentinel. > I think it's okay for aset.c to not do that, because its power-of-2 > behavior means there usually is room for a sentinel; but slab's > policy makes it much more likely that there won't be. > +1 to that For aset that's fine not just because of power-of-2 behavior, but because we use it for chunks of many different sizes - so at least some of those will have sentinel. But Slab in used only for changes and txns in reorderbuffer, and it just so happens both structs are maxaligned on 32-bit and 64-bit machines (rpi and x86-64). We're unlikely to use slab in many other places, and the structs don't change very often, and it'd probably grow to another maxaligned size anyway. So it may be pretty rare to have a sentinel. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Reducing the chunk header sizes on all memory context types
On 8/30/22 03:16, David Rowley wrote: > On Tue, 30 Aug 2022 at 12:45, David Rowley wrote: >> I think the existing sentinel check looks wrong: >> >> if (!sentinel_ok(chunk, slab->chunkSize)) >> >> shouldn't that be passing the pointer rather than the chunk? > > Here's v2 of the slab-fix patch. > > I've included the sentinel check fix. This passes make check-world > for me when do a 32-bit build on my x86_64 machine and adjust > pg_config.h to set MAXIMUM_ALIGNOF to 8. > > Any chance you could run make check-world on your 32-bit Raspberry PI? > Will do, but I think the sentinel fix should be if (!sentinel_ok(chunk, Slab_CHUNKHDRSZ + slab->chunkSize)) which is what the other contexts do. However, considering check-world passed even before the sentinel_ok fix, I'm a bit skeptical about that proving anything. FWIW I added a WARNING to SlabCheck before the condition guarding the sentinel check, printing the (full) chunk size and header size, and this is what I got in test_decoding (deduplicated): armv7l (32-bit rpi4) +WARNING: chunkSize 216 fullChunkSize 232 header 16 +WARNING: chunkSize 64 fullChunkSize 80 header 16 aarch64 (64-bit rpi4) +WARNING: chunkSize 304 fullChunkSize 320 header 16 +WARNING: chunkSize 80 fullChunkSize 96 header 16 So indeed, those are *perfect* matches and thus the sentinel_ok() never executed. So no failures until now. On x86-64 I get the same thing as on aarch64. I guess that explains why it never failed. Seems like a pretty amazing coincidence ... > I'm also wondering if this should also be backpatched back to v10, > providing the build farm likes it well enough on master. I'd say the sentinel fix may need to be backpatched. regard -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
