Overall LGTM. Just a few small comments:
On Sep 15, 2025, at 16:40, Aleksander Alekseev <aleksander@tigerdata.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/
