Is tuplesort meant to support bounded datum sorts?
David Rowley <dgrowleyml@gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
To: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Ronan Dunklau <ronan.dunklau@aiven.io>
Date: 2021-07-12T13:22:26Z
Lists: pgsql-hackers
Over on [1], Ronan is working on allowing Datum sorts for nodeSort.c
when we're just sorting a single Datum.
I was looking at his v4 patch and noticed that he'd modified
free_sort_tuple() to conditionally only free the sort tuple if it's
non-NULL. Without this change, the select.sql regression test fails
on:
select * from onek,
(values ((select i from
(values(10000), (2), (389), (1000), (2000), ((select 10029))) as foo(i)
order by i asc limit 1))) bar (i)
where onek.unique1 = bar.i;
The limit 1 makes this a bounded sort and we call free_sort_tuple()
during make_bounded_heap().
It looks like this has likely never come up before because the only
time we use tuplesort_set_bound() is in nodeSort.c and
nodeIncrementalSort.c, none of those currently use datum sorts.
However, I'm thinking this is still a bug that should be fixed
separately from Ronan's main patch.
Does anyone else have thoughts on this?
The fragment in question is:
@@ -4773,6 +4773,14 @@ leader_takeover_tapes(Tuplesortstate *state)
static void
free_sort_tuple(Tuplesortstate *state, SortTuple *stup)
{
- FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
- pfree(stup->tuple);
+ /*
+ * If the SortTuple is actually only a single Datum, which was not copied
+ * as it is a byval type, do not try to free it nor account for it in
+ * memory used.
+ */
+ if (stup->tuple)
+ {
+ FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
+ pfree(stup->tuple);
+ }
}
David
[1] https://www.postgresql.org/message-id/3060002.hb0XKQ11pn@aivenronan
Commits
-
Fix theoretical bug in tuplesort
- 87b7a652b3a2 9.6.23 landed
- b55e478a4c2c 10.18 landed
- 187e9c3996aa 11.13 landed
- 6f1c7a2d0fe0 12.8 landed
- 204f646a22f2 13.4 landed
- a3b8d91ccd79 14.0 landed
- 41469253e970 15.0 landed