MIME-Version: 1.0
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>
 <20220402202027.GG28503@telsasoft.com> <20220402210333.bwf4h375p4fxt5j6@alap3.anarazel.de>
 <CA+hUKGLNpDXytCwUnOQYvGkvjR4h+0ev=ivqLGuUO-w0WAJ_7A@mail.gmail.com>
 <20220402231151.635ohpc4q3gppdzb@alap3.anarazel.de> <CA+hUKG+n94QKCYZTO7Esu2L5y8mt8GsDm=3Ke__jkkntVpjnQA@mail.gmail.com>
 <20220403163256.ythjrgtwe2nbwajc@alap3.anarazel.de> <CA+hUKGLvvAiHy9AHPUWFR1uLskATvqkofjD7nxgcPtJD1tdojQ@mail.gmail.com>
 <CA+hUKGJRbzaAOUtBUcjF5hLtaSHnJUqXmtiaLEoi53zeWSizeA@mail.gmail.com>
 <CAApHDvoO0sJiGgqK3vx=XX4TF6qik3YbLT-XAfvQLW6uq=t-NQ@mail.gmail.com> <CAFBsxsEJxfQcMfCjGi+Vbkqf0SnpCW65vfh4SMySYo8NaBTLhg@mail.gmail.com>
In-Reply-To: <CAFBsxsEJxfQcMfCjGi+Vbkqf0SnpCW65vfh4SMySYo8NaBTLhg@mail.gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Tue, 19 Apr 2022 17:29:56 +1200
Message-ID: <CAApHDvqWQJrAwLBveQLTfcB46NSer7qRLTe_NJF2cPuXziu4oA@mail.gmail.com>
Subject: Re: A qsort template
To: John Naylor <john.naylor@enterprisedb.com>
Cc: Thomas Munro <thomas.munro@gmail.com>, Andres Freund <andres@anarazel.de>, 
	Justin Pryzby <pryzby@telsasoft.com>, Peter Geoghegan <pg@bowt.ie>, 
	pgsql-hackers <pgsql-hackers@postgresql.org>, Robert Haas <rhaas@postgresql.org>
Content-Type: multipart/mixed; boundary="0000000000007fea7005dcfb2bd4"
Archived-At: <https://www.postgresql.org/message-id/CAApHDvqWQJrAwLBveQLTfcB46NSer7qRLTe_NJF2cPuXziu4oA%40mail.gmail.com>
Precedence: bulk

--0000000000007fea7005dcfb2bd4
Content-Type: text/plain; charset="UTF-8"

Thanks for looking at this.

On Tue, 19 Apr 2022 at 02:11, John Naylor <john.naylor@enterprisedb.com> 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

--0000000000007fea7005dcfb2bd4
Content-Type: text/plain; charset="US-ASCII"; 
	name="skip_calling_sort_tiebreak_when_not_needed2.patch"
Content-Disposition: attachment; 
	filename="skip_calling_sort_tiebreak_when_not_needed2.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_l25pd7ds0>
X-Attachment-Id: f_l25pd7ds0

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3V0aWxzL3NvcnQvdHVwbGVzb3J0LmMgYi9zcmMvYmFj
a2VuZC91dGlscy9zb3J0L3R1cGxlc29ydC5jCmluZGV4IDExNzRlMWEzMWMuLjEyZDFiZjU1MWIg
MTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxzL3NvcnQvdHVwbGVzb3J0LmMKKysrIGIvc3Jj
L2JhY2tlbmQvdXRpbHMvc29ydC90dXBsZXNvcnQuYwpAQCAtNDM2LDcgKzQzNiwxMCBAQCBzdHJ1
Y3QgVHVwbGVzb3J0c3RhdGUKIAogCS8qCiAJICogVGhpcyB2YXJpYWJsZSBpcyBzaGFyZWQgYnkg
dGhlIHNpbmdsZS1rZXkgTWluaW1hbFR1cGxlIGNhc2UgYW5kIHRoZQotCSAqIERhdHVtIGNhc2Ug
KHdoaWNoIGJvdGggdXNlIHFzb3J0X3NzdXAoKSkuICBPdGhlcndpc2UgaXQncyBOVUxMLgorCSAq
IERhdHVtIGNhc2UgKHdoaWNoIGJvdGggdXNlIHFzb3J0X3NzdXAoKSkuICBJdCBpcyBhbHNvIHVz
ZWQgYnkgdmFyaW91cworCSAqIHNvcnQgc3BlY2lhbGl6YXRpb24gZnVuY3Rpb25zIHdoZW4gY29t
cGFyaW5nIHRoZSBsZWFkaW5nIGtleSBpbiBhCisJICogdGllYnJlYWsgc2l0dWF0aW9uIHRvIGRl
dGVybWluZSBpZiB0aGVyZSBhcmUgYW55IHN1YnNlcXVlbnQga2V5cyB0bworCSAqIHNvcnQgb24u
IEl0J3Mgb3RoZXJ3aXNlIE5VTEwuCiAJICovCiAJU29ydFN1cHBvcnQgb25seUtleTsKIApAQCAt
Njk4LDcgKzcwMSwxMiBAQCBxc29ydF90dXBsZV91bnNpZ25lZF9jb21wYXJlKFNvcnRUdXBsZSAq
YSwgU29ydFR1cGxlICpiLCBUdXBsZXNvcnRzdGF0ZSAqc3RhdGUpCiAJY29tcGFyZSA9IEFwcGx5
VW5zaWduZWRTb3J0Q29tcGFyYXRvcihhLT5kYXR1bTEsIGEtPmlzbnVsbDEsCiAJCQkJCQkJCQkJ
ICBiLT5kYXR1bTEsIGItPmlzbnVsbDEsCiAJCQkJCQkJCQkJICAmc3RhdGUtPnNvcnRLZXlzWzBd
KTsKLQlpZiAoY29tcGFyZSAhPSAwKQorCisJLyoKKwkgKiBObyBuZWVkIHRvIGNhbGwgdGhlIHRp
ZWJyZWFrIGZ1bmN0aW9uIHdoZW4gdGhlIGRhdHVtcyBkaWZmZXIgb3IgaWYgdGhpcworCSAqIGlz
IHRoZSBvbmx5IGtleSB3ZSdyZSBzb3J0aW5nIG9uLgorCSAqLworCWlmIChjb21wYXJlICE9IDAg
fHwgc3RhdGUtPm9ubHlLZXkgIT0gTlVMTCkKIAkJcmV0dXJuIGNvbXBhcmU7CiAKIAlyZXR1cm4g
c3RhdGUtPmNvbXBhcmV0dXAoYSwgYiwgc3RhdGUpOwpAQCAtNzEzLDcgKzcyMSwxMiBAQCBxc29y
dF90dXBsZV9zaWduZWRfY29tcGFyZShTb3J0VHVwbGUgKmEsIFNvcnRUdXBsZSAqYiwgVHVwbGVz
b3J0c3RhdGUgKnN0YXRlKQogCWNvbXBhcmUgPSBBcHBseVNpZ25lZFNvcnRDb21wYXJhdG9yKGEt
PmRhdHVtMSwgYS0+aXNudWxsMSwKIAkJCQkJCQkJCQliLT5kYXR1bTEsIGItPmlzbnVsbDEsCiAJ
CQkJCQkJCQkJJnN0YXRlLT5zb3J0S2V5c1swXSk7Ci0JaWYgKGNvbXBhcmUgIT0gMCkKKworCS8q
CisJICogTm8gbmVlZCB0byBjYWxsIHRoZSB0aWVicmVhayBmdW5jdGlvbiB3aGVuIHRoZSBkYXR1
bXMgZGlmZmVyIG9yIGlmIHRoaXMKKwkgKiBpcyB0aGUgb25seSBrZXkgd2UncmUgc29ydGluZyBv
bi4KKwkgKi8KKwlpZiAoY29tcGFyZSAhPSAwIHx8IHN0YXRlLT5vbmx5S2V5ICE9IE5VTEwpCiAJ
CXJldHVybiBjb21wYXJlOwogCiAJcmV0dXJuIHN0YXRlLT5jb21wYXJldHVwKGEsIGIsIHN0YXRl
KTsKQEAgLTcyOCw3ICs3NDEsMTIgQEAgcXNvcnRfdHVwbGVfaW50MzJfY29tcGFyZShTb3J0VHVw
bGUgKmEsIFNvcnRUdXBsZSAqYiwgVHVwbGVzb3J0c3RhdGUgKnN0YXRlKQogCWNvbXBhcmUgPSBB
cHBseUludDMyU29ydENvbXBhcmF0b3IoYS0+ZGF0dW0xLCBhLT5pc251bGwxLAogCQkJCQkJCQkJ
CWItPmRhdHVtMSwgYi0+aXNudWxsMSwKIAkJCQkJCQkJCQkmc3RhdGUtPnNvcnRLZXlzWzBdKTsK
LQlpZiAoY29tcGFyZSAhPSAwKQorCisJLyoKKwkgKiBObyBuZWVkIHRvIGNhbGwgdGhlIHRpZWJy
ZWFrIGZ1bmN0aW9uIHdoZW4gdGhlIGRhdHVtcyBkaWZmZXIgb3IgaWYgdGhpcworCSAqIGlzIHRo
ZSBvbmx5IGtleSB3ZSdyZSBzb3J0aW5nIG9uLgorCSAqLworCWlmIChjb21wYXJlICE9IDAgfHwg
c3RhdGUtPm9ubHlLZXkgIT0gTlVMTCkKIAkJcmV0dXJuIGNvbXBhcmU7CiAKIAlyZXR1cm4gc3Rh
dGUtPmNvbXBhcmV0dXAoYSwgYiwgc3RhdGUpOwo=
--0000000000007fea7005dcfb2bd4--