Re: [PATCH] Refactor bytea_sortsupport(), take two

Hi Aleksander,

Overall LGTM. Just a few small comments:

> On Sep 15, 2025, at 16:40, Aleksander Alekseev <aleksander(at)tigerdata(dot)com> wrote:
>
> --
> Best regards,
> Aleksander Alekseev
> <v3-0001-Refactor-bytea_sortsupport.patch>

1.
····
+ /* We can't afford to leak memory here. */
+ if (PointerGetDatum(arg1) != x)
+ pfree(arg1);
+ if (PointerGetDatum(arg2) != y)
+ pfree(arg2);
···

Should we do:
```
PG_FREE_IF_COPY(arg1, 0);
PG_FREE_IF_COPY(arg2, 1)
```

Similar to other places.

2.
···
+/*
+ * Conversion routine for sortsupport.
+ */
+static Datum
+bytea_abbrev_convert(Datum original, SortSupport ssup)
···

The function comment is less descriptive. I would suggest something like:
```
/*
* Abbreviated key conversion for bytea sortsupport.
*
* Returns the abbreviated key as a Datum. If a detoasted copy was made,
* it is freed before returning.
*/
```

3.
```
+ if (abbrev_distinct <= 1.0)
+ abbrev_distinct = 1.0;
+
+ if (key_distinct <= 1.0)
+ key_distinct = 1.0;
```

Why <= 1.0 then set to 1.0? Shouldn't “if" takes just <1.0?

4.
```
Datum
bytea_sortsupport(PG_FUNCTION_ARGS)
{
SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
MemoryContext oldcontext;
+ ByteaSortSupport *bss;
```

“Bss” can be defined in the “if” clause where it is used.

5.
```
Datum
bytea_sortsupport(PG_FUNCTION_ARGS)
{
SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
MemoryContext oldcontext;
+ ByteaSortSupport *bss;
+ bool abbreviate = ssup->abbreviate;
```

The local variable “abbreviate” is only used once, do we really need to cache ssup->abbreviate into a local variable?

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/