Re: An oversight in ExecInitAgg for grouping sets

On Tue, Jan 3, 2023 at 5:25 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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