Re: An oversight in ExecInitAgg for grouping sets

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Richard Guo <guofenglinux(at)gmail(dot)com>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, David Rowley <dgrowleyml(at)gmail(dot)com>
Subject: Re: An oversight in ExecInitAgg for grouping sets
Date: 2023-01-02 21:25:19
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Thu, Dec 22, 2022 at 2:02 PM Richard Guo <guofenglinux(at)gmail(dot)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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Verite 2023-01-02 21:32:03 TAP tests for psql \g piped into program
Previous Message Dag Lem 2023-01-02 21:00:34 Re: daitch_mokotoff module