Re: failures with tuplesort and ordered set aggregates (due to 5cefbf5a6c44)

On Fri, Feb 20, 2015 at 4:01 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Fri, Feb 20, 2015 at 11:58 AM, Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>> This seems to happen because ordered_set_startup() calls
>> tuplesort_begin_datum() when (use_tuples == true), which only sets
>> 'onlyKey' and leaves (sortKeys == NULL). So 'mergeruns' fails because it
>> does not expect that.
>
> Oops. You're right. Attached patch fixes the issue, by making
> tuplesort_begin_datum() consistent with other comparable routines for
> other tuple cases. I think it makes sense to have the 'sortKeys' field
> always set, and the 'onlyKey' field also set when that additional
> optimization makes sense. That was what I understood the API to be, so
> arguably this is a pre-existing issue with tuplesort_begin_datum().

This doesn't completely fix the issue. Even with this patch applied:

CREATE TABLE stuff AS SELECT random()::text AS randtext FROM
generate_series(1,50000000);
set maintenance_work_mem='1024';
create index on stuff using hash (randtext);
...wait for a bit, server crash...

I find your statement that this is a pre-existing issue in
tuplesort_begin_datum() to be pretty misleading, unless what you mean
by it is "pre-existing since November, when an earlier patch by Peter
Geoghegan changed the comments without changing the code to match".
Commit 5ea86e6e65dd2da3e9a3464484985d48328e7fe3, changed the comments
for the sortKey field to say that it would be set in all cases except
for the hash index case, but it did not make that statement true.
Subsequently, another of your patches, which I committed as
5cefbf5a6c4466ac6b1cc2a4316b4eba9108c802, added a dependency on
state->sortKeys always being set. Now you've got this patch here,
which you claim makes sure that sortKeys is always set, but it
actually doesn't, which is why the above still crashes. There are not
so many tuplesort_begin_xxx routines that you couldn't audit them all
before submitting your patches.

Anyway, I propose the attached instead, which makes both Tomas's test
case and the one above pass.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company