Re: A qsort template

Hi,

On 2022-04-02 15:20:27 -0500, Justin Pryzby wrote:
> On Sat, Apr 02, 2022 at 06:41:30PM +0700, John Naylor wrote:
> > On Sat, Apr 2, 2022 at 5:27 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> > > Reproduced locally, using the same few lines from the cluster.sql
> > > test. I'll try to dig more tomorrow...
> >
> > Thanks! Unfortunately I can't reproduce locally with clang 13/gcc 11,
> > with -Og or -O2 with CFLAGS="-fsanitize=undefined,alignment" ...
>
> Like Thomas just said, I had to use:
> CFLAGS="-Og -fsanitize=undefined,alignment -fno-sanitize-recover=all
>
> I'm a couple few steps out of my league here, but it may be an issue with:
>
> commit 4ea51cdfe85ceef8afabceb03c446574daa0ac23
> Author: Robert Haas <rhaas(at)postgresql(dot)org>
> Date: Mon Jan 19 15:20:31 2015 -0500
>
> Use abbreviated keys for faster sorting of text datums.
>
> This is enough to avoid the crash, which might be a useful hint..
>
> @@ -4126,22 +4126,23 @@ copytup_cluster(Tuplesortstate *state, SortTuple *stup, void *tup)
> /*
> * set up first-column key value, and potentially abbreviate, if it's a
> * simple column
> */
> + stup->isnull1 = false;
> if (state->indexInfo->ii_IndexAttrNumbers[0] == 0)
> return;
>
> original = heap_getattr(tuple,
> state->indexInfo->ii_IndexAttrNumbers[0],
> state->tupDesc,
> &stup->isnull1);

I don't think that can be correct - the column can be NULL afaics. And I don't
think in that patch it's needed, because it always goes through ->comparetup()
when state->onlyKey isn't explicitly set. Which tuplesort_begin_cluster() as
well as several others don't. And you'd just sort an uninitialized datum
immediately after.

It's certainly not pretty that copytup_cluster() can use SortTuples without
actually using SortTuples. Afaics it basically only computes isnull1/datum1 if
state->indexInfo->ii_IndexAttrNumbers[0] == 0.

Greetings,

Andres Freund