Hi Aleksander, Overall LGTM. Just a few small comments:
> On Sep 15, 2025, at 16:40, Aleksander Alekseev <[email protected]> > 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/
