public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: Richard Guo <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: David Rowley <[email protected]>
Subject: Re: An oversight in ExecInitAgg for grouping sets
Date: Mon, 02 Jan 2023 16:25:19 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAMbWs49nMd1up4BOq=gXutTJintTUpe1u3qxvhRhPj9=9h5U=g@mail.gmail.com>
References: <CAMbWs4-vZuuPOZsKOYnSAaPYGKhmacxhki+vpOKk0O7rymccXQ@mail.gmail.com>
	<CAMbWs49nMd1up4BOq=gXutTJintTUpe1u3qxvhRhPj9=9h5U=g@mail.gmail.com>

Richard Guo <[email protected]> writes:
> On Thu, Dec 22, 2022 at 2:02 PM Richard Guo <[email protected]> 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

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: An oversight in ExecInitAgg for grouping sets
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox