From: Tom Lane <tgl@sss.pgh.pa.us>
To: Richard Guo <guofenglinux@gmail.com>
cc: PostgreSQL-development <pgsql-hackers@postgresql.org>,
        David Rowley <dgrowleyml@gmail.com>
Subject: Re: An oversight in ExecInitAgg for grouping sets
In-reply-to: <CAMbWs49nMd1up4BOq=gXutTJintTUpe1u3qxvhRhPj9=9h5U=g@mail.gmail.com>
References: <CAMbWs4-vZuuPOZsKOYnSAaPYGKhmacxhki+vpOKk0O7rymccXQ@mail.gmail.com> <CAMbWs49nMd1up4BOq=gXutTJintTUpe1u3qxvhRhPj9=9h5U=g@mail.gmail.com>
Comments: In-reply-to Richard Guo <guofenglinux@gmail.com>
	message dated "Tue, 27 Dec 2022 15:12:17 +0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1650234.1672694719.1@sss.pgh.pa.us>
Date: Mon, 02 Jan 2023 16:25:19 -0500
Message-ID: <1650235.1672694719@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/1650235.1672694719%40sss.pgh.pa.us>
Precedence: bulk

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