Обсуждение: An oversight in ExecInitAgg for grouping sets

Richard Guo <guofenglinux@gmail.com> writes:
> On Thu, Dec 22, 2022 at 2:02 PM Richard Guo <guofenglinux@gmail.com> wrote:
>> If it is an empty grouping set, its length will be zero, and accessing
>> phasedata->eqfunctions[length - 1] is not right.

> Attached is a trivial patch for the fix.

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.

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.

            regards, tom lane

On Tue, 3 Jan 2023 at 10:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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, that probably could have been improved during the recent change.
Here's a patch for it.

I'm just doing a final Valgrind run on it now to check for errors.

David

On Thu, 5 Jan 2023 at 20:06, Richard Guo <guofenglinux@gmail.com> wrote:
> I reviewed this patch and have some comments.

Thanks for looking at this. I think I've fixed all the issues you mentioned.

One extra thing I noticed was that I had to add a new
VALGRIND_MAKE_MEM_DEFINED in AllocSetAlloc when grabbing an item off
the freelist. I didn't quite manage to figure out why that's needed as
when we do AllocSetFree() we don't mark the pfree'd memory with
NOACCESS, and it also looks like AllocSetReset() sets the keeper
block's memory to NOACCESS, but that function also clears the
freelists too, so the freelist chunk is not coming from a recently
reset context.

I might need to spend a bit more time on this to see if I can figure
out why this is happening.  On the other hand, maybe we should just
mark pfree'd memory as NOACCESS as that might find another class of
issues.

David

On Mon, 20 Mar 2023 at 19:18, Richard Guo <guofenglinux@gmail.com> wrote:
> It occurred to me that this hasn't been applied.  Should we add it to
> the CF to not lose track of it?

I have a git branch with it. That'll work for me personally as a
reminder to come back to it during the v17 cycle.

David

On Mon, 9 Jan 2023 at 22:21, David Rowley <dgrowleyml@gmail.com> wrote:
> One extra thing I noticed was that I had to add a new
> VALGRIND_MAKE_MEM_DEFINED in AllocSetAlloc when grabbing an item off
> the freelist. I didn't quite manage to figure out why that's needed as
> when we do AllocSetFree() we don't mark the pfree'd memory with
> NOACCESS, and it also looks like AllocSetReset() sets the keeper
> block's memory to NOACCESS, but that function also clears the
> freelists too, so the freelist chunk is not coming from a recently
> reset context.

It seems I didn't look hard enough for NOACCESS marking. It's in
wipe_mem(). So that explains why the VALGRIND_MAKE_MEM_DEFINED is
required in AllocSetAlloc.

Since this patch really only touches Valgrind macros, I don't really
feel like there's a good reason we can't still do this for v16, but
I'll start another thread to increase visibility to see if anyone else
thinks differently about that.

David