On Sun, Apr 3, 2022 at 9:03 AM Andres Freund <and...@anarazel.de> wrote:
> It's certainly not pretty that copytup_cluster() can use SortTuples without
> actually using SortTuples. Afaics it basically only computes isnull1/datum1 if
> state->indexInfo->ii_IndexAttrNumbers[0] == 0.
I think we just need to decide up front if we're in a situation that
can't provide datum1/isnull1 (in this case because it's an expression
index), and skip the optimised paths. Here's an experimental patch...
still looking into whether there are more cases like this...
(There's also room to recognise when you don't even need to look at
isnull1 for a less branchy optimised sort, but that was already
discussed and put off for later.)
From a8a7c9ec4f0b96ed9d889d008731864f3d4e87d1 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Sun, 3 Apr 2022 09:41:04 +1200
Subject: [PATCH] WIP: Fix tuplesort optimizations for expression-based
CLUSTER.
---
src/backend/utils/sort/tuplesort.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 361527098f..34714d9baf 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -268,6 +268,8 @@ struct Tuplesortstate
MemoryContext tuplecontext; /* sub-context of sortcontext for tuple data */
LogicalTapeSet *tapeset; /* logtape.c object for tapes in a temp file */
+ bool disable_datum1; /* disable leading value-based optimizations */
+
/*
* These function pointers decouple the routines that must know what kind
* of tuple we are sorting from the routines that don't need to know it.
@@ -1095,6 +1097,13 @@ tuplesort_begin_cluster(TupleDesc tupDesc,
state->indexInfo = BuildIndexInfo(indexRel);
+ /*
+ * If we don't have a simple attribute, disable the use of datum1/isnull1.
+ * copytup_cluster() doesn't know how to compute expressions.
+ */
+ if (state->indexInfo->ii_IndexAttrNumbers[0] == 0)
+ state->disable_datum1 = true;
+
state->tupDesc = tupDesc; /* assume we need not copy tupDesc */
indexScanKey = _bt_mkscankey(indexRel, NULL);
@@ -3593,20 +3602,27 @@ tuplesort_sort_memtuples(Tuplesortstate *state)
if (state->memtupcount > 1)
{
- /* Do we have a specialization for the leading column's comparator? */
+ /*
+ * Do we have a specialization for the leading column's comparator,
+ * and has the leading column's value or abbreviation been stored in
+ * datum1/isnull1?
+ */
if (state->sortKeys &&
+ !state->disable_datum1 &&
state->sortKeys[0].comparator == ssup_datum_unsigned_cmp)
{
elog(DEBUG1, "qsort_tuple_unsigned");
qsort_tuple_unsigned(state->memtuples, state->memtupcount, state);
}
else if (state->sortKeys &&
+ !state->disable_datum1 &&
state->sortKeys[0].comparator == ssup_datum_signed_cmp)
{
elog(DEBUG1, "qsort_tuple_signed");
qsort_tuple_signed(state->memtuples, state->memtupcount, state);
}
else if (state->sortKeys &&
+ !state->disable_datum1 &&
state->sortKeys[0].comparator == ssup_datum_int32_cmp)
{
elog(DEBUG1, "qsort_tuple_int32");
@@ -4134,7 +4150,7 @@ copytup_cluster(Tuplesortstate *state, SortTuple *stup, void *tup)
* set up first-column key value, and potentially abbreviate, if it's a
* simple column
*/
- if (state->indexInfo->ii_IndexAttrNumbers[0] == 0)
+ if (state->disable_datum1)
return;
original = heap_getattr(tuple,
--
2.35.1
