public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: John Naylor <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Peter Geoghegan <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Robert Haas <[email protected]>
Subject: Re: A qsort template
Date: Tue, 19 Apr 2022 17:29:56 +1200
Message-ID: <CAApHDvqWQJrAwLBveQLTfcB46NSer7qRLTe_NJF2cPuXziu4oA@mail.gmail.com> (raw)
In-Reply-To: <CAFBsxsEJxfQcMfCjGi+Vbkqf0SnpCW65vfh4SMySYo8NaBTLhg@mail.gmail.com>
References: <CAFBsxsEyWo3+eGMYy2w+q4WjGt5OxywNx-kDBHTT2zgQmQz1KA@mail.gmail.com>
<CA+hUKG+6Cj_yFgOiwSAqUCbysJuN9_HS3=ukUSV4ziY-irc26Q@mail.gmail.com>
<CAFBsxsGEixFbRyeO6wmbvOnR-Z70qZFdNkQwDQNeWfMpGYeFMw@mail.gmail.com>
<CA+hUKG+67T0nr1mcB2cFkKYktWc2TFh52QWfvGdvf259Ah+PhA@mail.gmail.com>
<CAFBsxsEsA2BjUcfWftLbaVMNs3Z_n2fZYHcErva9MdZcNYiYLA@mail.gmail.com>
<[email protected]>
<[email protected]>
<CA+hUKGLNpDXytCwUnOQYvGkvjR4h+0ev=ivqLGuUO-w0WAJ_7A@mail.gmail.com>
<[email protected]>
<CA+hUKG+n94QKCYZTO7Esu2L5y8mt8GsDm=3Ke__jkkntVpjnQA@mail.gmail.com>
<[email protected]>
<CA+hUKGLvvAiHy9AHPUWFR1uLskATvqkofjD7nxgcPtJD1tdojQ@mail.gmail.com>
<CA+hUKGJRbzaAOUtBUcjF5hLtaSHnJUqXmtiaLEoi53zeWSizeA@mail.gmail.com>
<CAApHDvoO0sJiGgqK3vx=XX4TF6qik3YbLT-XAfvQLW6uq=t-NQ@mail.gmail.com>
<CAFBsxsEJxfQcMfCjGi+Vbkqf0SnpCW65vfh4SMySYo8NaBTLhg@mail.gmail.com>
Thanks for looking at this.
On Tue, 19 Apr 2022 at 02:11, John Naylor <[email protected]> wrote:
> IIUC, this function is called by tuplesort_begin_common, which in turn
> is called by tuplesort_begin_{heap, indexes, etc}. The latter callers
> set the onlyKey and now oneKeySort variables as appropriate, and
> sometimes hard-coded to false. Is it intentional to set them here
> first?
>
> Falling under the polish that you were likely thinking of above:
I did put the patch together quickly just for the benchmark and at the
time I was subtly aware that the onlyKey field was being set using a
similar condition as I was using to set the boolean field I'd added.
On reflection today, it should be fine just to check if that field is
NULL or not in the 3 new comparison functions. Similarly to before,
this only needs to be done if the datums compare equally, so does not
add any code to the path where the datums are non-equal. It looks
like the other tuplesort_begin_* functions use a different comparison
function that will never make use of the specialization comparison
functions added by 697492434.
I separated out the "or" condition that I'd added tot he existing "if"
to make it easier to write a comment explaining why we can skip the
tiebreak function call.
Updated patch attached.
David
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 1174e1a31c..12d1bf551b 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -436,7 +436,10 @@ struct Tuplesortstate
/*
* This variable is shared by the single-key MinimalTuple case and the
- * Datum case (which both use qsort_ssup()). Otherwise it's NULL.
+ * Datum case (which both use qsort_ssup()). It is also used by various
+ * sort specialization functions when comparing the leading key in a
+ * tiebreak situation to determine if there are any subsequent keys to
+ * sort on. It's otherwise NULL.
*/
SortSupport onlyKey;
@@ -698,7 +701,12 @@ qsort_tuple_unsigned_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
compare = ApplyUnsignedSortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
@@ -713,7 +721,12 @@ qsort_tuple_signed_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
compare = ApplySignedSortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
@@ -728,7 +741,12 @@ qsort_tuple_int32_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
compare = ApplyInt32SortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
Attachments:
[text/plain] skip_calling_sort_tiebreak_when_not_needed2.patch (2.1K, ../CAApHDvqWQJrAwLBveQLTfcB46NSer7qRLTe_NJF2cPuXziu4oA@mail.gmail.com/2-skip_calling_sort_tiebreak_when_not_needed2.patch)
download | inline diff:
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 1174e1a31c..12d1bf551b 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -436,7 +436,10 @@ struct Tuplesortstate
/*
* This variable is shared by the single-key MinimalTuple case and the
- * Datum case (which both use qsort_ssup()). Otherwise it's NULL.
+ * Datum case (which both use qsort_ssup()). It is also used by various
+ * sort specialization functions when comparing the leading key in a
+ * tiebreak situation to determine if there are any subsequent keys to
+ * sort on. It's otherwise NULL.
*/
SortSupport onlyKey;
@@ -698,7 +701,12 @@ qsort_tuple_unsigned_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
compare = ApplyUnsignedSortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
@@ -713,7 +721,12 @@ qsort_tuple_signed_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
compare = ApplySignedSortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
@@ -728,7 +741,12 @@ qsort_tuple_int32_compare(SortTuple *a, SortTuple *b, Tuplesortstate *state)
compare = ApplyInt32SortComparator(a->datum1, a->isnull1,
b->datum1, b->isnull1,
&state->sortKeys[0]);
- if (compare != 0)
+
+ /*
+ * No need to call the tiebreak function when the datums differ or if this
+ * is the only key we're sorting on.
+ */
+ if (compare != 0 || state->onlyKey != NULL)
return compare;
return state->comparetup(a, b, state);
view thread (31+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: A qsort template
In-Reply-To: <CAApHDvqWQJrAwLBveQLTfcB46NSer7qRLTe_NJF2cPuXziu4oA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox