On Tue, Jan 3, 2023 at 5:25 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Agreed, that's a latent bug. It's only latent because the word just > before a palloc chunk will never be zero, either in our historical > palloc code or in v16's shiny new implementation. Nonetheless it > is a horrible idea for ExecInitAgg to depend on that fact, so I > pushed your fix.
Thanks for pushing it! > The thing that I find really distressing here is that it's been > like this for years and none of our automated testing caught it. > You'd have expected valgrind testing to do so ... but it does not, > because we've never marked that word NOACCESS. Maybe we should > rethink that? It'd require making mcxt.c do some valgrind flag > manipulations so it could access the hdrmask when appropriate. Yeah, maybe we can do that. It's true that it requires some additional work to access hdrmask, as in the new implementation the palloc'd chunk is always prefixed by hdrmask. BTW, I noticed a typo in the comment of memorychunk.h. --- a/src/include/utils/memutils_memorychunk.h +++ b/src/include/utils/memutils_memorychunk.h @@ -5,9 +5,9 @@ * MemoryContexts may use as a header for chunks of memory they allocate. * * MemoryChunk provides a lightweight header that a MemoryContext can use to - * store a reference back to the block the which the given chunk is allocated - * on and also an additional 30-bits to store another value such as the size - * of the allocated chunk. + * store a reference back to the block which the given chunk is allocated on + * and also an additional 30-bits to store another value such as the size of + * the allocated chunk. Thanks Richard
