Horton

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

Chao Li <li.evan.chao@gmail.com>

View thread
From: Chao Li <li.evan.chao@gmail.com>
To: Aleksander Alekseev <aleksander@tigerdata.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, John Naylor <johncnaylorls@gmail.com>
Date: 2025-09-16T08:04:51Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Separate out bytea sort support from varlena.c

Hi Aleksander,

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/