Horton

Thread

Commits

  1. Fix theoretical bug in tuplesort

  1. Is tuplesort meant to support bounded datum sorts?

    David Rowley <dgrowleyml@gmail.com> — 2021-07-12T13:22:26Z

    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

  2. Re: Is tuplesort meant to support bounded datum sorts?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-12T16:10:24Z

    David Rowley <dgrowleyml@gmail.com> writes:
> 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.

Yeah, I think you're right.  The comment seems a little confused
though.  Maybe there's no need for it at all --- there's equivalent
code in e.g. writetup_datum that has no comment.

			regards, tom lane

  3. Re: Is tuplesort meant to support bounded datum sorts?

    David Rowley <dgrowleyml@gmail.com> — 2021-07-13T01:36:21Z

    On Tue, 13 Jul 2021 at 04:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > 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.
>
> Yeah, I think you're right.  The comment seems a little confused
> though.  Maybe there's no need for it at all --- there's equivalent
> code in e.g. writetup_datum that has no comment.

Thanks for looking at this.  I've pushed a fix and backpatched.

David