Hi Andrey, Many thanks for your feedback!
> 1. The assignment ssup->ssup_extra = NULL can be removed. The > SortSupport struct is zeroed before the callback is called (see > sortsupport.h). There are about 22 similar assignments elsewhere; > it does not seem to be established practice, many other places have > no such assignment. Agree. I removed this assignment in 0001 and added 0002 that removes if for the rest of *_sortsupport() functions. -- Best regards, Aleksander Alekseev
From 2d3b1df3b946cad0f88624dc0ae5e07b6c28640b Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Mon, 9 Mar 2026 15:51:04 +0300 Subject: [PATCH v2 2/2] Remove redundant ssup_extra zeroing in *_sortsupport() SortSupportData is guaranteed to be zeroed by callers. Therefore, zeroing ssup_extra in *_sortsupport() functions is redundant. Remove corresponding code in order to make all the functions consistent. Author: Aleksander Alekseev <[email protected]> Suggested-by: Andrey Borodin <[email protected]> Reviewed-by: TODO FIXME Discussion: https://postgr.es/m/caj7c6tmk10rf_limz6j9rry1rqk-5s+wbpubeflix4cy+-4...@mail.gmail.com --- contrib/btree_gist/btree_bit.c | 2 -- contrib/btree_gist/btree_bool.c | 1 - contrib/btree_gist/btree_bytea.c | 1 - contrib/btree_gist/btree_cash.c | 1 - contrib/btree_gist/btree_date.c | 1 - contrib/btree_gist/btree_float4.c | 1 - contrib/btree_gist/btree_float8.c | 1 - contrib/btree_gist/btree_inet.c | 1 - contrib/btree_gist/btree_interval.c | 1 - contrib/btree_gist/btree_macaddr.c | 1 - contrib/btree_gist/btree_macaddr8.c | 1 - contrib/btree_gist/btree_numeric.c | 1 - contrib/btree_gist/btree_oid.c | 1 - contrib/btree_gist/btree_text.c | 2 -- contrib/btree_gist/btree_time.c | 1 - contrib/btree_gist/btree_ts.c | 1 - contrib/btree_gist/btree_uuid.c | 1 - src/backend/utils/adt/network.c | 1 - src/backend/utils/adt/rangetypes.c | 1 - src/backend/utils/adt/uuid.c | 1 - 20 files changed, 22 deletions(-) diff --git a/contrib/btree_gist/btree_bit.c b/contrib/btree_gist/btree_bit.c index 2b9c18a586f..3f6f73ac9dc 100644 --- a/contrib/btree_gist/btree_bit.c +++ b/contrib/btree_gist/btree_bit.c @@ -233,7 +233,6 @@ gbt_bit_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_bit_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } @@ -244,7 +243,6 @@ gbt_varbit_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_bit_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_bool.c b/contrib/btree_gist/btree_bool.c index 8e59523f474..7de93662c92 100644 --- a/contrib/btree_gist/btree_bool.c +++ b/contrib/btree_gist/btree_bool.c @@ -182,7 +182,6 @@ gbt_bool_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_bool_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_bytea.c b/contrib/btree_gist/btree_bytea.c index 50bb24308e9..221e4bd9490 100644 --- a/contrib/btree_gist/btree_bytea.c +++ b/contrib/btree_gist/btree_bytea.c @@ -187,7 +187,6 @@ gbt_bytea_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_bytea_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_cash.c b/contrib/btree_gist/btree_cash.c index a347b1320e4..13aacc0d762 100644 --- a/contrib/btree_gist/btree_cash.c +++ b/contrib/btree_gist/btree_cash.c @@ -237,7 +237,6 @@ gbt_cash_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_cash_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_date.c b/contrib/btree_gist/btree_date.c index 5e41f4574c5..74dccb9613b 100644 --- a/contrib/btree_gist/btree_date.c +++ b/contrib/btree_gist/btree_date.c @@ -273,7 +273,6 @@ gbt_date_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_date_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_float4.c b/contrib/btree_gist/btree_float4.c index c076918fd48..b694a175836 100644 --- a/contrib/btree_gist/btree_float4.c +++ b/contrib/btree_gist/btree_float4.c @@ -226,7 +226,6 @@ gbt_float4_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_float4_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_float8.c b/contrib/btree_gist/btree_float8.c index d7386e885a2..fb906a38610 100644 --- a/contrib/btree_gist/btree_float8.c +++ b/contrib/btree_gist/btree_float8.c @@ -234,7 +234,6 @@ gbt_float8_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_float8_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_inet.c b/contrib/btree_gist/btree_inet.c index 9d04b92d3b8..9684283fa4c 100644 --- a/contrib/btree_gist/btree_inet.c +++ b/contrib/btree_gist/btree_inet.c @@ -204,7 +204,6 @@ gbt_inet_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_inet_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_interval.c b/contrib/btree_gist/btree_interval.c index 6b81fa2b39b..f288d97ee09 100644 --- a/contrib/btree_gist/btree_interval.c +++ b/contrib/btree_gist/btree_interval.c @@ -315,7 +315,6 @@ gbt_intv_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_intv_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_macaddr.c b/contrib/btree_gist/btree_macaddr.c index df7c0040011..140f1ad4135 100644 --- a/contrib/btree_gist/btree_macaddr.c +++ b/contrib/btree_gist/btree_macaddr.c @@ -211,7 +211,6 @@ gbt_macaddr_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_macaddr_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_macaddr8.c b/contrib/btree_gist/btree_macaddr8.c index 3b0c9b81e85..8b2a01567b7 100644 --- a/contrib/btree_gist/btree_macaddr8.c +++ b/contrib/btree_gist/btree_macaddr8.c @@ -208,7 +208,6 @@ gbt_macad8_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_macaddr8_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_numeric.c b/contrib/btree_gist/btree_numeric.c index dba04c3a1b3..716b46c27ba 100644 --- a/contrib/btree_gist/btree_numeric.c +++ b/contrib/btree_gist/btree_numeric.c @@ -246,7 +246,6 @@ gbt_numeric_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_numeric_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_oid.c b/contrib/btree_gist/btree_oid.c index 3ddf2a993d4..70eb91d8781 100644 --- a/contrib/btree_gist/btree_oid.c +++ b/contrib/btree_gist/btree_oid.c @@ -236,7 +236,6 @@ gbt_oid_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_oid_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_text.c b/contrib/btree_gist/btree_text.c index 2ac12f1cab2..903d80a42d4 100644 --- a/contrib/btree_gist/btree_text.c +++ b/contrib/btree_gist/btree_text.c @@ -313,7 +313,6 @@ gbt_text_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_text_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } @@ -346,7 +345,6 @@ gbt_bpchar_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_bpchar_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_time.c b/contrib/btree_gist/btree_time.c index 9d23fb4b867..610cb90aa6f 100644 --- a/contrib/btree_gist/btree_time.c +++ b/contrib/btree_gist/btree_time.c @@ -341,7 +341,6 @@ gbt_time_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_timekey_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_ts.c b/contrib/btree_gist/btree_ts.c index 0f88d5d72f4..32d77c622af 100644 --- a/contrib/btree_gist/btree_ts.c +++ b/contrib/btree_gist/btree_ts.c @@ -412,7 +412,6 @@ gbt_ts_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_ts_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/contrib/btree_gist/btree_uuid.c b/contrib/btree_gist/btree_uuid.c index c891840fb25..a1345a8401f 100644 --- a/contrib/btree_gist/btree_uuid.c +++ b/contrib/btree_gist/btree_uuid.c @@ -251,7 +251,6 @@ gbt_uuid_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = gbt_uuid_ssup_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/src/backend/utils/adt/network.c b/src/backend/utils/adt/network.c index 3a2002097dd..223672ec8b4 100644 --- a/src/backend/utils/adt/network.c +++ b/src/backend/utils/adt/network.c @@ -435,7 +435,6 @@ network_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = network_fast_cmp; - ssup->ssup_extra = NULL; if (ssup->abbreviate) { diff --git a/src/backend/utils/adt/rangetypes.c b/src/backend/utils/adt/rangetypes.c index 06cc3af4f4a..883c0e40cf3 100644 --- a/src/backend/utils/adt/rangetypes.c +++ b/src/backend/utils/adt/rangetypes.c @@ -1467,7 +1467,6 @@ range_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = range_fast_cmp; - ssup->ssup_extra = NULL; PG_RETURN_VOID(); } diff --git a/src/backend/utils/adt/uuid.c b/src/backend/utils/adt/uuid.c index 6ee3752ac78..64519d4ead6 100644 --- a/src/backend/utils/adt/uuid.c +++ b/src/backend/utils/adt/uuid.c @@ -279,7 +279,6 @@ uuid_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = uuid_fast_cmp; - ssup->ssup_extra = NULL; if (ssup->abbreviate) { -- 2.43.0
From cee3499a36d8c5457fe8cedf538ed84e1b700c88 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Wed, 25 Feb 2026 14:40:26 +0300 Subject: [PATCH v2 1/2] Simplify SortSupport implementation for macaddr Since Datums are 64-bit values and MAC addresses have only 6 bytes, the abbreviated key contains the entire MAC address and is authoritative. Thus there is no need for cardinality estimation using HyperLogLog. Author: Aleksander Alekseev <[email protected]> Suggested-by: John Naylor <[email protected]> Reviewed-by: Andrey Borodin <[email protected]> Discussion: https://postgr.es/m/caj7c6tmk10rf_limz6j9rry1rqk-5s+wbpubeflix4cy+-4...@mail.gmail.com --- src/backend/utils/adt/mac.c | 101 ++---------------------------------- 1 file changed, 5 insertions(+), 96 deletions(-) diff --git a/src/backend/utils/adt/mac.c b/src/backend/utils/adt/mac.c index f14675dea40..b72dee37319 100644 --- a/src/backend/utils/adt/mac.c +++ b/src/backend/utils/adt/mac.c @@ -14,11 +14,9 @@ #include "postgres.h" #include "common/hashfn.h" -#include "lib/hyperloglog.h" #include "libpq/pqformat.h" #include "port/pg_bswap.h" #include "utils/fmgrprotos.h" -#include "utils/guc.h" #include "utils/inet.h" #include "utils/sortsupport.h" @@ -33,15 +31,6 @@ #define lobits(addr) \ ((unsigned long)(((addr)->d<<16)|((addr)->e<<8)|((addr)->f))) -/* sortsupport for macaddr */ -typedef struct -{ - int64 input_count; /* number of non-null values seen */ - bool estimating; /* true if estimating cardinality */ - - hyperLogLogState abbr_card; /* cardinality estimator */ -} macaddr_sortsupport_state; - static int macaddr_cmp_internal(macaddr *a1, macaddr *a2); static int macaddr_fast_cmp(Datum x, Datum y, SortSupport ssup); static bool macaddr_abbrev_abort(int memtupcount, SortSupport ssup); @@ -365,28 +354,13 @@ macaddr_sortsupport(PG_FUNCTION_ARGS) SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0); ssup->comparator = macaddr_fast_cmp; - ssup->ssup_extra = NULL; if (ssup->abbreviate) { - macaddr_sortsupport_state *uss; - MemoryContext oldcontext; - - oldcontext = MemoryContextSwitchTo(ssup->ssup_cxt); - - uss = palloc_object(macaddr_sortsupport_state); - uss->input_count = 0; - uss->estimating = true; - initHyperLogLog(&uss->abbr_card, 10); - - ssup->ssup_extra = uss; - ssup->comparator = ssup_datum_unsigned_cmp; ssup->abbrev_converter = macaddr_abbrev_convert; ssup->abbrev_abort = macaddr_abbrev_abort; ssup->abbrev_full_comparator = macaddr_fast_cmp; - - MemoryContextSwitchTo(oldcontext); } PG_RETURN_VOID(); @@ -406,61 +380,12 @@ macaddr_fast_cmp(Datum x, Datum y, SortSupport ssup) } /* - * Callback for estimating effectiveness of abbreviated key optimization. - * - * We pay no attention to the cardinality of the non-abbreviated data, because - * there is no equality fast-path within authoritative macaddr comparator. + * Abbreviation is never aborted for macaddr because the 6-byte MAC address + * fits entirely within a 64-bit Datum, making the abbreviated key authoritative. */ static bool macaddr_abbrev_abort(int memtupcount, SortSupport ssup) { - macaddr_sortsupport_state *uss = ssup->ssup_extra; - double abbr_card; - - if (memtupcount < 10000 || uss->input_count < 10000 || !uss->estimating) - return false; - - abbr_card = estimateHyperLogLog(&uss->abbr_card); - - /* - * If we have >100k distinct values, then even if we were sorting many - * billion rows we'd likely still break even, and the penalty of undoing - * that many rows of abbrevs would probably not be worth it. At this point - * we stop counting because we know that we're now fully committed. - */ - if (abbr_card > 100000.0) - { - if (trace_sort) - elog(LOG, - "macaddr_abbrev: estimation ends at cardinality %f" - " after " INT64_FORMAT " values (%d rows)", - abbr_card, uss->input_count, memtupcount); - uss->estimating = false; - return false; - } - - /* - * Target minimum cardinality is 1 per ~2k of non-null inputs. 0.5 row - * fudge factor allows us to abort earlier on genuinely pathological data - * where we've had exactly one abbreviated value in the first 2k - * (non-null) rows. - */ - if (abbr_card < uss->input_count / 2000.0 + 0.5) - { - if (trace_sort) - elog(LOG, - "macaddr_abbrev: aborting abbreviation at cardinality %f" - " below threshold %f after " INT64_FORMAT " values (%d rows)", - abbr_card, uss->input_count / 2000.0 + 0.5, uss->input_count, - memtupcount); - return true; - } - - if (trace_sort) - elog(LOG, - "macaddr_abbrev: cardinality %f after " INT64_FORMAT - " values (%d rows)", abbr_card, uss->input_count, memtupcount); - return false; } @@ -469,14 +394,13 @@ macaddr_abbrev_abort(int memtupcount, SortSupport ssup) * to abbreviated key representation. * * Packs the bytes of a 6-byte MAC address into a Datum and treats it as an - * unsigned integer for purposes of comparison. On a 64-bit machine, there - * will be two zeroed bytes of padding. The integer is converted to native - * endianness to facilitate easy comparison. + * unsigned integer for purposes of comparison. There will be two zeroed bytes + * of padding. The integer is converted to native endianness to facilitate + * easy comparison. */ static Datum macaddr_abbrev_convert(Datum original, SortSupport ssup) { - macaddr_sortsupport_state *uss = ssup->ssup_extra; macaddr *authoritative = DatumGetMacaddrP(original); Datum res; @@ -489,21 +413,6 @@ macaddr_abbrev_convert(Datum original, SortSupport ssup) "Datum is too small for macaddr"); memset(&res, 0, sizeof(res)); memcpy(&res, authoritative, sizeof(macaddr)); - uss->input_count += 1; - - /* - * Cardinality estimation. The estimate uses uint32, so XOR the two 32-bit - * halves together to produce slightly more entropy. The two zeroed bytes - * won't have any practical impact on this operation. - */ - if (uss->estimating) - { - uint32 tmp; - - tmp = DatumGetUInt32(res) ^ (uint32) (DatumGetUInt64(res) >> 32); - - addHyperLogLog(&uss->abbr_card, DatumGetUInt32(hash_uint32(tmp))); - } /* * Byteswap on little-endian machines. -- 2.43.0
