MIME-Version: 1.0
References: <CANWCAZYzx7a7E9AY16Jt_U3+GVKDADfgApZ-42SYNiig8dTnFA@mail.gmail.com>
 <CANWCAZbsx5VY-F5RVvZ5H1abBWcinMY90gNra6EbdGS1CTx=7g@mail.gmail.com>
 <CANWCAZaN8kW6o7Ymc_jzPO_Z5SqekQTTNra3xTGYTH+cjrVp8g@mail.gmail.com>
 <269A8FB9-6D43-43CF-A6FE-52D28CBDB8A9@Outlook.com> <CANWCAZZiMGj6QuHfyBPv9at7xn23NPAz4nit=G6fr26V+h8MKg@mail.gmail.com>
 <F7935986-4E3D-477C-B45C-878420B45C0D@gmail.com> <CANWCAZaKRzsYPn9YWRR8g5dkX-ep_jgyUb4r3KFekK5ZTNo7ew@mail.gmail.com>
 <606C775A-4C1A-482B-BE7D-2E7A46AE14B9@gmail.com> <CANWCAZaBeDPfF0sCdSssRXv6nBZeRP8Z=OO1731zsoUC58U5_Q@mail.gmail.com>
 <F7D64692-F979-4661-B91C-8A1FF09F69BD@gmail.com> <9D3D4647-868B-4562-B382-D201478AD67B@gmail.com>
 <CANWCAZZo5rgE4+NYYES1hLN9PvonXMH=K3Z7b0TKcCBNOAjaag@mail.gmail.com>
 <54659731-2232-4E74-9533-D136D01B153F@gmail.com> <CANWCAZYur4VDbx9zkpFM3mM4-Hj4_Vt1t=Z4TPhK7+3=T6BMOA@mail.gmail.com>
 <tencent_6EB1CE2270AFA3DD4164A5D2@qq.com> <CANWCAZYBgHCpS-w5E8+wJpfhwLK88Kys-5ukSta2gj4BRPiTSg@mail.gmail.com>
 <tencent_1565D02652E0DC26675AABE2@qq.com> <tencent_D9FB41A25D4A685F6176ED4A4A804C06DE06@qq.com>
In-Reply-To: <tencent_D9FB41A25D4A685F6176ED4A4A804C06DE06@qq.com>
From: John Naylor <johncnaylorls@gmail.com>
Date: Fri, 13 Feb 2026 09:32:29 +0700
Message-ID: <CANWCAZZA_AqWy7xw20O=jWG+t0Cy4rUnvs1RaAABvSp3iLnGhg@mail.gmail.com>
Subject: Re: tuple radix sort
To: cca5507 <cca5507@qq.com>
Cc: zengman <zengman@halodbtech.com>, 
	pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CANWCAZZA_AqWy7xw20O%3DjWG%2Bt0Cy4rUnvs1RaAABvSp3iLnGhg%40mail.gmail.com>
Precedence: bulk

On Wed, Feb 11, 2026 at 5:25=E2=80=AFPM zengman <zengman@halodbtech.com> wr=
ote:
> I'm wondering if we should replace `state->memtuples` with `data` in the =
`sort_byvalue_datum()` function here.
> ```
>         if (nulls_first)
>         {
>                 null_start =3D state->memtuples;

Sounds good.

On Wed, Feb 11, 2026 at 6:47=E2=80=AFPM cca5507 <cca5507@qq.com> wrote:
> +1. Now data =3D=3D state->memtuples, how about remove the parameter "dat=
a" and "n" and just
> get them from "Tuplesortstate"?

The parameter list currently looks like a sort function. I'm not sure
why you think that's objectionable?

> Some comments for v7:
>
> 1)
>
> ```
> /*
>  * Retrieve byte from datum, indexed by 'level': 0 for LSB, 7 for MSB
>  */
> static inline uint8
> current_byte(Datum key, int level)
> {
>         int                     shift =3D (SIZEOF_DATUM - 1 - level) * BI=
TS_PER_BYTE;
>
>         return (key >> shift) & 0xFF;
> }
> ```
>
> Maybe "0 for MSB, 7 for LSB"? If level =3D=3D 0, this function will retur=
n the Most Significant Byte.

Will fix, thanks.

> 2) radix_sort_tuple()
>
> ```
>                 size_t          end_offset =3D partitions[*rp].next_offse=
t;
>                 SortTuple  *partition_end =3D begin + end_offset;
>                 ptrdiff_t       num_elements =3D end_offset - start_offse=
t;
> ```
>
> Why the type of "num_elements" is "ptrdiff_t"? Maybe just "size_t"?

That's from upstream. It's pretty rare in our codebase for in-core
code, so I'll go ahead and change it.

> 3) tuplesort_sort_memtuples()
>
> ```
>                 /*
>                  * Do we have the leading column's value or abbreviation =
in datum1?
>                  */
>                 if (state->base.haveDatum1 && state->base.sortKeys)
>                 {
>                         SortSupportData ssup =3D state->base.sortKeys[0];
> ```
>
> I think we should avoid the copy of SortSupportData. We can just use a po=
inter.

Will do, for consistency.

> 4)
>
> Many places just consider "Datum" as integer, do we need to add a DatumGe=
tUInt**() for them?

It already does that for the case where it specifically needs uint32.
For the general case, I don't think so. We can treat a Datum as an
unsigned integer and don't care about the actual size or what it
contains. There is one exception that I can see: 0002 has a call to
pg_leftmost_one_pos64(), so a macro for that would be more appropriate
than an implicit cast.

Now I'm wondering if 0002 should normalize the datum as it determines
the common prefix. That should only matter for int32 where the input
has a mix of positive and negative, but it could be worth the
additional calculation. I'll hold off on committing 0002 for a bit
longer, but 0001 (plus the changes that I've agreed to) is still on
schedule.

Also, looking at commit 6aebedc38, it's preferred to use
"sizeof(Datum)" rather than the vestigial SIZEOF_DATUM, so I'll change
that in 0001/2.

--
John Naylor
Amazon Web Services