Обсуждение: pgsql: Fix bug #16784 in Disk-based Hash Aggregation.

Fix bug #16784 in Disk-based Hash Aggregation.

Before processing tuples, agg_refill_hash_table() was setting all
pergroup pointers to NULL to signal to advance_aggregates() that it
should not attempt to advance groups that had spilled.

The problem was that it also set the pergroups for sorted grouping
sets to NULL, which caused rescanning to fail.

Instead, change agg_refill_hash_table() to only set the pergroups for
hashed grouping sets to NULL; and when compiling the expression, pass
doSort=false.

Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/16784-7ff169bf2c3d1588%40postgresql.org
Backpatch-through: 13

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/05c0258966b502fae1bd63dcbe74d52f5c6f6948

Modified Files
--------------
src/backend/executor/nodeAgg.c             |  24 +++-
src/test/regress/expected/groupingsets.out | 207 +++++++++++++++++++++++++++++
src/test/regress/sql/groupingsets.sql      |  21 +++
3 files changed, 245 insertions(+), 7 deletions(-)

Jeff Davis <jdavis@postgresql.org> writes:
> Fix bug #16784 in Disk-based Hash Aggregation.

Buildfarm thinks this new test case is not completely stable.

It appears that only 32-bit machines are failing, and it's
not totally consistent even for them.  Dunno what to make of it.

            regards, tom lane

On Sun, 2020-12-27 at 02:09 -0500, Tom Lane wrote:
> Jeff Davis <jdavis@postgresql.org> writes:
> > Fix bug #16784 in Disk-based Hash Aggregation.
> 
> Buildfarm thinks this new test case is not completely stable.
> 
> It appears that only 32-bit machines are failing, and it's
> not totally consistent even for them.  Dunno what to make of it.

I think I just need to disable sort. Because there is a () group, we
don't need a Sort in the plan to exercise the bug.

Committed to master. I'll backport if the tests stabilize. I'm on
vacation now and will return on the 5th, so hopefully we're in a
reasonable state until then. If not, I can just revert my fix and deal
with it when I get back.

Regards,
    Jeff Davis

On Sun, 2020-12-27 at 09:59 -0800, Jeff Davis wrote:
> I think I just need to disable sort. Because there is a () group, we
> don't need a Sort in the plan to exercise the bug.

That still didn't stabilize the test, so I removed the EXPLAIN part of
the test and left the execution test in place.

The planning for grouping sets still uses work_mem in the knapsack
algorithm, so something about the other platforms is not considering
the plan with all hashes (except the () grouping set). I think we can
tune the test to be stable, but it doesn't seem critical so I think
we're fine just doing the execution-time test.

Regards,
    Jeff Davis