glibc qsort() vulnerability

Started by Mats Kindahlalmost 2 years ago61 messages
#1Mats Kindahl
mats@timescale.com
1 attachment(s)

Hi hackers,

There is a bug in glibc's qsort() algorithm that runs the risk of creating
an out-of-bounds error if the comparison function is not transitive, for
example, if subtraction is used so that it can create an overflow.

See
https://packetstormsecurity.com/files/176931/glibc-qsort-Out-Of-Bounds-Read-Write.html

I checked the existing functions in the latest version of Postgres source
code and most are safe, but there were a few ones that could lead to
overflow. I do not know if these can actually lead to problems, but better
safe than sorry, so I created a patch to fix those few cases and add a
comment to one case that was not clear that it could not overflow.

Best wishes,
Mats Kindahl, Timescale

Attachments:

0001-Ensure-comparison-functions-are-transitive.patchtext/x-patch; charset=US-ASCII; name=0001-Ensure-comparison-functions-are-transitive.patchDownload
From 83e2f14ab259f568034b07c2f99e34c6dff2c7b5 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: Ensure comparison functions are transitive

There are a few comparison functions to qsort() that are non-transitive
because they can cause an overflow. Fix these functions to instead use
normal comparisons and return -1, 0, or 1 explicitly.
---
 src/backend/access/spgist/spgtextproc.c |  6 +++++-
 src/backend/utils/cache/relcache.c      |  2 ++
 src/bin/pg_upgrade/function.c           | 10 +++++++---
 src/bin/psql/crosstabview.c             |  8 +++++++-
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..d0a2b4e6e1 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -325,7 +325,11 @@ cmpNodePtr(const void *a, const void *b)
 	const spgNodePtr *aa = (const spgNodePtr *) a;
 	const spgNodePtr *bb = (const spgNodePtr *) b;
 
-	return aa->c - bb->c;
+	if (aa->c > bb->c)
+		return 1;
+	if (aa->c < bb->c)
+		return -1;
+	return 0;
 }
 
 Datum
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..42e4359c7b 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4520,6 +4520,8 @@ AttrDefaultCmp(const void *a, const void *b)
 	const AttrDefault *ada = (const AttrDefault *) a;
 	const AttrDefault *adb = (const AttrDefault *) b;
 
+	/* Cannot overflow because standard conversions will convert to int and
+	 * sizeof(int16) < sizeof(int) */
 	return ada->adnum - adb->adnum;
 }
 
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index af998f74d3..c82ae11016 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -32,14 +32,18 @@ library_name_compare(const void *p1, const void *p2)
 	int			slen1 = strlen(str1);
 	int			slen2 = strlen(str2);
 	int			cmp = strcmp(str1, str2);
+	int lhsdb = ((const LibraryInfo *) p1)->dbnum;
+	int rhsdb = ((const LibraryInfo *) p2)->dbnum;
 
 	if (slen1 != slen2)
 		return slen1 - slen2;
 	if (cmp != 0)
 		return cmp;
-	else
-		return ((const LibraryInfo *) p1)->dbnum -
-			((const LibraryInfo *) p2)->dbnum;
+	if (lhsdb < rhsdb)
+		return -1;
+	if (lhsdb > rhsdb)
+		return 1;
+	return 0;
 }
 
 
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index c6116b7238..948ed9b1fe 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -709,5 +709,11 @@ pivotFieldCompare(const void *a, const void *b)
 static int
 rankCompare(const void *a, const void *b)
 {
-	return *((const int *) a) - *((const int *) b);
+	const int lhs = *(const int *) a;
+	const int rhs = *(const int *) b;
+	if (lhs < rhs)
+		return -1;
+	if (lhs > rhs)
+		return 1;
+	return 0;
 }
-- 
2.34.1

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Mats Kindahl (#1)
Re: glibc qsort() vulnerability

Mats Kindahl <mats@timescale.com> writes:

There is a bug in glibc's qsort() algorithm that runs the risk of creating
an out-of-bounds error if the comparison function is not transitive, for
example, if subtraction is used so that it can create an overflow.

We don't use glibc's qsort. Have you checked whether there's a
problem with the code we do use?

regards, tom lane

#3Nathan Bossart
nathandbossart@gmail.com
In reply to: Tom Lane (#2)
Re: glibc qsort() vulnerability

On Tue, Feb 06, 2024 at 10:11:16AM -0500, Tom Lane wrote:

Mats Kindahl <mats@timescale.com> writes:

There is a bug in glibc's qsort() algorithm that runs the risk of creating
an out-of-bounds error if the comparison function is not transitive, for
example, if subtraction is used so that it can create an overflow.

We don't use glibc's qsort. Have you checked whether there's a
problem with the code we do use?

Oh, interesting. I didn't know that! As of commit 6edd2b4, we've used an
in-tree qsort() for everything. port.h has the following line:

#define qsort(a,b,c,d) pg_qsort(a,b,c,d)

I see a handful of callers that use pg_qsort() directly, which IMO is odd.
I would've expected that to be something different if I didn't know about
that macro. Maybe we should change those to qsort()...

Even if the glibc issue doesn't apply to Postgres, I'm tempted to suggest
that we make it project policy that comparison functions must be
transitive. There might be no real issues today, but if we write all
comparison functions the way Mats is suggesting, it should be easier to
reason about overflow risks.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nathan Bossart (#3)
Re: glibc qsort() vulnerability

Nathan Bossart <nathandbossart@gmail.com> writes:

Even if the glibc issue doesn't apply to Postgres, I'm tempted to suggest
that we make it project policy that comparison functions must be
transitive. There might be no real issues today, but if we write all
comparison functions the way Mats is suggesting, it should be easier to
reason about overflow risks.

A comparison routine that is not is probably broken, agreed.
I didn't look through the details of the patch --- I was more
curious whether we had a version of the qsort bug, because
if we do, we should fix that too.

regards, tom lane

#5Nathan Bossart
nathandbossart@gmail.com
In reply to: Tom Lane (#4)
Re: glibc qsort() vulnerability

On Tue, Feb 06, 2024 at 03:55:58PM -0500, Tom Lane wrote:

A comparison routine that is not is probably broken, agreed.
I didn't look through the details of the patch --- I was more
curious whether we had a version of the qsort bug, because
if we do, we should fix that too.

+1

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#6Mats Kindahl
mats@timescale.com
In reply to: Tom Lane (#2)
Re: glibc qsort() vulnerability

On Tue, Feb 6, 2024 at 4:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Mats Kindahl <mats@timescale.com> writes:

There is a bug in glibc's qsort() algorithm that runs the risk of

creating

an out-of-bounds error if the comparison function is not transitive, for
example, if subtraction is used so that it can create an overflow.

We don't use glibc's qsort. Have you checked whether there's a
problem with the code we do use?

Interesting. No, haven't checked. Will do that.

Best wishes,
Mats Kindahl

Show quoted text

regards, tom lane

#7Mats Kindahl
mats@timescale.com
In reply to: Tom Lane (#4)
Re: glibc qsort() vulnerability

On Tue, Feb 6, 2024 at 9:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

Even if the glibc issue doesn't apply to Postgres, I'm tempted to suggest
that we make it project policy that comparison functions must be
transitive. There might be no real issues today, but if we write all
comparison functions the way Mats is suggesting, it should be easier to
reason about overflow risks.

A comparison routine that is not is probably broken, agreed.
I didn't look through the details of the patch --- I was more
curious whether we had a version of the qsort bug, because
if we do, we should fix that too.

The patch basically removes the risk of overflow in three routines and just
returns -1, 0, or 1, and adds a comment in one.

The routines modified do a subtraction of int:s and return that, which can
cause an overflow. This method is used for some int16 as well but since
standard conversions in C will perform the arithmetics in "int" precision,
this cannot overflow, so added a comment there. It might still be a good
idea to follow the same pattern for the int16 routines, but since there is
no bug there, I did not add them to the patch.

Best wishes,
Mats Kindahl

Show quoted text

regards, tom lane

#8Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Mats Kindahl (#7)
Re: glibc qsort() vulnerability

On 07/02/2024 11:09, Mats Kindahl wrote:

On Tue, Feb 6, 2024 at 9:56 PM Tom Lane <tgl@sss.pgh.pa.us
<mailto:tgl@sss.pgh.pa.us>> wrote:

Nathan Bossart <nathandbossart@gmail.com
<mailto:nathandbossart@gmail.com>> writes:

Even if the glibc issue doesn't apply to Postgres, I'm tempted to

suggest

that we make it project policy that comparison functions must be
transitive.  There might be no real issues today, but if we write all
comparison functions the way Mats is suggesting, it should be

easier to

reason about overflow risks.

A comparison routine that is not is probably broken, agreed.
I didn't look through the details of the patch --- I was more
curious whether we had a version of the qsort bug, because
if we do, we should fix that too.

The patch basically removes the risk of overflow in three routines and
just returns -1, 0, or 1, and adds a comment in one.

The routines modified do a subtraction of int:s and return that, which
can cause an overflow. This method is used for some int16 as well but
since standard conversions in C will perform the arithmetics in "int"
precision, this cannot overflow, so added a comment there. It might
still be a good idea to follow the same pattern for the int16 routines,
but since there is no bug there, I did not add them to the patch.

Doesn't hurt to fix the comparison functions, and +1 on using the same
pattern everywhere.

However, we use our qsort() with user-defined comparison functions, and
we cannot make any guarantees about what they might do. So we must
ensure that our qsort() doesn't overflow, no matter what the comparison
function does.

Looking at our ST_SORT(), it seems safe to me.

--
Heikki Linnakangas
Neon (https://neon.tech)

#9Nathan Bossart
nathandbossart@gmail.com
In reply to: Heikki Linnakangas (#8)
2 attachment(s)
Re: glibc qsort() vulnerability

On Wed, Feb 07, 2024 at 08:46:56PM +0200, Heikki Linnakangas wrote:

Doesn't hurt to fix the comparison functions, and +1 on using the same
pattern everywhere.

I attached a new version of the patch with some small adjustments. I
haven't looked through all in-tree qsort() comparators to see if any others
need to be adjusted, but we should definitely do so as part of this thread.
Mats, are you able to do this?

However, we use our qsort() with user-defined comparison functions, and we
cannot make any guarantees about what they might do. So we must ensure that
our qsort() doesn't overflow, no matter what the comparison function does.

Looking at our ST_SORT(), it seems safe to me.

Cool.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

v2-0001-remove-direct-calls-to-pg_qsort.patchtext/x-diff; charset=us-asciiDownload
From f5850579e92f201218c3025327b91d820eabd18e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 7 Feb 2024 14:29:04 -0600
Subject: [PATCH v2 1/2] remove direct calls to pg_qsort()

---
 contrib/pg_prewarm/autoprewarm.c            |  4 ++--
 src/backend/access/brin/brin_minmax_multi.c |  2 +-
 src/backend/storage/buffer/bufmgr.c         |  4 ++--
 src/backend/utils/cache/syscache.c          | 10 +++++-----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 06ee21d496..248b9914a3 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -346,8 +346,8 @@ apw_load_buffers(void)
 	FreeFile(file);
 
 	/* Sort the blocks to be loaded. */
-	pg_qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
-			 apw_compare_blockinfo);
+	qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
+		  apw_compare_blockinfo);
 
 	/* Populate shared memory state. */
 	apw_state->block_info_handle = dsm_segment_handle(seg);
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 3ffaad3e42..2c29aa3d4e 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -1369,7 +1369,7 @@ build_distances(FmgrInfo *distanceFn, Oid colloid,
 	 * Sort the distances in descending order, so that the longest gaps are at
 	 * the front.
 	 */
-	pg_qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
+	qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
 
 	return distances;
 }
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index eb1ec3b86d..07575ef312 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3915,7 +3915,7 @@ DropRelationsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
 
 	/* sort the list of rlocators if necessary */
 	if (use_bsearch)
-		pg_qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
+		qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
@@ -4269,7 +4269,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
 
 	/* sort the list of SMgrRelations if necessary */
 	if (use_bsearch)
-		pg_qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
+		qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..662f930864 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -146,14 +146,14 @@ InitCatalogCache(void)
 	Assert(SysCacheSupportingRelOidSize <= lengthof(SysCacheSupportingRelOid));
 
 	/* Sort and de-dup OID arrays, so we can use binary search. */
-	pg_qsort(SysCacheRelationOid, SysCacheRelationOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheRelationOid, SysCacheRelationOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheRelationOidSize =
 		qunique(SysCacheRelationOid, SysCacheRelationOidSize, sizeof(Oid),
 				oid_compare);
 
-	pg_qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheSupportingRelOidSize =
 		qunique(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
 				sizeof(Oid), oid_compare);
@@ -668,7 +668,7 @@ RelationSupportsSysCache(Oid relid)
 
 
 /*
- * OID comparator for pg_qsort
+ * OID comparator for qsort
  */
 static int
 oid_compare(const void *a, const void *b)
-- 
2.25.1

v2-0002-Ensure-comparison-functions-are-transitive.patchtext/x-diff; charset=us-asciiDownload
From f1a046c3c67266187f5861b27c2fad1daa25101c Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: [PATCH v2 2/2] Ensure comparison functions are transitive

There are a few comparison functions to qsort() that are non-transitive
because they can cause an overflow. Fix these functions to instead use
normal comparisons and return -1, 0, or 1 explicitly.
---
 src/backend/access/spgist/spgtextproc.c |  6 +++++-
 src/backend/utils/cache/relcache.c      | 12 ++++++++----
 src/bin/pg_upgrade/function.c           | 20 ++++++++++++++++----
 src/bin/psql/crosstabview.c             |  9 ++++++++-
 4 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..d0a2b4e6e1 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -325,7 +325,11 @@ cmpNodePtr(const void *a, const void *b)
 	const spgNodePtr *aa = (const spgNodePtr *) a;
 	const spgNodePtr *bb = (const spgNodePtr *) b;
 
-	return aa->c - bb->c;
+	if (aa->c > bb->c)
+		return 1;
+	if (aa->c < bb->c)
+		return -1;
+	return 0;
 }
 
 Datum
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..dc5a100d82 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4517,10 +4517,14 @@ AttrDefaultFetch(Relation relation, int ndef)
 static int
 AttrDefaultCmp(const void *a, const void *b)
 {
-	const AttrDefault *ada = (const AttrDefault *) a;
-	const AttrDefault *adb = (const AttrDefault *) b;
-
-	return ada->adnum - adb->adnum;
+	AttrNumber	ana = ((const AttrDefault *) a)->adnum;
+	AttrNumber	anb = ((const AttrDefault *) b)->adnum;
+
+	if (ana > anb)
+		return 1;
+	if (ana < anb)
+		return -1;
+	return 0;
 }
 
 /*
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index af998f74d3..483ede2ce4 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -32,14 +32,26 @@ library_name_compare(const void *p1, const void *p2)
 	int			slen1 = strlen(str1);
 	int			slen2 = strlen(str2);
 	int			cmp = strcmp(str1, str2);
+	int			p1db = ((const LibraryInfo *) p1)->dbnum;
+	int			p2db = ((const LibraryInfo *) p2)->dbnum;
 
 	if (slen1 != slen2)
-		return slen1 - slen2;
+	{
+		if (slen1 > slen2)
+			return 1;
+		if (slen1 < slen2)
+			return -1;
+		return 0;
+	}
+
 	if (cmp != 0)
 		return cmp;
-	else
-		return ((const LibraryInfo *) p1)->dbnum -
-			((const LibraryInfo *) p2)->dbnum;
+
+	if (p1db > p2db)
+		return 1;
+	if (p1db < p2db)
+		return -1;
+	return 0;
 }
 
 
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index c6116b7238..de6168bd67 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -709,5 +709,12 @@ pivotFieldCompare(const void *a, const void *b)
 static int
 rankCompare(const void *a, const void *b)
 {
-	return *((const int *) a) - *((const int *) b);
+	const int	an = *(const int *) a;
+	const int	bn = *(const int *) b;
+
+	if (an > bn)
+		return 1;
+	if (an < bn)
+		return -1;
+	return 0;
 }
-- 
2.25.1

#10Andres Freund
andres@anarazel.de
In reply to: Heikki Linnakangas (#8)
Re: glibc qsort() vulnerability

Hi,

On 2024-02-07 20:46:56 +0200, Heikki Linnakangas wrote:

The routines modified do a subtraction of int:s and return that, which
can cause an overflow. This method is used for some int16 as well but
since standard conversions in C will perform the arithmetics in "int"
precision, this cannot overflow, so added a comment there. It might
still be a good idea to follow the same pattern for the int16 routines,
but since there is no bug there, I did not add them to the patch.

Doesn't hurt to fix the comparison functions, and +1 on using the same
pattern everywhere.

It actually can hurt - the generated code will often be slower.

E.g.
#include <stdint.h>

int cmp_sub(int16_t a, int16_t b) {
return (int32_t) a - (int32_t) b;
}

int cmp_if(int16_t a, int16_t b) {
if (a < b)
return -1;
if (a > b)
return 1;
return 0;
}

yields

cmp_sub:
movsx eax, di
movsx esi, si
sub eax, esi
ret
cmp_if:
xor eax, eax
cmp di, si
mov edx, -1
setg al
cmovl eax, edx
ret

with gcc -O3. With other compilers, e.g. msvc, the difference is considerably
bigger, due to msvc for some reason not using cmov.

See https://godbolt.org/z/34qerPaPE for a few more details.

Now, in most cases this won't matter, the sorting isn't performance
critical. But I don't think it's a good idea to standardize on a generally
slower pattern.

Not that that's a good test, but I did quickly benchmark [1]-- prep CREATE EXTENSION IF NOT EXISTS intarray; DROP TABLE IF EXISTS arrays_to_sort; CREATE TABLE arrays_to_sort AS SELECT array_shuffle(a) arr FROM (SELECT ARRAY(SELECT generate_series(1, 1000000)) a), generate_series(1, 10); -- bench SELECT (sort(arr))[1] FROM arrays_to_sort; this with
intarray. There's about a 10% difference in performance between using the
existing compASC() and one using
return (int64) *(const int32 *) a - (int64) *(const int32 *) b;

Perhaps we could have a central helper for this somewhere?

Greetings,

Andres Freund

[1]: -- prep CREATE EXTENSION IF NOT EXISTS intarray; DROP TABLE IF EXISTS arrays_to_sort; CREATE TABLE arrays_to_sort AS SELECT array_shuffle(a) arr FROM (SELECT ARRAY(SELECT generate_series(1, 1000000)) a), generate_series(1, 10); -- bench SELECT (sort(arr))[1] FROM arrays_to_sort;
-- prep
CREATE EXTENSION IF NOT EXISTS intarray;
DROP TABLE IF EXISTS arrays_to_sort;
CREATE TABLE arrays_to_sort AS SELECT array_shuffle(a) arr FROM (SELECT ARRAY(SELECT generate_series(1, 1000000)) a), generate_series(1, 10);
-- bench
SELECT (sort(arr))[1]-- prep CREATE EXTENSION IF NOT EXISTS intarray; DROP TABLE IF EXISTS arrays_to_sort; CREATE TABLE arrays_to_sort AS SELECT array_shuffle(a) arr FROM (SELECT ARRAY(SELECT generate_series(1, 1000000)) a), generate_series(1, 10); -- bench SELECT (sort(arr))[1] FROM arrays_to_sort; FROM arrays_to_sort;

#11Nathan Bossart
nathandbossart@gmail.com
In reply to: Andres Freund (#10)
Re: glibc qsort() vulnerability

On Wed, Feb 07, 2024 at 01:48:57PM -0800, Andres Freund wrote:

Now, in most cases this won't matter, the sorting isn't performance
critical. But I don't think it's a good idea to standardize on a generally
slower pattern.

Not that that's a good test, but I did quickly benchmark [1] this with
intarray. There's about a 10% difference in performance between using the
existing compASC() and one using
return (int64) *(const int32 *) a - (int64) *(const int32 *) b;

Perhaps we could have a central helper for this somewhere?

Maybe said helper could use __builtin_sub_overflow() and fall back to the
slow "if" version only if absolutely necessary. The assembly for that
looks encouraging, but I still need to actually test it...

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#12Andres Freund
andres@anarazel.de
In reply to: Nathan Bossart (#11)
Re: glibc qsort() vulnerability

Hi,

On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote:

On Wed, Feb 07, 2024 at 01:48:57PM -0800, Andres Freund wrote:

Now, in most cases this won't matter, the sorting isn't performance
critical. But I don't think it's a good idea to standardize on a generally
slower pattern.

Not that that's a good test, but I did quickly benchmark [1] this with
intarray. There's about a 10% difference in performance between using the
existing compASC() and one using
return (int64) *(const int32 *) a - (int64) *(const int32 *) b;

Perhaps we could have a central helper for this somewhere?

Maybe said helper could use __builtin_sub_overflow() and fall back to the
slow "if" version only if absolutely necessary.

I suspect that'll be worse code in the common case, given the cmov generated
by gcc & clang for the typical branch-y formulation. But it's worth testing.

The assembly for that looks encouraging, but I still need to actually test
it...

Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit
that doesn't work, given the 32bit return, so we need something more.

Greetings,

Andres Freund

#13Nathan Bossart
nathandbossart@gmail.com
In reply to: Andres Freund (#12)
Re: glibc qsort() vulnerability

On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote:

On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote:

The assembly for that looks encouraging, but I still need to actually test
it...

Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit
that doesn't work, given the 32bit return, so we need something more.

For the same compASC() test, I see an ~8.4% improvement with your int64
code and a ~3.4% improvement with this:

int
compASC(const void *a, const void *b)
{
int result;

if (unlikely(pg_sub_s32_overflow(*(const int32 *) a,
*(const int32 *) b,
&result)))
{
if (*(const int32 *) a > *(const int32 *) b)
return 1;
if (*(const int32 *) a < *(const int32 *) b)
return -1;
return 0;
}

return result;
}

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#14Andres Freund
andres@anarazel.de
In reply to: Nathan Bossart (#13)
Re: glibc qsort() vulnerability

Hi,

On 2024-02-07 19:52:11 -0600, Nathan Bossart wrote:

On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote:

On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote:

The assembly for that looks encouraging, but I still need to actually test
it...

Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit
that doesn't work, given the 32bit return, so we need something more.

For the same compASC() test, I see an ~8.4% improvement with your int64
code

Just to be clear, that code unfortuntely isn't correct, the return value is a
32 bit integer, so the 64bit difference doesn't help. In contrast to the 16bit
case.

and a ~3.4% improvement with this:

I guess that's still something.

Another branchless variant is (a > b) - (a < b). It seems to get a similar
improvement as the overflow-checking version.

Greetings,

Andres Freund

#15Thomas Munro
thomas.munro@gmail.com
In reply to: Andres Freund (#14)
Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 3:06 PM Andres Freund <andres@anarazel.de> wrote:

On 2024-02-07 19:52:11 -0600, Nathan Bossart wrote:

On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote:

On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote:

The assembly for that looks encouraging, but I still need to actually test
it...

Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit
that doesn't work, given the 32bit return, so we need something more.

For the same compASC() test, I see an ~8.4% improvement with your int64
code

Just to be clear, that code unfortuntely isn't correct, the return value is a
32 bit integer, so the 64bit difference doesn't help. In contrast to the 16bit
case.

Perhaps you could wrap it in a branch-free sign() function so you get
a narrow answer?

https://stackoverflow.com/questions/14579920/fast-sign-of-integer-in-c

#16Nathan Bossart
nathandbossart@gmail.com
In reply to: Andres Freund (#14)
2 attachment(s)
Re: glibc qsort() vulnerability

On Wed, Feb 07, 2024 at 06:06:37PM -0800, Andres Freund wrote:

Another branchless variant is (a > b) - (a < b). It seems to get a similar
improvement as the overflow-checking version.

Well, that's certainly more elegant. I updated the patch to that approach
for now.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

v3-0001-remove-direct-calls-to-pg_qsort.patchtext/x-diff; charset=us-asciiDownload
From 8668a0dd83ecbdd356c76416b8398138407ef829 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 7 Feb 2024 14:29:04 -0600
Subject: [PATCH v3 1/2] remove direct calls to pg_qsort()

---
 contrib/pg_prewarm/autoprewarm.c            |  4 ++--
 src/backend/access/brin/brin_minmax_multi.c |  2 +-
 src/backend/storage/buffer/bufmgr.c         |  4 ++--
 src/backend/utils/cache/syscache.c          | 10 +++++-----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 06ee21d496..248b9914a3 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -346,8 +346,8 @@ apw_load_buffers(void)
 	FreeFile(file);
 
 	/* Sort the blocks to be loaded. */
-	pg_qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
-			 apw_compare_blockinfo);
+	qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
+		  apw_compare_blockinfo);
 
 	/* Populate shared memory state. */
 	apw_state->block_info_handle = dsm_segment_handle(seg);
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 3ffaad3e42..2c29aa3d4e 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -1369,7 +1369,7 @@ build_distances(FmgrInfo *distanceFn, Oid colloid,
 	 * Sort the distances in descending order, so that the longest gaps are at
 	 * the front.
 	 */
-	pg_qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
+	qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
 
 	return distances;
 }
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index eb1ec3b86d..07575ef312 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3915,7 +3915,7 @@ DropRelationsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
 
 	/* sort the list of rlocators if necessary */
 	if (use_bsearch)
-		pg_qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
+		qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
@@ -4269,7 +4269,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
 
 	/* sort the list of SMgrRelations if necessary */
 	if (use_bsearch)
-		pg_qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
+		qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..662f930864 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -146,14 +146,14 @@ InitCatalogCache(void)
 	Assert(SysCacheSupportingRelOidSize <= lengthof(SysCacheSupportingRelOid));
 
 	/* Sort and de-dup OID arrays, so we can use binary search. */
-	pg_qsort(SysCacheRelationOid, SysCacheRelationOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheRelationOid, SysCacheRelationOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheRelationOidSize =
 		qunique(SysCacheRelationOid, SysCacheRelationOidSize, sizeof(Oid),
 				oid_compare);
 
-	pg_qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheSupportingRelOidSize =
 		qunique(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
 				sizeof(Oid), oid_compare);
@@ -668,7 +668,7 @@ RelationSupportsSysCache(Oid relid)
 
 
 /*
- * OID comparator for pg_qsort
+ * OID comparator for qsort
  */
 static int
 oid_compare(const void *a, const void *b)
-- 
2.25.1

v3-0002-Ensure-comparison-functions-are-transitive.patchtext/x-diff; charset=us-asciiDownload
From a74708f14d6e862b836b2d5106491a8db3114433 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: [PATCH v3 2/2] Ensure comparison functions are transitive

There are a few comparison functions to qsort() that are non-transitive
because they can cause an overflow. Fix these functions to instead use
normal comparisons and return -1, 0, or 1 explicitly.
---
 src/backend/access/spgist/spgtextproc.c |  2 +-
 src/backend/utils/cache/relcache.c      |  9 ++++++---
 src/bin/pg_upgrade/function.c           | 10 ++++++----
 src/bin/psql/crosstabview.c             |  5 ++++-
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..e561bde63e 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -325,7 +325,7 @@ cmpNodePtr(const void *a, const void *b)
 	const spgNodePtr *aa = (const spgNodePtr *) a;
 	const spgNodePtr *bb = (const spgNodePtr *) b;
 
-	return aa->c - bb->c;
+	return (aa->c > bb->c) - (aa->c < bb->c);
 }
 
 Datum
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..54682a0e60 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4517,10 +4517,13 @@ AttrDefaultFetch(Relation relation, int ndef)
 static int
 AttrDefaultCmp(const void *a, const void *b)
 {
-	const AttrDefault *ada = (const AttrDefault *) a;
-	const AttrDefault *adb = (const AttrDefault *) b;
+	AttrNumber	ana = ((const AttrDefault *) a)->adnum;
+	AttrNumber	anb = ((const AttrDefault *) b)->adnum;
 
-	return ada->adnum - adb->adnum;
+	/* ensure upcasting approach is transitive */
+	Assert(sizeof(AttrNumber) == 2);
+
+	return (int) ana - (int) anb;
 }
 
 /*
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index af998f74d3..b421500bbb 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -32,14 +32,16 @@ library_name_compare(const void *p1, const void *p2)
 	int			slen1 = strlen(str1);
 	int			slen2 = strlen(str2);
 	int			cmp = strcmp(str1, str2);
+	int			p1db = ((const LibraryInfo *) p1)->dbnum;
+	int			p2db = ((const LibraryInfo *) p2)->dbnum;
 
 	if (slen1 != slen2)
-		return slen1 - slen2;
+		return (slen1 > slen2) - (slen1 < slen2);
+
 	if (cmp != 0)
 		return cmp;
-	else
-		return ((const LibraryInfo *) p1)->dbnum -
-			((const LibraryInfo *) p2)->dbnum;
+
+	return (p1db > p2db) - (p1db < p2db);
 }
 
 
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index c6116b7238..7660f0ed4b 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -709,5 +709,8 @@ pivotFieldCompare(const void *a, const void *b)
 static int
 rankCompare(const void *a, const void *b)
 {
-	return *((const int *) a) - *((const int *) b);
+	const int	an = *(const int *) a;
+	const int	bn = *(const int *) b;
+
+	return (an > bn) - (an < bn);
 }
-- 
2.25.1

#17Thomas Munro
thomas.munro@gmail.com
In reply to: Thomas Munro (#15)
Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 3:38 PM Thomas Munro <thomas.munro@gmail.com> wrote:

Perhaps you could wrap it in a branch-free sign() function so you get
a narrow answer?

https://stackoverflow.com/questions/14579920/fast-sign-of-integer-in-c

Ah, strike that, it is much like the suggested (a > b) - (a < b) but
with extra steps...

#18Nathan Bossart
nathandbossart@gmail.com
In reply to: Thomas Munro (#17)
Re: glibc qsort() vulnerability

On Thu, Feb 08, 2024 at 03:49:03PM +1300, Thomas Munro wrote:

On Thu, Feb 8, 2024 at 3:38 PM Thomas Munro <thomas.munro@gmail.com> wrote:

Perhaps you could wrap it in a branch-free sign() function so you get
a narrow answer?

https://stackoverflow.com/questions/14579920/fast-sign-of-integer-in-c

Ah, strike that, it is much like the suggested (a > b) - (a < b) but
with extra steps...

Yeah, https://godbolt.org/ indicates that the sign approach compiles to

movsx rsi, esi
movsx rdi, edi
xor eax, eax
sub rdi, rsi
test rdi, rdi
setg al
shr rdi, 63
sub eax, edi
ret

while the approach Andres suggested compiles to

xor eax, eax
cmp edi, esi
setl dl
setg al
movzx edx, dl
sub eax, edx
ret

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#19Nathan Bossart
nathandbossart@gmail.com
In reply to: Nathan Bossart (#18)
Re: glibc qsort() vulnerability

Mats, I apologize for steamrolling a bit here. I'll take a step back into
a reviewer role.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#20Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#9)
Re: glibc qsort() vulnerability

On Wed, Feb 7, 2024 at 9:56 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Wed, Feb 07, 2024 at 08:46:56PM +0200, Heikki Linnakangas wrote:

Doesn't hurt to fix the comparison functions, and +1 on using the same
pattern everywhere.

I attached a new version of the patch with some small adjustments. I
haven't looked through all in-tree qsort() comparators to see if any others
need to be adjusted, but we should definitely do so as part of this thread.
Mats, are you able to do this?

Sure, I checked them and the only ones remaining are those using int16.
Shall I modify those as well?

Show quoted text

However, we use our qsort() with user-defined comparison functions, and

we

cannot make any guarantees about what they might do. So we must ensure

that

our qsort() doesn't overflow, no matter what the comparison function

does.

Looking at our ST_SORT(), it seems safe to me.

Cool.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#21Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#19)
Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 4:08 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:

Mats, I apologize for steamrolling a bit here. I'll take a step back into
a reviewer role.

No worries. :)

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#22Mats Kindahl
mats@timescale.com
In reply to: Mats Kindahl (#20)
Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 12:01 PM Mats Kindahl <mats@timescale.com> wrote:

On Wed, Feb 7, 2024 at 9:56 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Wed, Feb 07, 2024 at 08:46:56PM +0200, Heikki Linnakangas wrote:

Doesn't hurt to fix the comparison functions, and +1 on using the same
pattern everywhere.

I attached a new version of the patch with some small adjustments. I
haven't looked through all in-tree qsort() comparators to see if any
others
need to be adjusted, but we should definitely do so as part of this
thread.
Mats, are you able to do this?

Sure, I checked them and the only ones remaining are those using int16.
Shall I modify those as well?

Seems your additional patch dealt with at least one of the cases.

Show quoted text

However, we use our qsort() with user-defined comparison functions, and

we

cannot make any guarantees about what they might do. So we must ensure

that

our qsort() doesn't overflow, no matter what the comparison function

does.

Looking at our ST_SORT(), it seems safe to me.

Cool.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#23Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#18)
1 attachment(s)
Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 3:56 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Thu, Feb 08, 2024 at 03:49:03PM +1300, Thomas Munro wrote:

On Thu, Feb 8, 2024 at 3:38 PM Thomas Munro <thomas.munro@gmail.com>

wrote:

Perhaps you could wrap it in a branch-free sign() function so you get
a narrow answer?

https://stackoverflow.com/questions/14579920/fast-sign-of-integer-in-c

Ah, strike that, it is much like the suggested (a > b) - (a < b) but
with extra steps...

Yeah, https://godbolt.org/ indicates that the sign approach compiles to

movsx rsi, esi
movsx rdi, edi
xor eax, eax
sub rdi, rsi
test rdi, rdi
setg al
shr rdi, 63
sub eax, edi
ret

while the approach Andres suggested compiles to

xor eax, eax
cmp edi, esi
setl dl
setg al
movzx edx, dl
sub eax, edx
ret

Here is a patch that fixes existing cases and introduces a macro for this
comparison (it uses the (a > b) - (a < b) approach). Not sure where to
place the macro nor what a suitable name should be, so feel free to suggest
anything.

I also noted that some functions are duplicated and it might be an idea to
introduce a few standard functions like pg_qsort_strcmp for, e.g., integers
and other common types.

Also noted it is quite common to have this pattern in various places to do
lexicographic sort of multiple values and continue the comparison if they
are equal. Not sure if that is something we should look at.

Best wishes,
Mats Kindahl

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

0001-Standardize-integer-comparison-for-qsort.patchtext/x-patch; charset=US-ASCII; name=0001-Standardize-integer-comparison-for-qsort.patchDownload
From 8c183806a6b0f95ab53db4b029bc82823785c363 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: Standardize integer comparison for qsort()

Introduce a macro INT_CMP() that will standardize integer comparison
for implementing comparison function for qsort(). The macro will ensure
that the function returns -1, 0, or 1 without risking overflow as a
result of subtracting the two integers, which is otherwise a common
pattern for implementing this.
---
 contrib/hstore/hstore_gist.c                    |  2 +-
 contrib/intarray/_intbig_gist.c                 |  2 +-
 contrib/pg_stat_statements/pg_stat_statements.c |  7 +------
 contrib/pg_trgm/trgm_op.c                       |  7 +------
 contrib/seg/seg.c                               |  7 +------
 src/backend/access/nbtree/nbtinsert.c           |  7 +------
 src/backend/access/nbtree/nbtpage.c             |  9 ++-------
 src/backend/access/nbtree/nbtsplitloc.c         |  7 +------
 src/backend/access/spgist/spgdoinsert.c         |  4 +---
 src/backend/access/spgist/spgkdtreeproc.c       |  8 ++------
 src/backend/access/spgist/spgtextproc.c         |  2 +-
 src/backend/backup/basebackup_incremental.c     |  7 +------
 src/backend/backup/walsummary.c                 |  6 +-----
 src/backend/catalog/heap.c                      |  7 +------
 src/backend/nodes/list.c                        | 12 ++----------
 src/backend/nodes/tidbitmap.c                   |  6 +-----
 src/backend/optimizer/geqo/geqo_pool.c          |  7 +------
 src/backend/parser/parse_agg.c                  |  2 +-
 src/backend/postmaster/autovacuum.c             |  5 +----
 src/backend/replication/logical/reorderbuffer.c |  6 +-----
 src/backend/replication/syncrep.c               |  7 +------
 src/backend/utils/adt/oid.c                     |  6 +-----
 src/backend/utils/adt/tsgistidx.c               |  9 ++-------
 src/backend/utils/adt/tsquery_gist.c            |  5 +----
 src/backend/utils/adt/tsvector.c                |  4 +---
 src/backend/utils/adt/tsvector_op.c             |  4 +---
 src/backend/utils/adt/xid.c                     |  6 +-----
 src/backend/utils/cache/relcache.c              |  2 +-
 src/backend/utils/cache/syscache.c              |  4 +---
 src/backend/utils/cache/typcache.c              |  7 +------
 src/backend/utils/resowner/resowner.c           |  9 +--------
 src/bin/pg_dump/pg_dump_sort.c                  |  6 +-----
 src/bin/pg_upgrade/function.c                   |  8 ++++----
 src/bin/pg_walsummary/pg_walsummary.c           |  7 +------
 src/bin/psql/crosstabview.c                     |  2 +-
 src/include/access/gin_private.h                |  7 +------
 src/include/c.h                                 |  8 ++++++++
 37 files changed, 51 insertions(+), 170 deletions(-)

diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index fe343739eb..e125b76290 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -356,7 +356,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return INT_CMP(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
index 8c6c4b5d05..7f0deda34d 100644
--- a/contrib/intarray/_intbig_gist.c
+++ b/contrib/intarray/_intbig_gist.c
@@ -312,7 +312,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return INT_CMP(((const SPLITCOST *) a)->cost,  ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 8c6a3a2d08..f32e6868ed 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -3007,10 +3007,5 @@ comp_location(const void *a, const void *b)
 	int			l = ((const LocationLen *) a)->location;
 	int			r = ((const LocationLen *) b)->location;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return +1;
-	else
-		return 0;
+	return INT_CMP(l, r);
 }
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 49d4497b4f..f6020141a2 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -433,12 +433,7 @@ comp_ptrgm(const void *v1, const void *v2)
 	if (cmp != 0)
 		return cmp;
 
-	if (p1->index < p2->index)
-		return -1;
-	else if (p1->index == p2->index)
-		return 0;
-	else
-		return 1;
+	return INT_CMP(p1->index, p2->index);
 }
 
 /*
diff --git a/contrib/seg/seg.c b/contrib/seg/seg.c
index 7f9fc24eb4..ff2ffaf8c7 100644
--- a/contrib/seg/seg.c
+++ b/contrib/seg/seg.c
@@ -300,12 +300,7 @@ gseg_picksplit_item_cmp(const void *a, const void *b)
 	const gseg_picksplit_item *i1 = (const gseg_picksplit_item *) a;
 	const gseg_picksplit_item *i2 = (const gseg_picksplit_item *) b;
 
-	if (i1->center < i2->center)
-		return -1;
-	else if (i1->center == i2->center)
-		return 0;
-	else
-		return 1;
+	return INT_CMP(i1->center, i2->center);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 709edd1c17..1aeaf19efd 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -3013,10 +3013,5 @@ _bt_blk_cmp(const void *arg1, const void *arg2)
 	BlockNumber b1 = *((BlockNumber *) arg1);
 	BlockNumber b2 = *((BlockNumber *) arg2);
 
-	if (b1 < b2)
-		return -1;
-	else if (b1 > b2)
-		return 1;
-
-	return 0;
+	return INT_CMP(b1, b2);
 }
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 567bade9f4..bb31cda542 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -1466,14 +1466,9 @@ _bt_delitems_cmp(const void *a, const void *b)
 	TM_IndexDelete *indexdelete1 = (TM_IndexDelete *) a;
 	TM_IndexDelete *indexdelete2 = (TM_IndexDelete *) b;
 
-	if (indexdelete1->id > indexdelete2->id)
-		return 1;
-	if (indexdelete1->id < indexdelete2->id)
-		return -1;
+	Assert(INT_CMP(indexdelete1->id, indexdelete2->id) != 0);
 
-	Assert(false);
-
-	return 0;
+	return INT_CMP(indexdelete1->id, indexdelete2->id);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtsplitloc.c b/src/backend/access/nbtree/nbtsplitloc.c
index d0b1d82578..f33f1f9d51 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -596,12 +596,7 @@ _bt_splitcmp(const void *arg1, const void *arg2)
 	SplitPoint *split1 = (SplitPoint *) arg1;
 	SplitPoint *split2 = (SplitPoint *) arg2;
 
-	if (split1->curdelta > split2->curdelta)
-		return 1;
-	if (split1->curdelta < split2->curdelta)
-		return -1;
-
-	return 0;
+	return INT_CMP(split1->curdelta,  split2->curdelta);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index bb063c858d..995392d480 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -110,9 +110,7 @@ addNode(SpGistState *state, SpGistInnerTuple tuple, Datum label, int offset)
 static int
 cmpOffsetNumbers(const void *a, const void *b)
 {
-	if (*(const OffsetNumber *) a == *(const OffsetNumber *) b)
-		return 0;
-	return (*(const OffsetNumber *) a > *(const OffsetNumber *) b) ? 1 : -1;
+	return INT_CMP(*(const OffsetNumber *) a, *(const OffsetNumber *) b);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgkdtreeproc.c b/src/backend/access/spgist/spgkdtreeproc.c
index 900fe0d2af..a1a2598a3f 100644
--- a/src/backend/access/spgist/spgkdtreeproc.c
+++ b/src/backend/access/spgist/spgkdtreeproc.c
@@ -87,9 +87,7 @@ x_cmp(const void *a, const void *b)
 	SortedPoint *pa = (SortedPoint *) a;
 	SortedPoint *pb = (SortedPoint *) b;
 
-	if (pa->p->x == pb->p->x)
-		return 0;
-	return (pa->p->x > pb->p->x) ? 1 : -1;
+	return INT_CMP(pa->p->x, pb->p->x);
 }
 
 static int
@@ -98,9 +96,7 @@ y_cmp(const void *a, const void *b)
 	SortedPoint *pa = (SortedPoint *) a;
 	SortedPoint *pb = (SortedPoint *) b;
 
-	if (pa->p->y == pb->p->y)
-		return 0;
-	return (pa->p->y > pb->p->y) ? 1 : -1;
+	return INT_CMP(pa->p->y, pb->p->y);
 }
 
 
diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..4855f1f1de 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -325,7 +325,7 @@ cmpNodePtr(const void *a, const void *b)
 	const spgNodePtr *aa = (const spgNodePtr *) a;
 	const spgNodePtr *bb = (const spgNodePtr *) b;
 
-	return aa->c - bb->c;
+	return INT_CMP(aa->c, bb->c);
 }
 
 Datum
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 0504c465db..c45c015f3c 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -994,10 +994,5 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return INT_CMP(aa,bb);
 }
diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 867870aaad..41e34fcb8b 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -355,9 +355,5 @@ ListComparatorForWalSummaryFiles(const ListCell *a, const ListCell *b)
 	WalSummaryFile *ws1 = lfirst(a);
 	WalSummaryFile *ws2 = lfirst(b);
 
-	if (ws1->start_lsn < ws2->start_lsn)
-		return -1;
-	if (ws1->start_lsn > ws2->start_lsn)
-		return 1;
-	return 0;
+	return INT_CMP(ws1->start_lsn, ws2->start_lsn);
 }
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c73f7bcd01..b26d2bf04a 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2758,12 +2758,7 @@ list_cookedconstr_attnum_cmp(const ListCell *p1, const ListCell *p2)
 {
 	AttrNumber	v1 = ((CookedConstraint *) lfirst(p1))->attnum;
 	AttrNumber	v2 = ((CookedConstraint *) lfirst(p2))->attnum;
-
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return INT_CMP(v1, v2);
 }
 
 /*
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 957186bef5..769c97ee83 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -1692,11 +1692,7 @@ list_int_cmp(const ListCell *p1, const ListCell *p2)
 	int			v1 = lfirst_int(p1);
 	int			v2 = lfirst_int(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return INT_CMP(v1, v2);
 }
 
 /*
@@ -1708,9 +1704,5 @@ list_oid_cmp(const ListCell *p1, const ListCell *p2)
 	Oid			v1 = lfirst_oid(p1);
 	Oid			v2 = lfirst_oid(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return INT_CMP(v1, v2);
 }
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index 0f4850065f..944f261e38 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -1425,11 +1425,7 @@ tbm_comparator(const void *left, const void *right)
 	BlockNumber l = (*((PagetableEntry *const *) left))->blockno;
 	BlockNumber r = (*((PagetableEntry *const *) right))->blockno;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return 1;
-	return 0;
+	return INT_CMP(l,r);
 }
 
 /*
diff --git a/src/backend/optimizer/geqo/geqo_pool.c b/src/backend/optimizer/geqo/geqo_pool.c
index 0ec97d5a3f..165ff7a380 100644
--- a/src/backend/optimizer/geqo/geqo_pool.c
+++ b/src/backend/optimizer/geqo/geqo_pool.c
@@ -147,12 +147,7 @@ compare(const void *arg1, const void *arg2)
 	const Chromosome *chromo1 = (const Chromosome *) arg1;
 	const Chromosome *chromo2 = (const Chromosome *) arg2;
 
-	if (chromo1->worth == chromo2->worth)
-		return 0;
-	else if (chromo1->worth > chromo2->worth)
-		return 1;
-	else
-		return -1;
+	return INT_CMP(chromo1->worth, chromo2->worth);
 }
 
 /* alloc_chromo
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 7b211a7743..af3c4da463 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -1760,7 +1760,7 @@ cmp_list_len_asc(const ListCell *a, const ListCell *b)
 	int			la = list_length((const List *) lfirst(a));
 	int			lb = list_length((const List *) lfirst(b));
 
-	return (la > lb) ? 1 : (la == lb) ? 0 : -1;
+	return INT_CMP(la, lb);
 }
 
 /* list_sort comparator to sort sub-lists by length and contents */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2c3099f76f..82a6f4a2f0 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1119,10 +1119,7 @@ rebuild_database_list(Oid newdb)
 static int
 db_comparator(const void *a, const void *b)
 {
-	if (((const avl_dbase *) a)->adl_score == ((const avl_dbase *) b)->adl_score)
-		return 0;
-	else
-		return (((const avl_dbase *) a)->adl_score < ((const avl_dbase *) b)->adl_score) ? 1 : -1;
+	return INT_CMP(((const avl_dbase *) a)->adl_score, ((const avl_dbase *) b)->adl_score);
 }
 
 /*
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index bbf0966182..5d61a2227e 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -5119,11 +5119,7 @@ file_sort_by_lsn(const ListCell *a_p, const ListCell *b_p)
 	RewriteMappingFile *a = (RewriteMappingFile *) lfirst(a_p);
 	RewriteMappingFile *b = (RewriteMappingFile *) lfirst(b_p);
 
-	if (a->lsn < b->lsn)
-		return -1;
-	else if (a->lsn > b->lsn)
-		return 1;
-	return 0;
+	return INT_CMP(a->lsn, b->lsn);
 }
 
 /*
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 2e6493aaaa..6aecce074d 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -698,12 +698,7 @@ cmp_lsn(const void *a, const void *b)
 	XLogRecPtr	lsn1 = *((const XLogRecPtr *) a);
 	XLogRecPtr	lsn2 = *((const XLogRecPtr *) b);
 
-	if (lsn1 > lsn2)
-		return -1;
-	else if (lsn1 == lsn2)
-		return 0;
-	else
-		return 1;
+	return INT_CMP(lsn2, lsn1);
 }
 
 /*
diff --git a/src/backend/utils/adt/oid.c b/src/backend/utils/adt/oid.c
index 62bcfc5b56..a46adfe764 100644
--- a/src/backend/utils/adt/oid.c
+++ b/src/backend/utils/adt/oid.c
@@ -259,11 +259,7 @@ oid_cmp(const void *p1, const void *p2)
 	Oid			v1 = *((const Oid *) p1);
 	Oid			v2 = *((const Oid *) p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return INT_CMP(v1, v2);
 }
 
 
diff --git a/src/backend/utils/adt/tsgistidx.c b/src/backend/utils/adt/tsgistidx.c
index a62b285365..bd6eac3ea4 100644
--- a/src/backend/utils/adt/tsgistidx.c
+++ b/src/backend/utils/adt/tsgistidx.c
@@ -136,9 +136,7 @@ compareint(const void *va, const void *vb)
 	int32		a = *((const int32 *) va);
 	int32		b = *((const int32 *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return INT_CMP(a,b);
 }
 
 static void
@@ -598,10 +596,7 @@ comparecost(const void *va, const void *vb)
 	const SPLITCOST *a = (const SPLITCOST *) va;
 	const SPLITCOST *b = (const SPLITCOST *) vb;
 
-	if (a->cost == b->cost)
-		return 0;
-	else
-		return (a->cost > b->cost) ? 1 : -1;
+	return INT_CMP(a->cost, b->cost);
 }
 
 
diff --git a/src/backend/utils/adt/tsquery_gist.c b/src/backend/utils/adt/tsquery_gist.c
index f0b1c81c81..29d49f45dc 100644
--- a/src/backend/utils/adt/tsquery_gist.c
+++ b/src/backend/utils/adt/tsquery_gist.c
@@ -156,10 +156,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	if (((const SPLITCOST *) a)->cost == ((const SPLITCOST *) b)->cost)
-		return 0;
-	else
-		return (((const SPLITCOST *) a)->cost > ((const SPLITCOST *) b)->cost) ? 1 : -1;
+	return INT_CMP(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 #define WISH_F(a,b,c) (double)( -(double)(((a)-(b))*((a)-(b))*((a)-(b)))*(c) )
diff --git a/src/backend/utils/adt/tsvector.c b/src/backend/utils/adt/tsvector.c
index fb7b7c712a..1875612019 100644
--- a/src/backend/utils/adt/tsvector.c
+++ b/src/backend/utils/adt/tsvector.c
@@ -37,9 +37,7 @@ compareWordEntryPos(const void *a, const void *b)
 	int			apos = WEP_GETPOS(*(const WordEntryPos *) a);
 	int			bpos = WEP_GETPOS(*(const WordEntryPos *) b);
 
-	if (apos == bpos)
-		return 0;
-	return (apos > bpos) ? 1 : -1;
+	return INT_CMP(apos, bpos);
 }
 
 /*
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index 71386d0a1f..c17d0599e3 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -435,9 +435,7 @@ compare_int(const void *va, const void *vb)
 	int			a = *((const int *) va);
 	int			b = *((const int *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return INT_CMP(a,b);
 }
 
 static int
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 63adf5668b..79b35cbd7b 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -140,11 +140,7 @@ xidComparator(const void *arg1, const void *arg2)
 	TransactionId xid1 = *(const TransactionId *) arg1;
 	TransactionId xid2 = *(const TransactionId *) arg2;
 
-	if (xid1 > xid2)
-		return 1;
-	if (xid1 < xid2)
-		return -1;
-	return 0;
+	return INT_CMP(xid1, xid2);
 }
 
 /*
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..fd3a56349e 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4520,7 +4520,7 @@ AttrDefaultCmp(const void *a, const void *b)
 	const AttrDefault *ada = (const AttrDefault *) a;
 	const AttrDefault *adb = (const AttrDefault *) b;
 
-	return ada->adnum - adb->adnum;
+	return INT_CMP(ada->adnum, adb->adnum);
 }
 
 /*
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..2bfc7e7fe0 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -676,7 +676,5 @@ oid_compare(const void *a, const void *b)
 	Oid			oa = *((const Oid *) a);
 	Oid			ob = *((const Oid *) b);
 
-	if (oa == ob)
-		return 0;
-	return (oa > ob) ? 1 : -1;
+	return INT_CMP(oa, ob);
 }
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 84fc83cb11..b689471e53 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -2722,12 +2722,7 @@ enum_oid_cmp(const void *left, const void *right)
 	const EnumItem *l = (const EnumItem *) left;
 	const EnumItem *r = (const EnumItem *) right;
 
-	if (l->enum_oid < r->enum_oid)
-		return -1;
-	else if (l->enum_oid > r->enum_oid)
-		return 1;
-	else
-		return 0;
+	return INT_CMP(l->enum_oid, r->enum_oid);
 }
 
 /*
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index aa199b23ff..4cda8dde7f 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -264,14 +264,7 @@ resource_priority_cmp(const void *a, const void *b)
 
 	/* Note: reverse order */
 	if (ra->kind->release_phase == rb->kind->release_phase)
-	{
-		if (ra->kind->release_priority == rb->kind->release_priority)
-			return 0;
-		else if (ra->kind->release_priority > rb->kind->release_priority)
-			return -1;
-		else
-			return 1;
-	}
+		return INT_CMP(ra->kind->release_priority, rb->kind->release_priority);
 	else if (ra->kind->release_phase > rb->kind->release_phase)
 		return -1;
 	else
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index f358dd22b9..72937622ac 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -1504,9 +1504,5 @@ int_cmp(void *a, void *b, void *arg)
 	int			ai = (int) (intptr_t) a;
 	int			bi = (int) (intptr_t) b;
 
-	if (ai < bi)
-		return -1;
-	if (ai > bi)
-		return 1;
-	return 0;
+	return INT_CMP(ai, bi);
 }
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index af998f74d3..d21c534d7c 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -32,14 +32,14 @@ library_name_compare(const void *p1, const void *p2)
 	int			slen1 = strlen(str1);
 	int			slen2 = strlen(str2);
 	int			cmp = strcmp(str1, str2);
+	int lhsdb = ((const LibraryInfo *) p1)->dbnum;
+	int rhsdb = ((const LibraryInfo *) p2)->dbnum;
 
 	if (slen1 != slen2)
-		return slen1 - slen2;
+		return INT_CMP(slen1, slen2);
 	if (cmp != 0)
 		return cmp;
-	else
-		return ((const LibraryInfo *) p1)->dbnum -
-			((const LibraryInfo *) p2)->dbnum;
+	return INT_CMP(lhsdb, rhsdb);
 }
 
 
diff --git a/src/bin/pg_walsummary/pg_walsummary.c b/src/bin/pg_walsummary/pg_walsummary.c
index 1341c83c69..2de4547279 100644
--- a/src/bin/pg_walsummary/pg_walsummary.c
+++ b/src/bin/pg_walsummary/pg_walsummary.c
@@ -219,12 +219,7 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return INT_CMP(aa, bb);
 }
 
 /*
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index c6116b7238..50f24055b6 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -709,5 +709,5 @@ pivotFieldCompare(const void *a, const void *b)
 static int
 rankCompare(const void *a, const void *b)
 {
-	return *((const int *) a) - *((const int *) b);
+	return INT_CMP(*(const int *) a, *(const int *) b);
 }
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 51d0c74a6b..da5646ab41 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -489,12 +489,7 @@ ginCompareItemPointers(ItemPointer a, ItemPointer b)
 	uint64		ia = (uint64) GinItemPointerGetBlockNumber(a) << 32 | GinItemPointerGetOffsetNumber(a);
 	uint64		ib = (uint64) GinItemPointerGetBlockNumber(b) << 32 | GinItemPointerGetOffsetNumber(b);
 
-	if (ia == ib)
-		return 0;
-	else if (ia > ib)
-		return 1;
-	else
-		return -1;
+	return INT_CMP(ia, ib);
 }
 
 extern int	ginTraverseLock(Buffer buffer, bool searchMode);
diff --git a/src/include/c.h b/src/include/c.h
index 2e3ea206e1..114c3373ca 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1090,6 +1090,14 @@ extern void ExceptionalCondition(const char *conditionName,
  * ----------------------------------------------------------------
  */
 
+/*
+ * Compare two integers and return -1, 0, or 1 without risking overflow.
+ *
+ * This macro is used to avoid running into overflow issues because a simple
+ * subtraction of the two values when implementing a cmp function for qsort().
+*/
+#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))
+
 /*
  * Invert the sign of a qsort-style comparison result, ie, exchange negative
  * and positive integer values, being careful not to get the wrong answer
-- 
2.34.1

#24Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#23)
Re: glibc qsort() vulnerability

On Thu, Feb 08, 2024 at 02:16:11PM +0100, Mats Kindahl wrote:

+/*
+ * Compare two integers and return -1, 0, or 1 without risking overflow.
+ *
+ * This macro is used to avoid running into overflow issues because a simple
+ * subtraction of the two values when implementing a cmp function for qsort().
+*/
+#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))

I think we should offer a few different macros, i.e., separate macros for
int8, uint8, int16, uint16, int32, etc. For int16, we can do something
faster like

(int32) (lhs) - (int32) (rhs)

but for int32, we need to do someting more like what's in the patch.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#25Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nathan Bossart (#24)
Re: glibc qsort() vulnerability

Nathan Bossart <nathandbossart@gmail.com> writes:

On Thu, Feb 08, 2024 at 02:16:11PM +0100, Mats Kindahl wrote:

+/*
+ * Compare two integers and return -1, 0, or 1 without risking overflow.
+ *
+ * This macro is used to avoid running into overflow issues because a simple
+ * subtraction of the two values when implementing a cmp function for qsort().
+*/
+#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))

I think we should offer a few different macros, i.e., separate macros for
int8, uint8, int16, uint16, int32, etc. For int16, we can do something
faster like

(int32) (lhs) - (int32) (rhs)

but for int32, we need to do someting more like what's in the patch.

Are we okay with using macros that (a) have double evaluation hazards
and (b) don't enforce the data types being compared are the same?
I think static inlines might be a safer technology. Perhaps it's
okay given the only expected use is in qsort comparators, but ...

regards, tom lane

#26Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#25)
Re: glibc qsort() vulnerability

Hi,

On 2024-02-08 13:44:02 -0500, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

On Thu, Feb 08, 2024 at 02:16:11PM +0100, Mats Kindahl wrote:

+/*
+ * Compare two integers and return -1, 0, or 1 without risking overflow.
+ *
+ * This macro is used to avoid running into overflow issues because a simple
+ * subtraction of the two values when implementing a cmp function for qsort().
+*/
+#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))

I think we should offer a few different macros, i.e., separate macros for
int8, uint8, int16, uint16, int32, etc. For int16, we can do something
faster like

+1

(int32) (lhs) - (int32) (rhs)

but for int32, we need to do someting more like what's in the patch.

Are we okay with using macros that (a) have double evaluation hazards
and (b) don't enforce the data types being compared are the same?
I think static inlines might be a safer technology.

+1

I'd put these static inlines into common/int.h. I don't think this is common
enough to warrant being in c.h. Probably also doesn't hurt to have a not quite
as generic name as INT_CMP, I'd not be too surprised if that's defined in some
library.

I think it's worth following int.h's pattern of including [s]igned/[u]nsigned
in the name, an efficient implementation for signed might not be the same as
for unsigned. And if we use static inlines, we need to do so for correct
semantics anyway.

Greetings,

Andres

#27Nathan Bossart
nathandbossart@gmail.com
In reply to: Andres Freund (#26)
Re: glibc qsort() vulnerability

On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:

On 2024-02-08 13:44:02 -0500, Tom Lane wrote:

Are we okay with using macros that (a) have double evaluation hazards
and (b) don't enforce the data types being compared are the same?
I think static inlines might be a safer technology.

+1

Agreed on static inlines.

I'd put these static inlines into common/int.h. I don't think this is common
enough to warrant being in c.h. Probably also doesn't hurt to have a not quite
as generic name as INT_CMP, I'd not be too surprised if that's defined in some
library.

I think it's worth following int.h's pattern of including [s]igned/[u]nsigned
in the name, an efficient implementation for signed might not be the same as
for unsigned. And if we use static inlines, we need to do so for correct
semantics anyway.

Seems reasonable to me.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#28Mats Kindahl
mats@timescale.com
In reply to: Tom Lane (#25)
Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 7:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

On Thu, Feb 08, 2024 at 02:16:11PM +0100, Mats Kindahl wrote:

+/*
+ * Compare two integers and return -1, 0, or 1 without risking

overflow.

+ *
+ * This macro is used to avoid running into overflow issues because a

simple

+ * subtraction of the two values when implementing a cmp function for

qsort().

+*/
+#define INT_CMP(lhs,rhs) (((lhs) > (rhs)) - ((lhs) < (rhs)))

I think we should offer a few different macros, i.e., separate macros for
int8, uint8, int16, uint16, int32, etc. For int16, we can do something
faster like

(int32) (lhs) - (int32) (rhs)

but for int32, we need to do someting more like what's in the patch.

Are we okay with using macros that (a) have double evaluation hazards
and (b) don't enforce the data types being compared are the same?
I think static inlines might be a safer technology. Perhaps it's
okay given the only expected use is in qsort comparators, but ...

I picked a macro simply because it can deal with all kinds of integers, but
if we want to have separate implementations for each then inline functions
work just as well and will be safer.

Best wishes,
Mats Kindahl

Show quoted text

regards, tom lane

#29Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#27)
Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 9:07 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:

On 2024-02-08 13:44:02 -0500, Tom Lane wrote:

Are we okay with using macros that (a) have double evaluation hazards
and (b) don't enforce the data types being compared are the same?
I think static inlines might be a safer technology.

+1

Agreed on static inlines.

Seems to be a general consensus on static inlines. I'll get a new patch.

I'd put these static inlines into common/int.h. I don't think this is

common

enough to warrant being in c.h. Probably also doesn't hurt to have a not

quite

as generic name as INT_CMP, I'd not be too surprised if that's defined

in some

library.

I think it's worth following int.h's pattern of including

[s]igned/[u]nsigned

in the name, an efficient implementation for signed might not be the

same as

for unsigned. And if we use static inlines, we need to do so for correct
semantics anyway.

Seems reasonable to me.

Agree.

Best wishes,
Mats Kindahl

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#30Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nathan Bossart (#27)
Re: glibc qsort() vulnerability

Nathan Bossart <nathandbossart@gmail.com> writes:

On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:

I'd put these static inlines into common/int.h. I don't think this is common
enough to warrant being in c.h. Probably also doesn't hurt to have a not quite
as generic name as INT_CMP, I'd not be too surprised if that's defined in some
library.

I think it's worth following int.h's pattern of including [s]igned/[u]nsigned
in the name, an efficient implementation for signed might not be the same as
for unsigned. And if we use static inlines, we need to do so for correct
semantics anyway.

Seems reasonable to me.

+1 here also.

regards, tom lane

#31Mats Kindahl
mats@timescale.com
In reply to: Tom Lane (#30)
1 attachment(s)
Re: glibc qsort() vulnerability

On Thu, Feb 8, 2024 at 9:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

On Thu, Feb 08, 2024 at 11:59:54AM -0800, Andres Freund wrote:

I'd put these static inlines into common/int.h. I don't think this is

common

enough to warrant being in c.h. Probably also doesn't hurt to have a

not quite

as generic name as INT_CMP, I'd not be too surprised if that's defined

in some

library.

I think it's worth following int.h's pattern of including

[s]igned/[u]nsigned

in the name, an efficient implementation for signed might not be the

same as

for unsigned. And if we use static inlines, we need to do so for correct
semantics anyway.

Seems reasonable to me.

+1 here also.

Here is a new version introducing pg_cmp_s32 and friends and use them
instead of the INT_CMP macro introduced before. It also moves the
definitions to common/int.h and adds that as an include to all locations
using these functions.

Note that for integers with sizes less than sizeof(int), C standard
conversions will convert the values to "int" before doing the arithmetic,
so no casting is *necessary*. I did not force the 16-bit functions to
return -1 or 1 and have updated the comment accordingly.

The types "int" and "size_t" are treated as s32 and u32 respectively since
that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

I also noted that in many situations size_t values are treated as "int" so
there is an overflow risk here, even if small. For example, the result of
"list_length" is assigned to an integer. I do not think this is an
immediate concern, but just wanted to mention it.

Best wishes,
Mats Kindahl

Show quoted text

regards, tom lane

Attachments:

0001-Add-integer-comparison-functions-for-qsort.patchtext/x-patch; charset=US-ASCII; name=0001-Add-integer-comparison-functions-for-qsort.patchDownload
From 6980199cefde28b4ccf5f0fc3d89e001e8b0c4ef Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Tue, 6 Feb 2024 14:53:53 +0100
Subject: Add integer comparison functions for qsort()

Introduce functions pg_cmp_xyz() that will standardize integer comparison
for implementing comparison function for qsort(). The functions will
returns negative, 0, or positive integer without risking overflow as a
result of subtracting the two integers, which is otherwise a common
pattern for implementing this.

For integer sizes less than sizeof(int) we can use normal subtraction,
which is faster, but for integers with size greater than or equal to
sizeof(int) we need to handle this differently.
---
 contrib/hstore/hstore_gist.c                  |  2 +-
 contrib/intarray/_intbig_gist.c               |  2 +-
 .../pg_stat_statements/pg_stat_statements.c   |  7 +---
 contrib/pg_trgm/trgm_op.c                     |  7 +---
 contrib/seg/seg.c                             |  7 +---
 src/backend/access/nbtree/nbtinsert.c         |  8 +---
 src/backend/access/nbtree/nbtpage.c           | 10 ++---
 src/backend/access/nbtree/nbtsplitloc.c       |  8 +---
 src/backend/access/spgist/spgdoinsert.c       |  5 +--
 src/backend/access/spgist/spgtextproc.c       |  3 +-
 src/backend/backup/basebackup_incremental.c   |  8 +---
 src/backend/backup/walsummary.c               |  7 +---
 src/backend/catalog/heap.c                    |  8 +---
 src/backend/nodes/list.c                      | 13 ++----
 src/backend/nodes/tidbitmap.c                 |  7 +---
 src/backend/parser/parse_agg.c                |  3 +-
 src/backend/postmaster/autovacuum.c           |  6 +--
 .../replication/logical/reorderbuffer.c       |  7 +---
 src/backend/replication/syncrep.c             |  8 +---
 src/backend/utils/adt/oid.c                   |  7 +---
 src/backend/utils/adt/tsgistidx.c             | 10 ++---
 src/backend/utils/adt/tsquery_gist.c          |  6 +--
 src/backend/utils/adt/tsvector.c              |  5 +--
 src/backend/utils/adt/tsvector_op.c           |  5 +--
 src/backend/utils/adt/xid.c                   |  7 +---
 src/backend/utils/cache/relcache.c            |  3 +-
 src/backend/utils/cache/syscache.c            |  5 +--
 src/backend/utils/cache/typcache.c            |  8 +---
 src/backend/utils/resowner/resowner.c         | 10 +----
 src/bin/pg_dump/pg_dump_sort.c                |  7 +---
 src/bin/pg_upgrade/function.c                 | 13 +++---
 src/bin/pg_walsummary/pg_walsummary.c         |  8 +---
 src/bin/psql/crosstabview.c                   |  3 +-
 src/include/access/gin_private.h              |  8 +---
 src/include/common/int.h                      | 41 +++++++++++++++++++
 35 files changed, 112 insertions(+), 160 deletions(-)

diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index fe343739eb..e125b76290 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -356,7 +356,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return INT_CMP(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
index 8c6c4b5d05..7f0deda34d 100644
--- a/contrib/intarray/_intbig_gist.c
+++ b/contrib/intarray/_intbig_gist.c
@@ -312,7 +312,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return INT_CMP(((const SPLITCOST *) a)->cost,  ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 8c6a3a2d08..f32e6868ed 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -3007,10 +3007,5 @@ comp_location(const void *a, const void *b)
 	int			l = ((const LocationLen *) a)->location;
 	int			r = ((const LocationLen *) b)->location;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return +1;
-	else
-		return 0;
+	return INT_CMP(l, r);
 }
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 49d4497b4f..f6020141a2 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -433,12 +433,7 @@ comp_ptrgm(const void *v1, const void *v2)
 	if (cmp != 0)
 		return cmp;
 
-	if (p1->index < p2->index)
-		return -1;
-	else if (p1->index == p2->index)
-		return 0;
-	else
-		return 1;
+	return INT_CMP(p1->index, p2->index);
 }
 
 /*
diff --git a/contrib/seg/seg.c b/contrib/seg/seg.c
index 7f9fc24eb4..ff2ffaf8c7 100644
--- a/contrib/seg/seg.c
+++ b/contrib/seg/seg.c
@@ -300,12 +300,7 @@ gseg_picksplit_item_cmp(const void *a, const void *b)
 	const gseg_picksplit_item *i1 = (const gseg_picksplit_item *) a;
 	const gseg_picksplit_item *i2 = (const gseg_picksplit_item *) b;
 
-	if (i1->center < i2->center)
-		return -1;
-	else if (i1->center == i2->center)
-		return 0;
-	else
-		return 1;
+	return INT_CMP(i1->center, i2->center);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 709edd1c17..9cfe4cc094 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -20,6 +20,7 @@
 #include "access/transam.h"
 #include "access/xloginsert.h"
 #include "common/pg_prng.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
@@ -3013,10 +3014,5 @@ _bt_blk_cmp(const void *arg1, const void *arg2)
 	BlockNumber b1 = *((BlockNumber *) arg1);
 	BlockNumber b2 = *((BlockNumber *) arg2);
 
-	if (b1 < b2)
-		return -1;
-	else if (b1 > b2)
-		return 1;
-
-	return 0;
+	return pg_cmp_u32(b1, b2);
 }
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 567bade9f4..b8dd9fd12b 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -28,6 +28,7 @@
 #include "access/transam.h"
 #include "access/xlog.h"
 #include "access/xloginsert.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "storage/indexfsm.h"
 #include "storage/lmgr.h"
@@ -1466,14 +1467,9 @@ _bt_delitems_cmp(const void *a, const void *b)
 	TM_IndexDelete *indexdelete1 = (TM_IndexDelete *) a;
 	TM_IndexDelete *indexdelete2 = (TM_IndexDelete *) b;
 
-	if (indexdelete1->id > indexdelete2->id)
-		return 1;
-	if (indexdelete1->id < indexdelete2->id)
-		return -1;
+	Assert(INT_CMP(indexdelete1->id, indexdelete2->id) != 0);
 
-	Assert(false);
-
-	return 0;
+	return pg_cmp_s16(indexdelete1->id, indexdelete2->id);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtsplitloc.c b/src/backend/access/nbtree/nbtsplitloc.c
index d0b1d82578..0bcc8e90ae 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -15,6 +15,7 @@
 #include "postgres.h"
 
 #include "access/nbtree.h"
+#include "common/int.h"
 #include "storage/lmgr.h"
 
 typedef enum
@@ -596,12 +597,7 @@ _bt_splitcmp(const void *arg1, const void *arg2)
 	SplitPoint *split1 = (SplitPoint *) arg1;
 	SplitPoint *split2 = (SplitPoint *) arg2;
 
-	if (split1->curdelta > split2->curdelta)
-		return 1;
-	if (split1->curdelta < split2->curdelta)
-		return -1;
-
-	return 0;
+	return pg_cmp_s16(split1->curdelta,  split2->curdelta);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index bb063c858d..8695370139 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -20,6 +20,7 @@
 #include "access/spgxlog.h"
 #include "access/xloginsert.h"
 #include "common/pg_prng.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
 #include "utils/rel.h"
@@ -110,9 +111,7 @@ addNode(SpGistState *state, SpGistInnerTuple tuple, Datum label, int offset)
 static int
 cmpOffsetNumbers(const void *a, const void *b)
 {
-	if (*(const OffsetNumber *) a == *(const OffsetNumber *) b)
-		return 0;
-	return (*(const OffsetNumber *) a > *(const OffsetNumber *) b) ? 1 : -1;
+	return pg_cmp_u16(*(const OffsetNumber *) a, *(const OffsetNumber *) b);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..d5db5225a9 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -40,6 +40,7 @@
 #include "postgres.h"
 
 #include "access/spgist.h"
+#include "common/int.h"
 #include "catalog/pg_type.h"
 #include "mb/pg_wchar.h"
 #include "utils/builtins.h"
@@ -325,7 +326,7 @@ cmpNodePtr(const void *a, const void *b)
 	const spgNodePtr *aa = (const spgNodePtr *) a;
 	const spgNodePtr *bb = (const spgNodePtr *) b;
 
-	return aa->c - bb->c;
+	return pg_cmp_s16(aa->c, bb->c);
 }
 
 Datum
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 0504c465db..d08dfb740b 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -27,6 +27,7 @@
 #include "common/blkreftable.h"
 #include "common/parse_manifest.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "postmaster/walsummarizer.h"
 
 #define	BLOCKS_PER_READ			512
@@ -994,10 +995,5 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return pg_cmp_u32(aa,bb);
 }
diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 867870aaad..4d047e1c02 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -17,6 +17,7 @@
 
 #include "access/xlog_internal.h"
 #include "backup/walsummary.h"
+#include "common/int.h"
 #include "utils/wait_event.h"
 
 static bool IsWalSummaryFilename(char *filename);
@@ -355,9 +356,5 @@ ListComparatorForWalSummaryFiles(const ListCell *a, const ListCell *b)
 	WalSummaryFile *ws1 = lfirst(a);
 	WalSummaryFile *ws2 = lfirst(b);
 
-	if (ws1->start_lsn < ws2->start_lsn)
-		return -1;
-	if (ws1->start_lsn > ws2->start_lsn)
-		return 1;
-	return 0;
+	return pg_cmp_u64(ws1->start_lsn, ws2->start_lsn);
 }
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c73f7bcd01..f8145d4cf5 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -56,6 +56,7 @@
 #include "catalog/storage.h"
 #include "commands/tablecmds.h"
 #include "commands/typecmds.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -2758,12 +2759,7 @@ list_cookedconstr_attnum_cmp(const ListCell *p1, const ListCell *p2)
 {
 	AttrNumber	v1 = ((CookedConstraint *) lfirst(p1))->attnum;
 	AttrNumber	v2 = ((CookedConstraint *) lfirst(p2))->attnum;
-
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_s16(v1, v2);
 }
 
 /*
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 957186bef5..e2615ab105 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -17,6 +17,7 @@
  */
 #include "postgres.h"
 
+#include "common/int.h"
 #include "nodes/pg_list.h"
 #include "port/pg_bitutils.h"
 #include "utils/memdebug.h"
@@ -1692,11 +1693,7 @@ list_int_cmp(const ListCell *p1, const ListCell *p2)
 	int			v1 = lfirst_int(p1);
 	int			v2 = lfirst_int(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_s32(v1, v2);
 }
 
 /*
@@ -1708,9 +1705,5 @@ list_oid_cmp(const ListCell *p1, const ListCell *p2)
 	Oid			v1 = lfirst_oid(p1);
 	Oid			v2 = lfirst_oid(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_u32(v1, v2);
 }
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index 0f4850065f..79e54ba111 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -42,6 +42,7 @@
 
 #include "access/htup_details.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "nodes/bitmapset.h"
 #include "nodes/tidbitmap.h"
 #include "storage/lwlock.h"
@@ -1425,11 +1426,7 @@ tbm_comparator(const void *left, const void *right)
 	BlockNumber l = (*((PagetableEntry *const *) left))->blockno;
 	BlockNumber r = (*((PagetableEntry *const *) right))->blockno;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return 1;
-	return 0;
+	return pg_cmp_u32(l,r);
 }
 
 /*
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 7b211a7743..9d151a880b 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -18,6 +18,7 @@
 #include "catalog/pg_aggregate.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -1760,7 +1761,7 @@ cmp_list_len_asc(const ListCell *a, const ListCell *b)
 	int			la = list_length((const List *) lfirst(a));
 	int			lb = list_length((const List *) lfirst(b));
 
-	return (la > lb) ? 1 : (la == lb) ? 0 : -1;
+	return pg_cmp_s32(la, lb);
 }
 
 /* list_sort comparator to sort sub-lists by length and contents */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2c3099f76f..483244254f 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -78,6 +78,7 @@
 #include "catalog/pg_database.h"
 #include "commands/dbcommands.h"
 #include "commands/vacuum.h"
+#include "common/int.h"
 #include "lib/ilist.h"
 #include "libpq/pqsignal.h"
 #include "miscadmin.h"
@@ -1119,10 +1120,7 @@ rebuild_database_list(Oid newdb)
 static int
 db_comparator(const void *a, const void *b)
 {
-	if (((const avl_dbase *) a)->adl_score == ((const avl_dbase *) b)->adl_score)
-		return 0;
-	else
-		return (((const avl_dbase *) a)->adl_score < ((const avl_dbase *) b)->adl_score) ? 1 : -1;
+	return pg_cmp_s32(((const avl_dbase *) a)->adl_score, ((const avl_dbase *) b)->adl_score);
 }
 
 /*
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index bbf0966182..5446df3c64 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -91,6 +91,7 @@
 #include "access/xact.h"
 #include "access/xlog_internal.h"
 #include "catalog/catalog.h"
+#include "common/int.h"
 #include "lib/binaryheap.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -5119,11 +5120,7 @@ file_sort_by_lsn(const ListCell *a_p, const ListCell *b_p)
 	RewriteMappingFile *a = (RewriteMappingFile *) lfirst(a_p);
 	RewriteMappingFile *b = (RewriteMappingFile *) lfirst(b_p);
 
-	if (a->lsn < b->lsn)
-		return -1;
-	else if (a->lsn > b->lsn)
-		return 1;
-	return 0;
+	return pg_cmp_u64(a->lsn, b->lsn);
 }
 
 /*
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 2e6493aaaa..bfcd8fa13e 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -75,6 +75,7 @@
 #include <unistd.h>
 
 #include "access/xact.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "replication/syncrep.h"
@@ -698,12 +699,7 @@ cmp_lsn(const void *a, const void *b)
 	XLogRecPtr	lsn1 = *((const XLogRecPtr *) a);
 	XLogRecPtr	lsn2 = *((const XLogRecPtr *) b);
 
-	if (lsn1 > lsn2)
-		return -1;
-	else if (lsn1 == lsn2)
-		return 0;
-	else
-		return 1;
+	return pg_cmp_u64(lsn2, lsn1);
 }
 
 /*
diff --git a/src/backend/utils/adt/oid.c b/src/backend/utils/adt/oid.c
index 62bcfc5b56..56fb1fd77c 100644
--- a/src/backend/utils/adt/oid.c
+++ b/src/backend/utils/adt/oid.c
@@ -18,6 +18,7 @@
 #include <limits.h>
 
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
 #include "nodes/value.h"
@@ -259,11 +260,7 @@ oid_cmp(const void *p1, const void *p2)
 	Oid			v1 = *((const Oid *) p1);
 	Oid			v2 = *((const Oid *) p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_u32(v1, v2);
 }
 
 
diff --git a/src/backend/utils/adt/tsgistidx.c b/src/backend/utils/adt/tsgistidx.c
index a62b285365..225f0d49cb 100644
--- a/src/backend/utils/adt/tsgistidx.c
+++ b/src/backend/utils/adt/tsgistidx.c
@@ -17,6 +17,7 @@
 #include "access/gist.h"
 #include "access/heaptoast.h"
 #include "access/reloptions.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "port/pg_bitutils.h"
 #include "tsearch/ts_utils.h"
@@ -136,9 +137,7 @@ compareint(const void *va, const void *vb)
 	int32		a = *((const int32 *) va);
 	int32		b = *((const int32 *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return pg_cmp_s32(a,b);
 }
 
 static void
@@ -598,10 +597,7 @@ comparecost(const void *va, const void *vb)
 	const SPLITCOST *a = (const SPLITCOST *) va;
 	const SPLITCOST *b = (const SPLITCOST *) vb;
 
-	if (a->cost == b->cost)
-		return 0;
-	else
-		return (a->cost > b->cost) ? 1 : -1;
+	return pg_cmp_s32(a->cost, b->cost);
 }
 
 
diff --git a/src/backend/utils/adt/tsquery_gist.c b/src/backend/utils/adt/tsquery_gist.c
index f0b1c81c81..0b3355f7fe 100644
--- a/src/backend/utils/adt/tsquery_gist.c
+++ b/src/backend/utils/adt/tsquery_gist.c
@@ -16,6 +16,7 @@
 
 #include "access/gist.h"
 #include "access/stratnum.h"
+#include "common/int.h"
 #include "tsearch/ts_utils.h"
 #include "utils/builtins.h"
 
@@ -156,10 +157,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	if (((const SPLITCOST *) a)->cost == ((const SPLITCOST *) b)->cost)
-		return 0;
-	else
-		return (((const SPLITCOST *) a)->cost > ((const SPLITCOST *) b)->cost) ? 1 : -1;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 #define WISH_F(a,b,c) (double)( -(double)(((a)-(b))*((a)-(b))*((a)-(b)))*(c) )
diff --git a/src/backend/utils/adt/tsvector.c b/src/backend/utils/adt/tsvector.c
index fb7b7c712a..10bc4f2234 100644
--- a/src/backend/utils/adt/tsvector.c
+++ b/src/backend/utils/adt/tsvector.c
@@ -14,6 +14,7 @@
 
 #include "postgres.h"
 
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
 #include "tsearch/ts_locale.h"
@@ -37,9 +38,7 @@ compareWordEntryPos(const void *a, const void *b)
 	int			apos = WEP_GETPOS(*(const WordEntryPos *) a);
 	int			bpos = WEP_GETPOS(*(const WordEntryPos *) b);
 
-	if (apos == bpos)
-		return 0;
-	return (apos > bpos) ? 1 : -1;
+	return pg_cmp_s32(apos, bpos);
 }
 
 /*
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index 71386d0a1f..70bdc09dd7 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -19,6 +19,7 @@
 #include "catalog/namespace.h"
 #include "catalog/pg_type.h"
 #include "commands/trigger.h"
+#include "common/int.h"
 #include "executor/spi.h"
 #include "funcapi.h"
 #include "lib/qunique.h"
@@ -435,9 +436,7 @@ compare_int(const void *va, const void *vb)
 	int			a = *((const int *) va);
 	int			b = *((const int *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return pg_cmp_s32(a,b);
 }
 
 static int
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 63adf5668b..ae273b1961 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -19,6 +19,7 @@
 #include "access/multixact.h"
 #include "access/transam.h"
 #include "access/xact.h"
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "utils/builtins.h"
 #include "utils/xid8.h"
@@ -140,11 +141,7 @@ xidComparator(const void *arg1, const void *arg2)
 	TransactionId xid1 = *(const TransactionId *) arg1;
 	TransactionId xid2 = *(const TransactionId *) arg2;
 
-	if (xid1 > xid2)
-		return 1;
-	if (xid1 < xid2)
-		return -1;
-	return 0;
+	return pg_cmp_u32(xid1, xid2);
 }
 
 /*
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..50acae4529 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -69,6 +69,7 @@
 #include "commands/policy.h"
 #include "commands/publicationcmds.h"
 #include "commands/trigger.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
@@ -4520,7 +4521,7 @@ AttrDefaultCmp(const void *a, const void *b)
 	const AttrDefault *ada = (const AttrDefault *) a;
 	const AttrDefault *adb = (const AttrDefault *) b;
 
-	return ada->adnum - adb->adnum;
+	return pg_cmp_s16(ada->adnum, adb->adnum);
 }
 
 /*
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..a7c9c23399 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -29,6 +29,7 @@
 #include "catalog/pg_shdepend_d.h"
 #include "catalog/pg_shdescription_d.h"
 #include "catalog/pg_shseclabel_d.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "utils/catcache.h"
 #include "utils/lsyscache.h"
@@ -676,7 +677,5 @@ oid_compare(const void *a, const void *b)
 	Oid			oa = *((const Oid *) a);
 	Oid			ob = *((const Oid *) b);
 
-	if (oa == ob)
-		return 0;
-	return (oa > ob) ? 1 : -1;
+	return pg_cmp_u32(oa, ob);
 }
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 84fc83cb11..2842bde907 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -57,6 +57,7 @@
 #include "catalog/pg_range.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
+#include "common/int.h"
 #include "executor/executor.h"
 #include "lib/dshash.h"
 #include "optimizer/optimizer.h"
@@ -2722,12 +2723,7 @@ enum_oid_cmp(const void *left, const void *right)
 	const EnumItem *l = (const EnumItem *) left;
 	const EnumItem *r = (const EnumItem *) right;
 
-	if (l->enum_oid < r->enum_oid)
-		return -1;
-	else if (l->enum_oid > r->enum_oid)
-		return 1;
-	else
-		return 0;
+	return pg_cmp_u32(l->enum_oid, r->enum_oid);
 }
 
 /*
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index aa199b23ff..a75efd9adc 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -46,6 +46,7 @@
 #include "postgres.h"
 
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "storage/ipc.h"
 #include "storage/predicate.h"
 #include "storage/proc.h"
@@ -264,14 +265,7 @@ resource_priority_cmp(const void *a, const void *b)
 
 	/* Note: reverse order */
 	if (ra->kind->release_phase == rb->kind->release_phase)
-	{
-		if (ra->kind->release_priority == rb->kind->release_priority)
-			return 0;
-		else if (ra->kind->release_priority > rb->kind->release_priority)
-			return -1;
-		else
-			return 1;
-	}
+		return pg_cmp_u32(ra->kind->release_priority, rb->kind->release_priority);
 	else if (ra->kind->release_phase > rb->kind->release_phase)
 		return -1;
 	else
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index f358dd22b9..8ee8a42781 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -16,6 +16,7 @@
 #include "postgres_fe.h"
 
 #include "catalog/pg_class_d.h"
+#include "common/int.h"
 #include "lib/binaryheap.h"
 #include "pg_backup_archiver.h"
 #include "pg_backup_utils.h"
@@ -1504,9 +1505,5 @@ int_cmp(void *a, void *b, void *arg)
 	int			ai = (int) (intptr_t) a;
 	int			bi = (int) (intptr_t) b;
 
-	if (ai < bi)
-		return -1;
-	if (ai > bi)
-		return 1;
-	return 0;
+	return pg_cmp_s32(ai, bi);
 }
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index af998f74d3..ab1870d012 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -11,6 +11,7 @@
 
 #include "access/transam.h"
 #include "catalog/pg_language_d.h"
+#include "common/int.h"
 #include "pg_upgrade.h"
 
 /*
@@ -29,17 +30,17 @@ library_name_compare(const void *p1, const void *p2)
 {
 	const char *str1 = ((const LibraryInfo *) p1)->name;
 	const char *str2 = ((const LibraryInfo *) p2)->name;
-	int			slen1 = strlen(str1);
-	int			slen2 = strlen(str2);
+	size_t			slen1 = strlen(str1);
+	size_t			slen2 = strlen(str2);
 	int			cmp = strcmp(str1, str2);
+	int lhsdb = ((const LibraryInfo *) p1)->dbnum;
+	int rhsdb = ((const LibraryInfo *) p2)->dbnum;
 
 	if (slen1 != slen2)
-		return slen1 - slen2;
+		return pg_cmp_s32(slen1, slen2);
 	if (cmp != 0)
 		return cmp;
-	else
-		return ((const LibraryInfo *) p1)->dbnum -
-			((const LibraryInfo *) p2)->dbnum;
+	return pg_cmp_s32(lhsdb, rhsdb);
 }
 
 
diff --git a/src/bin/pg_walsummary/pg_walsummary.c b/src/bin/pg_walsummary/pg_walsummary.c
index 1341c83c69..39f77d159b 100644
--- a/src/bin/pg_walsummary/pg_walsummary.c
+++ b/src/bin/pg_walsummary/pg_walsummary.c
@@ -17,6 +17,7 @@
 
 #include "common/blkreftable.h"
 #include "common/logging.h"
+#include "common/int.h"
 #include "fe_utils/option_utils.h"
 #include "lib/stringinfo.h"
 #include "getopt_long.h"
@@ -219,12 +220,7 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return pg_cmp_u32(aa, bb);
 }
 
 /*
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index c6116b7238..305ed4ab0a 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -8,6 +8,7 @@
 #include "postgres_fe.h"
 
 #include "common.h"
+#include "common/int.h"
 #include "common/logging.h"
 #include "crosstabview.h"
 #include "pqexpbuffer.h"
@@ -709,5 +710,5 @@ pivotFieldCompare(const void *a, const void *b)
 static int
 rankCompare(const void *a, const void *b)
 {
-	return *((const int *) a) - *((const int *) b);
+	return pg_cmp_s32(*(const int *) a, *(const int *) b);
 }
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 51d0c74a6b..3013a44bae 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -14,6 +14,7 @@
 #include "access/gin.h"
 #include "access/ginblock.h"
 #include "access/itup.h"
+#include "common/int.h"
 #include "catalog/pg_am_d.h"
 #include "fmgr.h"
 #include "lib/rbtree.h"
@@ -489,12 +490,7 @@ ginCompareItemPointers(ItemPointer a, ItemPointer b)
 	uint64		ia = (uint64) GinItemPointerGetBlockNumber(a) << 32 | GinItemPointerGetOffsetNumber(a);
 	uint64		ib = (uint64) GinItemPointerGetBlockNumber(b) << 32 | GinItemPointerGetOffsetNumber(b);
 
-	if (ia == ib)
-		return 0;
-	else if (ia > ib)
-		return 1;
-	else
-		return -1;
+	return pg_cmp_u64(ia, ib);
 }
 
 extern int	ginTraverseLock(Buffer buffer, bool searchMode);
diff --git a/src/include/common/int.h b/src/include/common/int.h
index fedf7b3999..3123d8d59d 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -438,4 +438,45 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
 #endif
 }
 
+/*------------------------------------------------------------------------
+ * Comparison routines for integers
+ *------------------------------------------------------------------------
+ */
+
+static inline int
+pg_cmp_s16(int16 a, int16 b)
+{
+	return a - b;
+}
+
+static inline int
+pg_cmp_u16(uint16 a, uint16 b)
+{
+	return a - b;
+}
+
+static inline int
+pg_cmp_s32(int32 a, int32 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u32(uint32 a, uint32 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_s64(int64 a, int64 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u64(uint64 a, uint64 b)
+{
+	return (a > b) - (a < b);
+}
+
 #endif							/* COMMON_INT_H */
-- 
2.34.1

#32Andrey M. Borodin
x4mmm@yandex-team.ru
In reply to: Nathan Bossart (#13)
Re: glibc qsort() vulnerability

On 8 Feb 2024, at 06:52, Nathan Bossart <nathandbossart@gmail.com> wrote:

For the same compASC() test, I see an ~8.4% improvement with your int64
code and a ~3.4% improvement with this:

If we care about branch prediction in comparison function, maybe we could produce sorting that inlines comparator, thus eliminating function call to comparator? We convert comparison logic to int, to extract comparison back then.

I bet “call" is more expensive than “if".

Best regards, Andrey Borodin.

#33Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#31)
Re: glibc qsort() vulnerability

On Fri, Feb 09, 2024 at 08:52:26AM +0100, Mats Kindahl wrote:

Here is a new version introducing pg_cmp_s32 and friends and use them
instead of the INT_CMP macro introduced before. It also moves the
definitions to common/int.h and adds that as an include to all locations
using these functions.

Thanks for the new version of the patch.

Note that for integers with sizes less than sizeof(int), C standard
conversions will convert the values to "int" before doing the arithmetic,
so no casting is *necessary*. I did not force the 16-bit functions to
return -1 or 1 and have updated the comment accordingly.

It might not be necessary, but this is one of those places where I would
add casting anyway to make it abundantly clear what we are expecting to
happen and why it is safe.

The types "int" and "size_t" are treated as s32 and u32 respectively since
that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

Why is it safe to do this?

-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return INT_CMP(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);

The patch still contains several calls to INT_CMP.

+/*------------------------------------------------------------------------
+ * Comparison routines for integers
+ *------------------------------------------------------------------------
+ */

I'd suggest separating this part out to a 0001 patch to make it easier to
review. The 0002 patch could take care of converting the existing qsort
comparators.

+static inline int
+pg_cmp_s16(int16 a, int16 b)
+{
+	return a - b;
+}
+
+static inline int
+pg_cmp_u16(uint16 a, uint16 b)
+{
+	return a - b;
+}
+
+static inline int
+pg_cmp_s32(int32 a, int32 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u32(uint32 a, uint32 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_s64(int64 a, int64 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u64(uint64 a, uint64 b)
+{
+	return (a > b) - (a < b);
+}

As suggested above, IMHO we should be rather liberal with the casting to
ensure it is abundantly clear what is happening here.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#34Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nathan Bossart (#33)
Re: glibc qsort() vulnerability

Nathan Bossart <nathandbossart@gmail.com> writes:

On Fri, Feb 09, 2024 at 08:52:26AM +0100, Mats Kindahl wrote:

The types "int" and "size_t" are treated as s32 and u32 respectively since
that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

Why is it safe to do this?

We do pretty much assume that "int" is "int32". But I agree that
assuming anything about the width of size_t is bad. I think we need
a separate pg_cmp_size() or pg_cmp_size_t().

regards, tom lane

#35Nathan Bossart
nathandbossart@gmail.com
In reply to: Andrey M. Borodin (#32)
Re: glibc qsort() vulnerability

On Fri, Feb 09, 2024 at 01:19:49PM +0500, Andrey M. Borodin wrote:

If we care about branch prediction in comparison function, maybe we could
produce sorting that inlines comparator, thus eliminating function call
to comparator? We convert comparison logic to int, to extract comparison
back then.

I bet “call" is more expensive than “if".

It might make sense to have a couple of built-in qsort implementations for
pointers to integers, pointers to unsigned integers, etc. However, a lot
of current use-cases require inspecting specific fields of structs, so
(assuming I understand your proposal correctly), we'd end up with many
qsort implementations. If that can be made simple and elegant and
demonstrates substantial improvements, then it might be worth considering,
but I'm somewhat skeptical that the current uses are performance-sensitive
enough to be worth the effort.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#36Andres Freund
andres@anarazel.de
In reply to: Andrey M. Borodin (#32)
Re: glibc qsort() vulnerability

Hi,

On 2024-02-09 13:19:49 +0500, Andrey M. Borodin wrote:

On 8 Feb 2024, at 06:52, Nathan Bossart <nathandbossart@gmail.com> wrote:
For the same compASC() test, I see an ~8.4% improvement with your int64
code and a ~3.4% improvement with this:

If we care about branch prediction in comparison function, maybe we could
produce sorting that inlines comparator, thus eliminating function call to
comparator? We convert comparison logic to int, to extract comparison back
then.

We have some infrastructure for that actually, see sort_template.h. But
perhaps we should define a static inline of the generic pg_qsort() even. OTOH,
plenty places that'll just end up to a pointless amount of code emitted to
sort ~5 elements on average.

I bet “call" is more expensive than “if".

Not really in this case. The call is perfectly predictable - a single qsort()
will use the same callback for every comparison, whereas the if is perfectly
*unpredictable*. A branch mispredict is far more expensive than a correctly
predicted function call.

Greetings,

Andres

#37Andrey Borodin
x4mmm@yandex-team.ru
In reply to: Nathan Bossart (#35)
Re: glibc qsort() vulnerability

On 9 Feb 2024, at 21:32, Nathan Bossart <nathandbossart@gmail.com> wrote:
a lot
of current use-cases require inspecting specific fields of structs

Yes, I'm proposing to pass to sorting routine not a comparator, but value extractor. And then rely on operators <,>,==.
In a pseudocode: instead of sort(array, (a,b)->a.field-b.field) use sort(array, x->x.field). And rely on "field" being comparable.

If that can be made simple and elegant and
demonstrates substantial improvements

I'll try to produce a PoC and measure it with Andres' intarray test.

On 9 Feb 2024, at 23:40, Andres Freund <andres@anarazel.de> wrote:

We have some infrastructure for that actually, see sort_template.h. But
perhaps we should define a static inline of the generic pg_qsort() even. OTOH,
plenty places that'll just end up to a pointless amount of code emitted to
sort ~5 elements on average.

I think there might be another benefit. It's easier to think about values order than function comparator that returns -1,0,+1...

I bet “call" is more expensive than “if".

Not really in this case. The call is perfectly predictable - a single qsort()
will use the same callback for every comparison, whereas the if is perfectly
*unpredictable*. A branch mispredict is far more expensive than a correctly
predicted function call.

Oh, make sense... I did not understand that. But does cpu predicts what instruction to fetch even after a call instruction? These cpus are really neat things... so, probably, yes, it does.

Best regards, Andrey Borodin.

#38Andres Freund
andres@anarazel.de
In reply to: Andrey Borodin (#37)
Re: glibc qsort() vulnerability

Hi,

On 2024-02-10 00:02:08 +0500, Andrey Borodin wrote:

Not really in this case. The call is perfectly predictable - a single qsort()
will use the same callback for every comparison, whereas the if is perfectly
*unpredictable*. A branch mispredict is far more expensive than a correctly
predicted function call.

Oh, make sense... I did not understand that. But does cpu predicts what
instruction to fetch even after a call instruction?

Yes, it does predict that. Both for branches and calls (which are just special
kinds of branches in the end). If you want to find more about this, the term
to search for is "branch target buffer". There's also predictions about where
a function return will jump to, since that obviously can differ.

Modern predictors aren't just taking the instruction pointer into account, to
predict where a jump/call will go to. Tey take the history of recent branches
into account, i.e. the same instruction will be predicted to jump to different
locations, depending on where the current function was called from. This is
important as a function obviously can behave very differently depending on the
input.

These cpus are really neat things...

Indeed.

Greetings,

Andres Freund

#39Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#33)
Re: glibc qsort() vulnerability

On Fri, Feb 9, 2024 at 5:24 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Fri, Feb 09, 2024 at 08:52:26AM +0100, Mats Kindahl wrote:

Here is a new version introducing pg_cmp_s32 and friends and use them
instead of the INT_CMP macro introduced before. It also moves the
definitions to common/int.h and adds that as an include to all locations
using these functions.

Thanks for the new version of the patch.

Note that for integers with sizes less than sizeof(int), C standard
conversions will convert the values to "int" before doing the arithmetic,
so no casting is *necessary*. I did not force the 16-bit functions to
return -1 or 1 and have updated the comment accordingly.

It might not be necessary, but this is one of those places where I would
add casting anyway to make it abundantly clear what we are expecting to
happen and why it is safe.

I'll add it.

The types "int" and "size_t" are treated as s32 and u32 respectively

since

that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

Why is it safe to do this?

- return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *)

b)->cost;

+ return INT_CMP(((const SPLITCOST *) a)->cost, ((const SPLITCOST *)

b)->cost);

The patch still contains several calls to INT_CMP.

I'll fix it.

+/*------------------------------------------------------------------------

+ * Comparison routines for integers
+

*------------------------------------------------------------------------

+ */

I'd suggest separating this part out to a 0001 patch to make it easier to
review. The 0002 patch could take care of converting the existing qsort
comparators.

Ok. Will split it out into two patches.

+static inline int
+pg_cmp_s16(int16 a, int16 b)
+{
+     return a - b;
+}
+
+static inline int
+pg_cmp_u16(uint16 a, uint16 b)
+{
+     return a - b;
+}
+
+static inline int
+pg_cmp_s32(int32 a, int32 b)
+{
+     return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u32(uint32 a, uint32 b)
+{
+     return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_s64(int64 a, int64 b)
+{
+     return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u64(uint64 a, uint64 b)
+{
+     return (a > b) - (a < b);
+}

As suggested above, IMHO we should be rather liberal with the casting to
ensure it is abundantly clear what is happening here.

Ok.

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#40Mats Kindahl
mats@timescale.com
In reply to: Tom Lane (#34)
Re: glibc qsort() vulnerability

On Fri, Feb 9, 2024 at 5:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

On Fri, Feb 09, 2024 at 08:52:26AM +0100, Mats Kindahl wrote:

The types "int" and "size_t" are treated as s32 and u32 respectively

since

that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

Why is it safe to do this?

We do pretty much assume that "int" is "int32". But I agree that
assuming anything about the width of size_t is bad. I think we need
a separate pg_cmp_size() or pg_cmp_size_t().

I added precisely one first, but removed it when I saw that all uses
assumed that it was an int. :)

I'll add it back.

Best wishes,
Mats Kindahl

Show quoted text

regards, tom lane

#41Mats Kindahl
mats@timescale.com
In reply to: Tom Lane (#34)
Re: glibc qsort() vulnerability

On Fri, Feb 9, 2024 at 5:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

On Fri, Feb 09, 2024 at 08:52:26AM +0100, Mats Kindahl wrote:

The types "int" and "size_t" are treated as s32 and u32 respectively

since

that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

Why is it safe to do this?

We do pretty much assume that "int" is "int32". But I agree that
assuming anything about the width of size_t is bad. I think we need
a separate pg_cmp_size() or pg_cmp_size_t().

Do we want to have something similar for "int" as well? It seems to be
quite common and even though it usually is an int32, it does not have to be.

Best wishes,
Mats Kindahl

Show quoted text

regards, tom lane

#42Mats Kindahl
mats@timescale.com
In reply to: Mats Kindahl (#39)
Re: glibc qsort() vulnerability

On Fri, Feb 9, 2024 at 8:26 PM Mats Kindahl <mats@timescale.com> wrote:

On Fri, Feb 9, 2024 at 5:24 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Fri, Feb 09, 2024 at 08:52:26AM +0100, Mats Kindahl wrote:

Here is a new version introducing pg_cmp_s32 and friends and use them
instead of the INT_CMP macro introduced before. It also moves the
definitions to common/int.h and adds that as an include to all locations
using these functions.

Thanks for the new version of the patch.

Note that for integers with sizes less than sizeof(int), C standard
conversions will convert the values to "int" before doing the

arithmetic,

so no casting is *necessary*. I did not force the 16-bit functions to
return -1 or 1 and have updated the comment accordingly.

It might not be necessary, but this is one of those places where I would
add casting anyway to make it abundantly clear what we are expecting to
happen and why it is safe.

I'll add it.

The types "int" and "size_t" are treated as s32 and u32 respectively

since

that seems to be the case for most of the code, even if strictly not
correct (size_t can be an unsigned long int for some architecture).

Why is it safe to do this?

- return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *)

b)->cost;

+ return INT_CMP(((const SPLITCOST *) a)->cost, ((const SPLITCOST

*) b)->cost);

The patch still contains several calls to INT_CMP.

I'll fix it.

+/*------------------------------------------------------------------------

+ * Comparison routines for integers
+

*------------------------------------------------------------------------

+ */

I'd suggest separating this part out to a 0001 patch to make it easier to
review. The 0002 patch could take care of converting the existing qsort
comparators.

Ok. Will split it out into two patches.

+static inline int
+pg_cmp_s16(int16 a, int16 b)
+{
+     return a - b;
+}
+
+static inline int
+pg_cmp_u16(uint16 a, uint16 b)
+{
+     return a - b;
+}
+
+static inline int
+pg_cmp_s32(int32 a, int32 b)
+{
+     return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u32(uint32 a, uint32 b)
+{
+     return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_s64(int64 a, int64 b)
+{
+     return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u64(uint64 a, uint64 b)
+{
+     return (a > b) - (a < b);
+}

As suggested above, IMHO we should be rather liberal with the casting to
ensure it is abundantly clear what is happening here.

Ok.

QQ: right now it looks like this:

static inline int
pg_cmp_u16(uint16 a, uint16 b)
{

return (int32)a - (int32)b;

}

and

static inline int
pg_cmp_u32(uint32 a, uint32 b)
{

return (a > b) - (a < b);

}

I think that is clear enough, but do you want more casts added for the
return value as well?

Best wishes,
Mats Kindahl

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#43Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#41)
Re: glibc qsort() vulnerability

On Fri, Feb 09, 2024 at 08:40:47PM +0100, Mats Kindahl wrote:

On Fri, Feb 9, 2024 at 5:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

We do pretty much assume that "int" is "int32". But I agree that
assuming anything about the width of size_t is bad. I think we need
a separate pg_cmp_size() or pg_cmp_size_t().

Do we want to have something similar for "int" as well? It seems to be
quite common and even though it usually is an int32, it does not have to be.

I don't think we need separate functions for int and int32. As Tom noted,
we assume they are the same.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#44Andres Freund
andres@anarazel.de
In reply to: Nathan Bossart (#43)
Re: glibc qsort() vulnerability

On 2024-02-09 14:04:29 -0600, Nathan Bossart wrote:

On Fri, Feb 09, 2024 at 08:40:47PM +0100, Mats Kindahl wrote:

On Fri, Feb 9, 2024 at 5:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

We do pretty much assume that "int" is "int32". But I agree that
assuming anything about the width of size_t is bad. I think we need
a separate pg_cmp_size() or pg_cmp_size_t().

Do we want to have something similar for "int" as well? It seems to be
quite common and even though it usually is an int32, it does not have to be.

I don't think we need separate functions for int and int32. As Tom noted,
we assume they are the same.

+1

#45Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#42)
Re: glibc qsort() vulnerability

On Fri, Feb 09, 2024 at 08:43:21PM +0100, Mats Kindahl wrote:

QQ: right now it looks like this:

static inline int
pg_cmp_u16(uint16 a, uint16 b)
{

return (int32)a - (int32)b;

}

and

static inline int
pg_cmp_u32(uint32 a, uint32 b)
{

return (a > b) - (a < b);

}

I think that is clear enough, but do you want more casts added for the
return value as well?

I think that is reasonably clear. The latter does require you to know that
< and > return (int) 0 or (int) 1, which might be worth a short comment.
But that's just nitpicking...

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#46Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#45)
2 attachment(s)
Re: glibc qsort() vulnerability

On Fri, Feb 9, 2024 at 9:08 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Fri, Feb 09, 2024 at 08:43:21PM +0100, Mats Kindahl wrote:

QQ: right now it looks like this:

static inline int
pg_cmp_u16(uint16 a, uint16 b)
{

return (int32)a - (int32)b;

}

and

static inline int
pg_cmp_u32(uint32 a, uint32 b)
{

return (a > b) - (a < b);

}

I think that is clear enough, but do you want more casts added for the
return value as well?

I think that is reasonably clear. The latter does require you to know that
< and > return (int) 0 or (int) 1, which might be worth a short comment.
But that's just nitpicking...

Hi all,

Split the code into two patches: one that just adds the functions
(including the new pg_cmp_size()) to common/int.h and one that starts using
them. I picked the name "pg_cmp_size" rather than "pg_cmp_size_t" since
"_t" is usually used as a suffix for types.

I added a comment to the (a > b) - (a < b) return and have also added casts
to (int32) for the int16 and uint16 functions (we need a signed int for
uin16 since we need to be able to get a negative number).

Changed the type of two instances that had an implicit cast from size_t to
int and used the new pg_,cmp_size() function.

Also fixed the missed replacements in the "contrib" directory.

Best wishes,
Mats Kindahl

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

0002-Use-integer-comparison-functions.patchapplication/x-patch; name=0002-Use-integer-comparison-functions.patchDownload
From 84ed91e96f950cdb92952c8248b2c754d3c4a034 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Fri, 9 Feb 2024 21:48:27 +0100
Subject: Use integer comparison functions

Use overflow-safe integer functions when implementing
qsort() comparison functions.
---
 contrib/hstore/hstore_gist.c                    |  3 ++-
 contrib/intarray/_intbig_gist.c                 |  3 ++-
 contrib/pg_stat_statements/pg_stat_statements.c |  8 ++------
 contrib/pg_trgm/trgm_op.c                       |  8 ++------
 src/backend/access/nbtree/nbtinsert.c           |  8 ++------
 src/backend/access/nbtree/nbtpage.c             | 10 +++-------
 src/backend/access/nbtree/nbtsplitloc.c         |  8 ++------
 src/backend/access/spgist/spgdoinsert.c         |  5 ++---
 src/backend/access/spgist/spgtextproc.c         |  3 ++-
 src/backend/backup/basebackup_incremental.c     |  8 ++------
 src/backend/backup/walsummary.c                 |  7 ++-----
 src/backend/catalog/heap.c                      |  8 ++------
 src/backend/nodes/list.c                        | 13 +++----------
 src/backend/nodes/tidbitmap.c                   |  7 ++-----
 src/backend/parser/parse_agg.c                  |  7 ++++---
 src/backend/postmaster/autovacuum.c             |  6 ++----
 src/backend/replication/logical/reorderbuffer.c |  7 ++-----
 src/backend/replication/syncrep.c               |  8 ++------
 src/backend/utils/adt/oid.c                     |  7 ++-----
 src/backend/utils/adt/tsgistidx.c               | 10 +++-------
 src/backend/utils/adt/tsquery_gist.c            |  6 ++----
 src/backend/utils/adt/tsvector.c                |  5 ++---
 src/backend/utils/adt/tsvector_op.c             |  5 ++---
 src/backend/utils/adt/xid.c                     |  7 ++-----
 src/backend/utils/cache/relcache.c              |  3 ++-
 src/backend/utils/cache/syscache.c              |  5 ++---
 src/backend/utils/cache/typcache.c              |  8 ++------
 src/backend/utils/resowner/resowner.c           | 10 ++--------
 src/bin/pg_dump/pg_dump_sort.c                  |  7 ++-----
 src/bin/pg_upgrade/function.c                   | 13 +++++++------
 src/bin/pg_walsummary/pg_walsummary.c           |  8 ++------
 src/bin/psql/crosstabview.c                     |  3 ++-
 src/include/access/gin_private.h                |  8 ++------
 33 files changed, 76 insertions(+), 156 deletions(-)

diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index fe343739eb..1079f25bf7 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -7,6 +7,7 @@
 #include "access/reloptions.h"
 #include "access/stratnum.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "hstore.h"
 #include "utils/pg_crc.h"
 
@@ -356,7 +357,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
index 8c6c4b5d05..aef9e13dcc 100644
--- a/contrib/intarray/_intbig_gist.c
+++ b/contrib/intarray/_intbig_gist.c
@@ -9,6 +9,7 @@
 #include "access/gist.h"
 #include "access/reloptions.h"
 #include "access/stratnum.h"
+#include "common/int.h"
 #include "port/pg_bitutils.h"
 
 #define GETENTRY(vec,pos) ((GISTTYPE *) DatumGetPointer((vec)->vector[(pos)].key))
@@ -312,7 +313,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost,  ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 8c6a3a2d08..67cec865ba 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -50,6 +50,7 @@
 #include "access/parallel.h"
 #include "catalog/pg_authid.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "executor/instrument.h"
 #include "funcapi.h"
 #include "jit/jit.h"
@@ -3007,10 +3008,5 @@ comp_location(const void *a, const void *b)
 	int			l = ((const LocationLen *) a)->location;
 	int			r = ((const LocationLen *) b)->location;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return +1;
-	else
-		return 0;
+	return pg_cmp_s32(l, r);
 }
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 49d4497b4f..c509d15ee4 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -6,6 +6,7 @@
 #include <ctype.h>
 
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "miscadmin.h"
 #include "trgm.h"
@@ -433,12 +434,7 @@ comp_ptrgm(const void *v1, const void *v2)
 	if (cmp != 0)
 		return cmp;
 
-	if (p1->index < p2->index)
-		return -1;
-	else if (p1->index == p2->index)
-		return 0;
-	else
-		return 1;
+	return pg_cmp_s32(p1->index, p2->index);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 709edd1c17..9cfe4cc094 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -20,6 +20,7 @@
 #include "access/transam.h"
 #include "access/xloginsert.h"
 #include "common/pg_prng.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
@@ -3013,10 +3014,5 @@ _bt_blk_cmp(const void *arg1, const void *arg2)
 	BlockNumber b1 = *((BlockNumber *) arg1);
 	BlockNumber b2 = *((BlockNumber *) arg2);
 
-	if (b1 < b2)
-		return -1;
-	else if (b1 > b2)
-		return 1;
-
-	return 0;
+	return pg_cmp_u32(b1, b2);
 }
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 567bade9f4..0f712cea57 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -28,6 +28,7 @@
 #include "access/transam.h"
 #include "access/xlog.h"
 #include "access/xloginsert.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "storage/indexfsm.h"
 #include "storage/lmgr.h"
@@ -1466,14 +1467,9 @@ _bt_delitems_cmp(const void *a, const void *b)
 	TM_IndexDelete *indexdelete1 = (TM_IndexDelete *) a;
 	TM_IndexDelete *indexdelete2 = (TM_IndexDelete *) b;
 
-	if (indexdelete1->id > indexdelete2->id)
-		return 1;
-	if (indexdelete1->id < indexdelete2->id)
-		return -1;
+	Assert(pg_cmp_s16(indexdelete1->id, indexdelete2->id) != 0);
 
-	Assert(false);
-
-	return 0;
+	return pg_cmp_s16(indexdelete1->id, indexdelete2->id);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtsplitloc.c b/src/backend/access/nbtree/nbtsplitloc.c
index d0b1d82578..0bcc8e90ae 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -15,6 +15,7 @@
 #include "postgres.h"
 
 #include "access/nbtree.h"
+#include "common/int.h"
 #include "storage/lmgr.h"
 
 typedef enum
@@ -596,12 +597,7 @@ _bt_splitcmp(const void *arg1, const void *arg2)
 	SplitPoint *split1 = (SplitPoint *) arg1;
 	SplitPoint *split2 = (SplitPoint *) arg2;
 
-	if (split1->curdelta > split2->curdelta)
-		return 1;
-	if (split1->curdelta < split2->curdelta)
-		return -1;
-
-	return 0;
+	return pg_cmp_s16(split1->curdelta,  split2->curdelta);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index bb063c858d..8695370139 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -20,6 +20,7 @@
 #include "access/spgxlog.h"
 #include "access/xloginsert.h"
 #include "common/pg_prng.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
 #include "utils/rel.h"
@@ -110,9 +111,7 @@ addNode(SpGistState *state, SpGistInnerTuple tuple, Datum label, int offset)
 static int
 cmpOffsetNumbers(const void *a, const void *b)
 {
-	if (*(const OffsetNumber *) a == *(const OffsetNumber *) b)
-		return 0;
-	return (*(const OffsetNumber *) a > *(const OffsetNumber *) b) ? 1 : -1;
+	return pg_cmp_u16(*(const OffsetNumber *) a, *(const OffsetNumber *) b);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..d5db5225a9 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -40,6 +40,7 @@
 #include "postgres.h"
 
 #include "access/spgist.h"
+#include "common/int.h"
 #include "catalog/pg_type.h"
 #include "mb/pg_wchar.h"
 #include "utils/builtins.h"
@@ -325,7 +326,7 @@ cmpNodePtr(const void *a, const void *b)
 	const spgNodePtr *aa = (const spgNodePtr *) a;
 	const spgNodePtr *bb = (const spgNodePtr *) b;
 
-	return aa->c - bb->c;
+	return pg_cmp_s16(aa->c, bb->c);
 }
 
 Datum
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 0504c465db..d08dfb740b 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -27,6 +27,7 @@
 #include "common/blkreftable.h"
 #include "common/parse_manifest.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "postmaster/walsummarizer.h"
 
 #define	BLOCKS_PER_READ			512
@@ -994,10 +995,5 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return pg_cmp_u32(aa,bb);
 }
diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 867870aaad..4d047e1c02 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -17,6 +17,7 @@
 
 #include "access/xlog_internal.h"
 #include "backup/walsummary.h"
+#include "common/int.h"
 #include "utils/wait_event.h"
 
 static bool IsWalSummaryFilename(char *filename);
@@ -355,9 +356,5 @@ ListComparatorForWalSummaryFiles(const ListCell *a, const ListCell *b)
 	WalSummaryFile *ws1 = lfirst(a);
 	WalSummaryFile *ws2 = lfirst(b);
 
-	if (ws1->start_lsn < ws2->start_lsn)
-		return -1;
-	if (ws1->start_lsn > ws2->start_lsn)
-		return 1;
-	return 0;
+	return pg_cmp_u64(ws1->start_lsn, ws2->start_lsn);
 }
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c73f7bcd01..f8145d4cf5 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -56,6 +56,7 @@
 #include "catalog/storage.h"
 #include "commands/tablecmds.h"
 #include "commands/typecmds.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -2758,12 +2759,7 @@ list_cookedconstr_attnum_cmp(const ListCell *p1, const ListCell *p2)
 {
 	AttrNumber	v1 = ((CookedConstraint *) lfirst(p1))->attnum;
 	AttrNumber	v2 = ((CookedConstraint *) lfirst(p2))->attnum;
-
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_s16(v1, v2);
 }
 
 /*
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 957186bef5..e2615ab105 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -17,6 +17,7 @@
  */
 #include "postgres.h"
 
+#include "common/int.h"
 #include "nodes/pg_list.h"
 #include "port/pg_bitutils.h"
 #include "utils/memdebug.h"
@@ -1692,11 +1693,7 @@ list_int_cmp(const ListCell *p1, const ListCell *p2)
 	int			v1 = lfirst_int(p1);
 	int			v2 = lfirst_int(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_s32(v1, v2);
 }
 
 /*
@@ -1708,9 +1705,5 @@ list_oid_cmp(const ListCell *p1, const ListCell *p2)
 	Oid			v1 = lfirst_oid(p1);
 	Oid			v2 = lfirst_oid(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_u32(v1, v2);
 }
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index 0f4850065f..79e54ba111 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -42,6 +42,7 @@
 
 #include "access/htup_details.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "nodes/bitmapset.h"
 #include "nodes/tidbitmap.h"
 #include "storage/lwlock.h"
@@ -1425,11 +1426,7 @@ tbm_comparator(const void *left, const void *right)
 	BlockNumber l = (*((PagetableEntry *const *) left))->blockno;
 	BlockNumber r = (*((PagetableEntry *const *) right))->blockno;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return 1;
-	return 0;
+	return pg_cmp_u32(l,r);
 }
 
 /*
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 7b211a7743..a50d799f03 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -18,6 +18,7 @@
 #include "catalog/pg_aggregate.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -1757,10 +1758,10 @@ expand_groupingset_node(GroupingSet *gs)
 static int
 cmp_list_len_asc(const ListCell *a, const ListCell *b)
 {
-	int			la = list_length((const List *) lfirst(a));
-	int			lb = list_length((const List *) lfirst(b));
+	size_t		la = list_length((const List *) lfirst(a));
+	size_t		lb = list_length((const List *) lfirst(b));
 
-	return (la > lb) ? 1 : (la == lb) ? 0 : -1;
+	return pg_cmp_size(la, lb);
 }
 
 /* list_sort comparator to sort sub-lists by length and contents */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2c3099f76f..483244254f 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -78,6 +78,7 @@
 #include "catalog/pg_database.h"
 #include "commands/dbcommands.h"
 #include "commands/vacuum.h"
+#include "common/int.h"
 #include "lib/ilist.h"
 #include "libpq/pqsignal.h"
 #include "miscadmin.h"
@@ -1119,10 +1120,7 @@ rebuild_database_list(Oid newdb)
 static int
 db_comparator(const void *a, const void *b)
 {
-	if (((const avl_dbase *) a)->adl_score == ((const avl_dbase *) b)->adl_score)
-		return 0;
-	else
-		return (((const avl_dbase *) a)->adl_score < ((const avl_dbase *) b)->adl_score) ? 1 : -1;
+	return pg_cmp_s32(((const avl_dbase *) a)->adl_score, ((const avl_dbase *) b)->adl_score);
 }
 
 /*
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index bbf0966182..5446df3c64 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -91,6 +91,7 @@
 #include "access/xact.h"
 #include "access/xlog_internal.h"
 #include "catalog/catalog.h"
+#include "common/int.h"
 #include "lib/binaryheap.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -5119,11 +5120,7 @@ file_sort_by_lsn(const ListCell *a_p, const ListCell *b_p)
 	RewriteMappingFile *a = (RewriteMappingFile *) lfirst(a_p);
 	RewriteMappingFile *b = (RewriteMappingFile *) lfirst(b_p);
 
-	if (a->lsn < b->lsn)
-		return -1;
-	else if (a->lsn > b->lsn)
-		return 1;
-	return 0;
+	return pg_cmp_u64(a->lsn, b->lsn);
 }
 
 /*
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 2e6493aaaa..bfcd8fa13e 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -75,6 +75,7 @@
 #include <unistd.h>
 
 #include "access/xact.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "replication/syncrep.h"
@@ -698,12 +699,7 @@ cmp_lsn(const void *a, const void *b)
 	XLogRecPtr	lsn1 = *((const XLogRecPtr *) a);
 	XLogRecPtr	lsn2 = *((const XLogRecPtr *) b);
 
-	if (lsn1 > lsn2)
-		return -1;
-	else if (lsn1 == lsn2)
-		return 0;
-	else
-		return 1;
+	return pg_cmp_u64(lsn2, lsn1);
 }
 
 /*
diff --git a/src/backend/utils/adt/oid.c b/src/backend/utils/adt/oid.c
index 62bcfc5b56..56fb1fd77c 100644
--- a/src/backend/utils/adt/oid.c
+++ b/src/backend/utils/adt/oid.c
@@ -18,6 +18,7 @@
 #include <limits.h>
 
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
 #include "nodes/value.h"
@@ -259,11 +260,7 @@ oid_cmp(const void *p1, const void *p2)
 	Oid			v1 = *((const Oid *) p1);
 	Oid			v2 = *((const Oid *) p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_u32(v1, v2);
 }
 
 
diff --git a/src/backend/utils/adt/tsgistidx.c b/src/backend/utils/adt/tsgistidx.c
index a62b285365..225f0d49cb 100644
--- a/src/backend/utils/adt/tsgistidx.c
+++ b/src/backend/utils/adt/tsgistidx.c
@@ -17,6 +17,7 @@
 #include "access/gist.h"
 #include "access/heaptoast.h"
 #include "access/reloptions.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "port/pg_bitutils.h"
 #include "tsearch/ts_utils.h"
@@ -136,9 +137,7 @@ compareint(const void *va, const void *vb)
 	int32		a = *((const int32 *) va);
 	int32		b = *((const int32 *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return pg_cmp_s32(a,b);
 }
 
 static void
@@ -598,10 +597,7 @@ comparecost(const void *va, const void *vb)
 	const SPLITCOST *a = (const SPLITCOST *) va;
 	const SPLITCOST *b = (const SPLITCOST *) vb;
 
-	if (a->cost == b->cost)
-		return 0;
-	else
-		return (a->cost > b->cost) ? 1 : -1;
+	return pg_cmp_s32(a->cost, b->cost);
 }
 
 
diff --git a/src/backend/utils/adt/tsquery_gist.c b/src/backend/utils/adt/tsquery_gist.c
index f0b1c81c81..0b3355f7fe 100644
--- a/src/backend/utils/adt/tsquery_gist.c
+++ b/src/backend/utils/adt/tsquery_gist.c
@@ -16,6 +16,7 @@
 
 #include "access/gist.h"
 #include "access/stratnum.h"
+#include "common/int.h"
 #include "tsearch/ts_utils.h"
 #include "utils/builtins.h"
 
@@ -156,10 +157,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	if (((const SPLITCOST *) a)->cost == ((const SPLITCOST *) b)->cost)
-		return 0;
-	else
-		return (((const SPLITCOST *) a)->cost > ((const SPLITCOST *) b)->cost) ? 1 : -1;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 #define WISH_F(a,b,c) (double)( -(double)(((a)-(b))*((a)-(b))*((a)-(b)))*(c) )
diff --git a/src/backend/utils/adt/tsvector.c b/src/backend/utils/adt/tsvector.c
index fb7b7c712a..10bc4f2234 100644
--- a/src/backend/utils/adt/tsvector.c
+++ b/src/backend/utils/adt/tsvector.c
@@ -14,6 +14,7 @@
 
 #include "postgres.h"
 
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
 #include "tsearch/ts_locale.h"
@@ -37,9 +38,7 @@ compareWordEntryPos(const void *a, const void *b)
 	int			apos = WEP_GETPOS(*(const WordEntryPos *) a);
 	int			bpos = WEP_GETPOS(*(const WordEntryPos *) b);
 
-	if (apos == bpos)
-		return 0;
-	return (apos > bpos) ? 1 : -1;
+	return pg_cmp_s32(apos, bpos);
 }
 
 /*
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index 71386d0a1f..70bdc09dd7 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -19,6 +19,7 @@
 #include "catalog/namespace.h"
 #include "catalog/pg_type.h"
 #include "commands/trigger.h"
+#include "common/int.h"
 #include "executor/spi.h"
 #include "funcapi.h"
 #include "lib/qunique.h"
@@ -435,9 +436,7 @@ compare_int(const void *va, const void *vb)
 	int			a = *((const int *) va);
 	int			b = *((const int *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return pg_cmp_s32(a,b);
 }
 
 static int
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 63adf5668b..ae273b1961 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -19,6 +19,7 @@
 #include "access/multixact.h"
 #include "access/transam.h"
 #include "access/xact.h"
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "utils/builtins.h"
 #include "utils/xid8.h"
@@ -140,11 +141,7 @@ xidComparator(const void *arg1, const void *arg2)
 	TransactionId xid1 = *(const TransactionId *) arg1;
 	TransactionId xid2 = *(const TransactionId *) arg2;
 
-	if (xid1 > xid2)
-		return 1;
-	if (xid1 < xid2)
-		return -1;
-	return 0;
+	return pg_cmp_u32(xid1, xid2);
 }
 
 /*
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..50acae4529 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -69,6 +69,7 @@
 #include "commands/policy.h"
 #include "commands/publicationcmds.h"
 #include "commands/trigger.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
@@ -4520,7 +4521,7 @@ AttrDefaultCmp(const void *a, const void *b)
 	const AttrDefault *ada = (const AttrDefault *) a;
 	const AttrDefault *adb = (const AttrDefault *) b;
 
-	return ada->adnum - adb->adnum;
+	return pg_cmp_s16(ada->adnum, adb->adnum);
 }
 
 /*
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..a7c9c23399 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -29,6 +29,7 @@
 #include "catalog/pg_shdepend_d.h"
 #include "catalog/pg_shdescription_d.h"
 #include "catalog/pg_shseclabel_d.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "utils/catcache.h"
 #include "utils/lsyscache.h"
@@ -676,7 +677,5 @@ oid_compare(const void *a, const void *b)
 	Oid			oa = *((const Oid *) a);
 	Oid			ob = *((const Oid *) b);
 
-	if (oa == ob)
-		return 0;
-	return (oa > ob) ? 1 : -1;
+	return pg_cmp_u32(oa, ob);
 }
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 84fc83cb11..2842bde907 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -57,6 +57,7 @@
 #include "catalog/pg_range.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
+#include "common/int.h"
 #include "executor/executor.h"
 #include "lib/dshash.h"
 #include "optimizer/optimizer.h"
@@ -2722,12 +2723,7 @@ enum_oid_cmp(const void *left, const void *right)
 	const EnumItem *l = (const EnumItem *) left;
 	const EnumItem *r = (const EnumItem *) right;
 
-	if (l->enum_oid < r->enum_oid)
-		return -1;
-	else if (l->enum_oid > r->enum_oid)
-		return 1;
-	else
-		return 0;
+	return pg_cmp_u32(l->enum_oid, r->enum_oid);
 }
 
 /*
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index aa199b23ff..a75efd9adc 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -46,6 +46,7 @@
 #include "postgres.h"
 
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "storage/ipc.h"
 #include "storage/predicate.h"
 #include "storage/proc.h"
@@ -264,14 +265,7 @@ resource_priority_cmp(const void *a, const void *b)
 
 	/* Note: reverse order */
 	if (ra->kind->release_phase == rb->kind->release_phase)
-	{
-		if (ra->kind->release_priority == rb->kind->release_priority)
-			return 0;
-		else if (ra->kind->release_priority > rb->kind->release_priority)
-			return -1;
-		else
-			return 1;
-	}
+		return pg_cmp_u32(ra->kind->release_priority, rb->kind->release_priority);
 	else if (ra->kind->release_phase > rb->kind->release_phase)
 		return -1;
 	else
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index f358dd22b9..8ee8a42781 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -16,6 +16,7 @@
 #include "postgres_fe.h"
 
 #include "catalog/pg_class_d.h"
+#include "common/int.h"
 #include "lib/binaryheap.h"
 #include "pg_backup_archiver.h"
 #include "pg_backup_utils.h"
@@ -1504,9 +1505,5 @@ int_cmp(void *a, void *b, void *arg)
 	int			ai = (int) (intptr_t) a;
 	int			bi = (int) (intptr_t) b;
 
-	if (ai < bi)
-		return -1;
-	if (ai > bi)
-		return 1;
-	return 0;
+	return pg_cmp_s32(ai, bi);
 }
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index af998f74d3..3cbe2e5f1c 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -11,6 +11,7 @@
 
 #include "access/transam.h"
 #include "catalog/pg_language_d.h"
+#include "common/int.h"
 #include "pg_upgrade.h"
 
 /*
@@ -29,17 +30,17 @@ library_name_compare(const void *p1, const void *p2)
 {
 	const char *str1 = ((const LibraryInfo *) p1)->name;
 	const char *str2 = ((const LibraryInfo *) p2)->name;
-	int			slen1 = strlen(str1);
-	int			slen2 = strlen(str2);
+	size_t			slen1 = strlen(str1);
+	size_t			slen2 = strlen(str2);
 	int			cmp = strcmp(str1, str2);
+	int lhsdb = ((const LibraryInfo *) p1)->dbnum;
+	int rhsdb = ((const LibraryInfo *) p2)->dbnum;
 
 	if (slen1 != slen2)
-		return slen1 - slen2;
+		return pg_cmp_size(slen1, slen2);
 	if (cmp != 0)
 		return cmp;
-	else
-		return ((const LibraryInfo *) p1)->dbnum -
-			((const LibraryInfo *) p2)->dbnum;
+	return pg_cmp_s32(lhsdb, rhsdb);
 }
 
 
diff --git a/src/bin/pg_walsummary/pg_walsummary.c b/src/bin/pg_walsummary/pg_walsummary.c
index 1341c83c69..39f77d159b 100644
--- a/src/bin/pg_walsummary/pg_walsummary.c
+++ b/src/bin/pg_walsummary/pg_walsummary.c
@@ -17,6 +17,7 @@
 
 #include "common/blkreftable.h"
 #include "common/logging.h"
+#include "common/int.h"
 #include "fe_utils/option_utils.h"
 #include "lib/stringinfo.h"
 #include "getopt_long.h"
@@ -219,12 +220,7 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return pg_cmp_u32(aa, bb);
 }
 
 /*
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index c6116b7238..305ed4ab0a 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -8,6 +8,7 @@
 #include "postgres_fe.h"
 
 #include "common.h"
+#include "common/int.h"
 #include "common/logging.h"
 #include "crosstabview.h"
 #include "pqexpbuffer.h"
@@ -709,5 +710,5 @@ pivotFieldCompare(const void *a, const void *b)
 static int
 rankCompare(const void *a, const void *b)
 {
-	return *((const int *) a) - *((const int *) b);
+	return pg_cmp_s32(*(const int *) a, *(const int *) b);
 }
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 51d0c74a6b..3013a44bae 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -14,6 +14,7 @@
 #include "access/gin.h"
 #include "access/ginblock.h"
 #include "access/itup.h"
+#include "common/int.h"
 #include "catalog/pg_am_d.h"
 #include "fmgr.h"
 #include "lib/rbtree.h"
@@ -489,12 +490,7 @@ ginCompareItemPointers(ItemPointer a, ItemPointer b)
 	uint64		ia = (uint64) GinItemPointerGetBlockNumber(a) << 32 | GinItemPointerGetOffsetNumber(a);
 	uint64		ib = (uint64) GinItemPointerGetBlockNumber(b) << 32 | GinItemPointerGetOffsetNumber(b);
 
-	if (ia == ib)
-		return 0;
-	else if (ia > ib)
-		return 1;
-	else
-		return -1;
+	return pg_cmp_u64(ia, ib);
 }
 
 extern int	ginTraverseLock(Buffer buffer, bool searchMode);
-- 
2.34.1

0001-Add-integer-comparison-functions.patchtext/x-patch; charset=US-ASCII; name=0001-Add-integer-comparison-functions.patchDownload
From fc051295976bd79c44b9980044e335c424dc2170 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Fri, 9 Feb 2024 21:24:42 +0100
Subject: Add integer comparison functions

Introduce functions pg_cmp_xyz() that will standardize integer comparison
for implementing comparison function for qsort(). The functions will
returns negative, 0, or positive integer without risking overflow as a
result of subtracting the two integers, which is otherwise a common
pattern for implementing this.

For integer sizes less than sizeof(int) we can use normal subtraction,
which is faster, but for integers with size greater than or equal to
sizeof(int) we need to handle this differently.
---
 src/include/common/int.h | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/src/include/common/int.h b/src/include/common/int.h
index fedf7b3999..5aa856980a 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -438,4 +438,51 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
 #endif
 }
 
+/*------------------------------------------------------------------------
+ * Comparison routines for integers
+ *------------------------------------------------------------------------
+ */
+
+static inline int
+pg_cmp_s16(int16 a, int16 b)
+{
+	return (int32)a - (int32)b;
+}
+
+static inline int
+pg_cmp_u16(uint16 a, uint16 b)
+{
+	return (int32)a - (int32)b;
+}
+
+static inline int
+pg_cmp_s32(int32 a, int32 b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
+static inline int
+pg_cmp_u32(uint32 a, uint32 b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
+static inline int
+pg_cmp_s64(int64 a, int64 b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
+static inline int
+pg_cmp_u64(uint64 a, uint64 b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
+static inline int
+pg_cmp_size(size_t a, size_t b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
 #endif							/* COMMON_INT_H */
-- 
2.34.1

#47Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#46)
Re: glibc qsort() vulnerability

On Sat, Feb 10, 2024 at 08:59:06AM +0100, Mats Kindahl wrote:

Split the code into two patches: one that just adds the functions
(including the new pg_cmp_size()) to common/int.h and one that starts using
them. I picked the name "pg_cmp_size" rather than "pg_cmp_size_t" since
"_t" is usually used as a suffix for types.

I added a comment to the (a > b) - (a < b) return and have also added casts
to (int32) for the int16 and uint16 functions (we need a signed int for
uin16 since we need to be able to get a negative number).

Changed the type of two instances that had an implicit cast from size_t to
int and used the new pg_,cmp_size() function.

Also fixed the missed replacements in the "contrib" directory.

Thanks for the new patches. I think the comparison in resowner.c is
backwards, and I think we should expand on some of the commentary in int.h.
For example, the comment at the top of int.h seems very tailored to the
existing functions and should probably be adjusted. And the "comparison
routines for integers" comment might benefit from some additional details
about the purpose and guarantees of the new functions.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#48Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#47)
Re: glibc qsort() vulnerability

On Sat, Feb 10, 2024 at 9:53 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Sat, Feb 10, 2024 at 08:59:06AM +0100, Mats Kindahl wrote:

Split the code into two patches: one that just adds the functions
(including the new pg_cmp_size()) to common/int.h and one that starts

using

them. I picked the name "pg_cmp_size" rather than "pg_cmp_size_t" since
"_t" is usually used as a suffix for types.

I added a comment to the (a > b) - (a < b) return and have also added

casts

to (int32) for the int16 and uint16 functions (we need a signed int for
uin16 since we need to be able to get a negative number).

Changed the type of two instances that had an implicit cast from size_t

to

int and used the new pg_,cmp_size() function.

Also fixed the missed replacements in the "contrib" directory.

Thanks for the new patches. I think the comparison in resowner.c is
backwards,

Thanks for catching that.

and I think we should expand on some of the commentary in int.h.
For example, the comment at the top of int.h seems very tailored to the
existing functions and should probably be adjusted.

I rewrote the beginning to the following, does that look good?

* int.h
* Routines to perform signed and unsigned integer arithmetics, including
* comparisons, in an overflow-safe way.

And the "comparison
routines for integers" comment might benefit from some additional details
about the purpose and guarantees of the new functions.

I expanded that into the following. WDYT?

/*------------------------------------------------------------------------
* Comparison routines for integers.
*
* These routines are used to implement comparison functions for, e.g.,
* qsort(). They are designed to be efficient and not risk overflows in
* internal computations that could cause strange results, such as INT_MIN >
* INT_MAX if you just return "lhs - rhs".
*------------------------------------------------------------------------

Best wishes,
Mats Kindahl

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#49Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#48)
Re: glibc qsort() vulnerability

On Sun, Feb 11, 2024 at 03:44:42PM +0100, Mats Kindahl wrote:

On Sat, Feb 10, 2024 at 9:53 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

and I think we should expand on some of the commentary in int.h.
For example, the comment at the top of int.h seems very tailored to the
existing functions and should probably be adjusted.

I rewrote the beginning to the following, does that look good?

* int.h
* Routines to perform signed and unsigned integer arithmetics, including
* comparisons, in an overflow-safe way.

And the "comparison
routines for integers" comment might benefit from some additional details
about the purpose and guarantees of the new functions.

I expanded that into the following. WDYT?

/*------------------------------------------------------------------------
* Comparison routines for integers.
*
* These routines are used to implement comparison functions for, e.g.,
* qsort(). They are designed to be efficient and not risk overflows in
* internal computations that could cause strange results, such as INT_MIN >
* INT_MAX if you just return "lhs - rhs".
*------------------------------------------------------------------------

LGTM. I might editorialize a bit before committing, but I think your
proposed wording illustrates the thrust of the change.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#50Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#49)
2 attachment(s)
Re: glibc qsort() vulnerability

On Mon, Feb 12, 2024 at 4:57 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Sun, Feb 11, 2024 at 03:44:42PM +0100, Mats Kindahl wrote:

On Sat, Feb 10, 2024 at 9:53 PM Nathan Bossart <nathandbossart@gmail.com

wrote:

and I think we should expand on some of the commentary in int.h.
For example, the comment at the top of int.h seems very tailored to the
existing functions and should probably be adjusted.

I rewrote the beginning to the following, does that look good?

* int.h
* Routines to perform signed and unsigned integer arithmetics,

including

* comparisons, in an overflow-safe way.

And the "comparison
routines for integers" comment might benefit from some additional

details

about the purpose and guarantees of the new functions.

I expanded that into the following. WDYT?

/*------------------------------------------------------------------------

* Comparison routines for integers.
*
* These routines are used to implement comparison functions for, e.g.,
* qsort(). They are designed to be efficient and not risk overflows in
* internal computations that could cause strange results, such as

INT_MIN >

* INT_MAX if you just return "lhs - rhs".

*------------------------------------------------------------------------

LGTM. I might editorialize a bit before committing, but I think your
proposed wording illustrates the thrust of the change.

Thanks Nathan,

Here are the two fixed patches.

Best wishes,
Mats Kindahl

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

0002-Use-integer-comparison-functions.v2.patchtext/x-patch; charset=US-ASCII; name=0002-Use-integer-comparison-functions.v2.patchDownload
From b68f08b6afd62b75755e88f11a3ca31cfdce2645 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Fri, 9 Feb 2024 21:48:27 +0100
Subject: Use integer comparison functions

Use overflow-safe integer functions when implementing
qsort() comparison functions.
---
 contrib/hstore/hstore_gist.c                    |  3 ++-
 contrib/intarray/_intbig_gist.c                 |  3 ++-
 contrib/pg_stat_statements/pg_stat_statements.c |  8 ++------
 contrib/pg_trgm/trgm_op.c                       |  8 ++------
 src/backend/access/nbtree/nbtinsert.c           |  8 ++------
 src/backend/access/nbtree/nbtpage.c             | 10 +++-------
 src/backend/access/nbtree/nbtsplitloc.c         |  8 ++------
 src/backend/access/spgist/spgdoinsert.c         |  5 ++---
 src/backend/access/spgist/spgtextproc.c         |  3 ++-
 src/backend/backup/basebackup_incremental.c     |  8 ++------
 src/backend/backup/walsummary.c                 |  7 ++-----
 src/backend/catalog/heap.c                      |  8 ++------
 src/backend/nodes/list.c                        | 13 +++----------
 src/backend/nodes/tidbitmap.c                   |  7 ++-----
 src/backend/parser/parse_agg.c                  |  7 ++++---
 src/backend/postmaster/autovacuum.c             |  6 ++----
 src/backend/replication/logical/reorderbuffer.c |  7 ++-----
 src/backend/replication/syncrep.c               |  8 ++------
 src/backend/utils/adt/oid.c                     |  7 ++-----
 src/backend/utils/adt/tsgistidx.c               | 10 +++-------
 src/backend/utils/adt/tsquery_gist.c            |  6 ++----
 src/backend/utils/adt/tsvector.c                |  5 ++---
 src/backend/utils/adt/tsvector_op.c             |  5 ++---
 src/backend/utils/adt/xid.c                     |  7 ++-----
 src/backend/utils/cache/relcache.c              |  3 ++-
 src/backend/utils/cache/syscache.c              |  5 ++---
 src/backend/utils/cache/typcache.c              |  8 ++------
 src/backend/utils/resowner/resowner.c           | 10 ++--------
 src/bin/pg_dump/pg_dump_sort.c                  |  7 ++-----
 src/bin/pg_upgrade/function.c                   | 13 +++++++------
 src/bin/pg_walsummary/pg_walsummary.c           |  8 ++------
 src/bin/psql/crosstabview.c                     |  3 ++-
 src/include/access/gin_private.h                |  8 ++------
 33 files changed, 76 insertions(+), 156 deletions(-)

diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index fe343739eb..1079f25bf7 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -7,6 +7,7 @@
 #include "access/reloptions.h"
 #include "access/stratnum.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "hstore.h"
 #include "utils/pg_crc.h"
 
@@ -356,7 +357,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
index 8c6c4b5d05..aef9e13dcc 100644
--- a/contrib/intarray/_intbig_gist.c
+++ b/contrib/intarray/_intbig_gist.c
@@ -9,6 +9,7 @@
 #include "access/gist.h"
 #include "access/reloptions.h"
 #include "access/stratnum.h"
+#include "common/int.h"
 #include "port/pg_bitutils.h"
 
 #define GETENTRY(vec,pos) ((GISTTYPE *) DatumGetPointer((vec)->vector[(pos)].key))
@@ -312,7 +313,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost,  ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 8c6a3a2d08..67cec865ba 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -50,6 +50,7 @@
 #include "access/parallel.h"
 #include "catalog/pg_authid.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "executor/instrument.h"
 #include "funcapi.h"
 #include "jit/jit.h"
@@ -3007,10 +3008,5 @@ comp_location(const void *a, const void *b)
 	int			l = ((const LocationLen *) a)->location;
 	int			r = ((const LocationLen *) b)->location;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return +1;
-	else
-		return 0;
+	return pg_cmp_s32(l, r);
 }
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 49d4497b4f..c509d15ee4 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -6,6 +6,7 @@
 #include <ctype.h>
 
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "miscadmin.h"
 #include "trgm.h"
@@ -433,12 +434,7 @@ comp_ptrgm(const void *v1, const void *v2)
 	if (cmp != 0)
 		return cmp;
 
-	if (p1->index < p2->index)
-		return -1;
-	else if (p1->index == p2->index)
-		return 0;
-	else
-		return 1;
+	return pg_cmp_s32(p1->index, p2->index);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 709edd1c17..9cfe4cc094 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -20,6 +20,7 @@
 #include "access/transam.h"
 #include "access/xloginsert.h"
 #include "common/pg_prng.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
@@ -3013,10 +3014,5 @@ _bt_blk_cmp(const void *arg1, const void *arg2)
 	BlockNumber b1 = *((BlockNumber *) arg1);
 	BlockNumber b2 = *((BlockNumber *) arg2);
 
-	if (b1 < b2)
-		return -1;
-	else if (b1 > b2)
-		return 1;
-
-	return 0;
+	return pg_cmp_u32(b1, b2);
 }
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 567bade9f4..0f712cea57 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -28,6 +28,7 @@
 #include "access/transam.h"
 #include "access/xlog.h"
 #include "access/xloginsert.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "storage/indexfsm.h"
 #include "storage/lmgr.h"
@@ -1466,14 +1467,9 @@ _bt_delitems_cmp(const void *a, const void *b)
 	TM_IndexDelete *indexdelete1 = (TM_IndexDelete *) a;
 	TM_IndexDelete *indexdelete2 = (TM_IndexDelete *) b;
 
-	if (indexdelete1->id > indexdelete2->id)
-		return 1;
-	if (indexdelete1->id < indexdelete2->id)
-		return -1;
+	Assert(pg_cmp_s16(indexdelete1->id, indexdelete2->id) != 0);
 
-	Assert(false);
-
-	return 0;
+	return pg_cmp_s16(indexdelete1->id, indexdelete2->id);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtsplitloc.c b/src/backend/access/nbtree/nbtsplitloc.c
index d0b1d82578..0bcc8e90ae 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -15,6 +15,7 @@
 #include "postgres.h"
 
 #include "access/nbtree.h"
+#include "common/int.h"
 #include "storage/lmgr.h"
 
 typedef enum
@@ -596,12 +597,7 @@ _bt_splitcmp(const void *arg1, const void *arg2)
 	SplitPoint *split1 = (SplitPoint *) arg1;
 	SplitPoint *split2 = (SplitPoint *) arg2;
 
-	if (split1->curdelta > split2->curdelta)
-		return 1;
-	if (split1->curdelta < split2->curdelta)
-		return -1;
-
-	return 0;
+	return pg_cmp_s16(split1->curdelta,  split2->curdelta);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index bb063c858d..8695370139 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -20,6 +20,7 @@
 #include "access/spgxlog.h"
 #include "access/xloginsert.h"
 #include "common/pg_prng.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
 #include "utils/rel.h"
@@ -110,9 +111,7 @@ addNode(SpGistState *state, SpGistInnerTuple tuple, Datum label, int offset)
 static int
 cmpOffsetNumbers(const void *a, const void *b)
 {
-	if (*(const OffsetNumber *) a == *(const OffsetNumber *) b)
-		return 0;
-	return (*(const OffsetNumber *) a > *(const OffsetNumber *) b) ? 1 : -1;
+	return pg_cmp_u16(*(const OffsetNumber *) a, *(const OffsetNumber *) b);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..d5db5225a9 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -40,6 +40,7 @@
 #include "postgres.h"
 
 #include "access/spgist.h"
+#include "common/int.h"
 #include "catalog/pg_type.h"
 #include "mb/pg_wchar.h"
 #include "utils/builtins.h"
@@ -325,7 +326,7 @@ cmpNodePtr(const void *a, const void *b)
 	const spgNodePtr *aa = (const spgNodePtr *) a;
 	const spgNodePtr *bb = (const spgNodePtr *) b;
 
-	return aa->c - bb->c;
+	return pg_cmp_s16(aa->c, bb->c);
 }
 
 Datum
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 0504c465db..d08dfb740b 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -27,6 +27,7 @@
 #include "common/blkreftable.h"
 #include "common/parse_manifest.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "postmaster/walsummarizer.h"
 
 #define	BLOCKS_PER_READ			512
@@ -994,10 +995,5 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return pg_cmp_u32(aa,bb);
 }
diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 867870aaad..4d047e1c02 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -17,6 +17,7 @@
 
 #include "access/xlog_internal.h"
 #include "backup/walsummary.h"
+#include "common/int.h"
 #include "utils/wait_event.h"
 
 static bool IsWalSummaryFilename(char *filename);
@@ -355,9 +356,5 @@ ListComparatorForWalSummaryFiles(const ListCell *a, const ListCell *b)
 	WalSummaryFile *ws1 = lfirst(a);
 	WalSummaryFile *ws2 = lfirst(b);
 
-	if (ws1->start_lsn < ws2->start_lsn)
-		return -1;
-	if (ws1->start_lsn > ws2->start_lsn)
-		return 1;
-	return 0;
+	return pg_cmp_u64(ws1->start_lsn, ws2->start_lsn);
 }
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c73f7bcd01..f8145d4cf5 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -56,6 +56,7 @@
 #include "catalog/storage.h"
 #include "commands/tablecmds.h"
 #include "commands/typecmds.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -2758,12 +2759,7 @@ list_cookedconstr_attnum_cmp(const ListCell *p1, const ListCell *p2)
 {
 	AttrNumber	v1 = ((CookedConstraint *) lfirst(p1))->attnum;
 	AttrNumber	v2 = ((CookedConstraint *) lfirst(p2))->attnum;
-
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_s16(v1, v2);
 }
 
 /*
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 957186bef5..e2615ab105 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -17,6 +17,7 @@
  */
 #include "postgres.h"
 
+#include "common/int.h"
 #include "nodes/pg_list.h"
 #include "port/pg_bitutils.h"
 #include "utils/memdebug.h"
@@ -1692,11 +1693,7 @@ list_int_cmp(const ListCell *p1, const ListCell *p2)
 	int			v1 = lfirst_int(p1);
 	int			v2 = lfirst_int(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_s32(v1, v2);
 }
 
 /*
@@ -1708,9 +1705,5 @@ list_oid_cmp(const ListCell *p1, const ListCell *p2)
 	Oid			v1 = lfirst_oid(p1);
 	Oid			v2 = lfirst_oid(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_u32(v1, v2);
 }
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index 0f4850065f..79e54ba111 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -42,6 +42,7 @@
 
 #include "access/htup_details.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "nodes/bitmapset.h"
 #include "nodes/tidbitmap.h"
 #include "storage/lwlock.h"
@@ -1425,11 +1426,7 @@ tbm_comparator(const void *left, const void *right)
 	BlockNumber l = (*((PagetableEntry *const *) left))->blockno;
 	BlockNumber r = (*((PagetableEntry *const *) right))->blockno;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return 1;
-	return 0;
+	return pg_cmp_u32(l,r);
 }
 
 /*
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 7b211a7743..a50d799f03 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -18,6 +18,7 @@
 #include "catalog/pg_aggregate.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -1757,10 +1758,10 @@ expand_groupingset_node(GroupingSet *gs)
 static int
 cmp_list_len_asc(const ListCell *a, const ListCell *b)
 {
-	int			la = list_length((const List *) lfirst(a));
-	int			lb = list_length((const List *) lfirst(b));
+	size_t		la = list_length((const List *) lfirst(a));
+	size_t		lb = list_length((const List *) lfirst(b));
 
-	return (la > lb) ? 1 : (la == lb) ? 0 : -1;
+	return pg_cmp_size(la, lb);
 }
 
 /* list_sort comparator to sort sub-lists by length and contents */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2c3099f76f..483244254f 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -78,6 +78,7 @@
 #include "catalog/pg_database.h"
 #include "commands/dbcommands.h"
 #include "commands/vacuum.h"
+#include "common/int.h"
 #include "lib/ilist.h"
 #include "libpq/pqsignal.h"
 #include "miscadmin.h"
@@ -1119,10 +1120,7 @@ rebuild_database_list(Oid newdb)
 static int
 db_comparator(const void *a, const void *b)
 {
-	if (((const avl_dbase *) a)->adl_score == ((const avl_dbase *) b)->adl_score)
-		return 0;
-	else
-		return (((const avl_dbase *) a)->adl_score < ((const avl_dbase *) b)->adl_score) ? 1 : -1;
+	return pg_cmp_s32(((const avl_dbase *) a)->adl_score, ((const avl_dbase *) b)->adl_score);
 }
 
 /*
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index bbf0966182..5446df3c64 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -91,6 +91,7 @@
 #include "access/xact.h"
 #include "access/xlog_internal.h"
 #include "catalog/catalog.h"
+#include "common/int.h"
 #include "lib/binaryheap.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -5119,11 +5120,7 @@ file_sort_by_lsn(const ListCell *a_p, const ListCell *b_p)
 	RewriteMappingFile *a = (RewriteMappingFile *) lfirst(a_p);
 	RewriteMappingFile *b = (RewriteMappingFile *) lfirst(b_p);
 
-	if (a->lsn < b->lsn)
-		return -1;
-	else if (a->lsn > b->lsn)
-		return 1;
-	return 0;
+	return pg_cmp_u64(a->lsn, b->lsn);
 }
 
 /*
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 2e6493aaaa..bfcd8fa13e 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -75,6 +75,7 @@
 #include <unistd.h>
 
 #include "access/xact.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "replication/syncrep.h"
@@ -698,12 +699,7 @@ cmp_lsn(const void *a, const void *b)
 	XLogRecPtr	lsn1 = *((const XLogRecPtr *) a);
 	XLogRecPtr	lsn2 = *((const XLogRecPtr *) b);
 
-	if (lsn1 > lsn2)
-		return -1;
-	else if (lsn1 == lsn2)
-		return 0;
-	else
-		return 1;
+	return pg_cmp_u64(lsn2, lsn1);
 }
 
 /*
diff --git a/src/backend/utils/adt/oid.c b/src/backend/utils/adt/oid.c
index 62bcfc5b56..56fb1fd77c 100644
--- a/src/backend/utils/adt/oid.c
+++ b/src/backend/utils/adt/oid.c
@@ -18,6 +18,7 @@
 #include <limits.h>
 
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
 #include "nodes/value.h"
@@ -259,11 +260,7 @@ oid_cmp(const void *p1, const void *p2)
 	Oid			v1 = *((const Oid *) p1);
 	Oid			v2 = *((const Oid *) p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_u32(v1, v2);
 }
 
 
diff --git a/src/backend/utils/adt/tsgistidx.c b/src/backend/utils/adt/tsgistidx.c
index a62b285365..225f0d49cb 100644
--- a/src/backend/utils/adt/tsgistidx.c
+++ b/src/backend/utils/adt/tsgistidx.c
@@ -17,6 +17,7 @@
 #include "access/gist.h"
 #include "access/heaptoast.h"
 #include "access/reloptions.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "port/pg_bitutils.h"
 #include "tsearch/ts_utils.h"
@@ -136,9 +137,7 @@ compareint(const void *va, const void *vb)
 	int32		a = *((const int32 *) va);
 	int32		b = *((const int32 *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return pg_cmp_s32(a,b);
 }
 
 static void
@@ -598,10 +597,7 @@ comparecost(const void *va, const void *vb)
 	const SPLITCOST *a = (const SPLITCOST *) va;
 	const SPLITCOST *b = (const SPLITCOST *) vb;
 
-	if (a->cost == b->cost)
-		return 0;
-	else
-		return (a->cost > b->cost) ? 1 : -1;
+	return pg_cmp_s32(a->cost, b->cost);
 }
 
 
diff --git a/src/backend/utils/adt/tsquery_gist.c b/src/backend/utils/adt/tsquery_gist.c
index f0b1c81c81..0b3355f7fe 100644
--- a/src/backend/utils/adt/tsquery_gist.c
+++ b/src/backend/utils/adt/tsquery_gist.c
@@ -16,6 +16,7 @@
 
 #include "access/gist.h"
 #include "access/stratnum.h"
+#include "common/int.h"
 #include "tsearch/ts_utils.h"
 #include "utils/builtins.h"
 
@@ -156,10 +157,7 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	if (((const SPLITCOST *) a)->cost == ((const SPLITCOST *) b)->cost)
-		return 0;
-	else
-		return (((const SPLITCOST *) a)->cost > ((const SPLITCOST *) b)->cost) ? 1 : -1;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost, ((const SPLITCOST *) b)->cost);
 }
 
 #define WISH_F(a,b,c) (double)( -(double)(((a)-(b))*((a)-(b))*((a)-(b)))*(c) )
diff --git a/src/backend/utils/adt/tsvector.c b/src/backend/utils/adt/tsvector.c
index fb7b7c712a..10bc4f2234 100644
--- a/src/backend/utils/adt/tsvector.c
+++ b/src/backend/utils/adt/tsvector.c
@@ -14,6 +14,7 @@
 
 #include "postgres.h"
 
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
 #include "tsearch/ts_locale.h"
@@ -37,9 +38,7 @@ compareWordEntryPos(const void *a, const void *b)
 	int			apos = WEP_GETPOS(*(const WordEntryPos *) a);
 	int			bpos = WEP_GETPOS(*(const WordEntryPos *) b);
 
-	if (apos == bpos)
-		return 0;
-	return (apos > bpos) ? 1 : -1;
+	return pg_cmp_s32(apos, bpos);
 }
 
 /*
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index 71386d0a1f..70bdc09dd7 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -19,6 +19,7 @@
 #include "catalog/namespace.h"
 #include "catalog/pg_type.h"
 #include "commands/trigger.h"
+#include "common/int.h"
 #include "executor/spi.h"
 #include "funcapi.h"
 #include "lib/qunique.h"
@@ -435,9 +436,7 @@ compare_int(const void *va, const void *vb)
 	int			a = *((const int *) va);
 	int			b = *((const int *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return pg_cmp_s32(a,b);
 }
 
 static int
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 63adf5668b..ae273b1961 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -19,6 +19,7 @@
 #include "access/multixact.h"
 #include "access/transam.h"
 #include "access/xact.h"
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "utils/builtins.h"
 #include "utils/xid8.h"
@@ -140,11 +141,7 @@ xidComparator(const void *arg1, const void *arg2)
 	TransactionId xid1 = *(const TransactionId *) arg1;
 	TransactionId xid2 = *(const TransactionId *) arg2;
 
-	if (xid1 > xid2)
-		return 1;
-	if (xid1 < xid2)
-		return -1;
-	return 0;
+	return pg_cmp_u32(xid1, xid2);
 }
 
 /*
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..50acae4529 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -69,6 +69,7 @@
 #include "commands/policy.h"
 #include "commands/publicationcmds.h"
 #include "commands/trigger.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
@@ -4520,7 +4521,7 @@ AttrDefaultCmp(const void *a, const void *b)
 	const AttrDefault *ada = (const AttrDefault *) a;
 	const AttrDefault *adb = (const AttrDefault *) b;
 
-	return ada->adnum - adb->adnum;
+	return pg_cmp_s16(ada->adnum, adb->adnum);
 }
 
 /*
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..a7c9c23399 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -29,6 +29,7 @@
 #include "catalog/pg_shdepend_d.h"
 #include "catalog/pg_shdescription_d.h"
 #include "catalog/pg_shseclabel_d.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "utils/catcache.h"
 #include "utils/lsyscache.h"
@@ -676,7 +677,5 @@ oid_compare(const void *a, const void *b)
 	Oid			oa = *((const Oid *) a);
 	Oid			ob = *((const Oid *) b);
 
-	if (oa == ob)
-		return 0;
-	return (oa > ob) ? 1 : -1;
+	return pg_cmp_u32(oa, ob);
 }
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 84fc83cb11..2842bde907 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -57,6 +57,7 @@
 #include "catalog/pg_range.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
+#include "common/int.h"
 #include "executor/executor.h"
 #include "lib/dshash.h"
 #include "optimizer/optimizer.h"
@@ -2722,12 +2723,7 @@ enum_oid_cmp(const void *left, const void *right)
 	const EnumItem *l = (const EnumItem *) left;
 	const EnumItem *r = (const EnumItem *) right;
 
-	if (l->enum_oid < r->enum_oid)
-		return -1;
-	else if (l->enum_oid > r->enum_oid)
-		return 1;
-	else
-		return 0;
+	return pg_cmp_u32(l->enum_oid, r->enum_oid);
 }
 
 /*
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index aa199b23ff..ab9343bc5c 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -46,6 +46,7 @@
 #include "postgres.h"
 
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "storage/ipc.h"
 #include "storage/predicate.h"
 #include "storage/proc.h"
@@ -264,14 +265,7 @@ resource_priority_cmp(const void *a, const void *b)
 
 	/* Note: reverse order */
 	if (ra->kind->release_phase == rb->kind->release_phase)
-	{
-		if (ra->kind->release_priority == rb->kind->release_priority)
-			return 0;
-		else if (ra->kind->release_priority > rb->kind->release_priority)
-			return -1;
-		else
-			return 1;
-	}
+		return pg_cmp_u32(rb->kind->release_priority, ra->kind->release_priority);
 	else if (ra->kind->release_phase > rb->kind->release_phase)
 		return -1;
 	else
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index f358dd22b9..8ee8a42781 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -16,6 +16,7 @@
 #include "postgres_fe.h"
 
 #include "catalog/pg_class_d.h"
+#include "common/int.h"
 #include "lib/binaryheap.h"
 #include "pg_backup_archiver.h"
 #include "pg_backup_utils.h"
@@ -1504,9 +1505,5 @@ int_cmp(void *a, void *b, void *arg)
 	int			ai = (int) (intptr_t) a;
 	int			bi = (int) (intptr_t) b;
 
-	if (ai < bi)
-		return -1;
-	if (ai > bi)
-		return 1;
-	return 0;
+	return pg_cmp_s32(ai, bi);
 }
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index af998f74d3..3cbe2e5f1c 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -11,6 +11,7 @@
 
 #include "access/transam.h"
 #include "catalog/pg_language_d.h"
+#include "common/int.h"
 #include "pg_upgrade.h"
 
 /*
@@ -29,17 +30,17 @@ library_name_compare(const void *p1, const void *p2)
 {
 	const char *str1 = ((const LibraryInfo *) p1)->name;
 	const char *str2 = ((const LibraryInfo *) p2)->name;
-	int			slen1 = strlen(str1);
-	int			slen2 = strlen(str2);
+	size_t			slen1 = strlen(str1);
+	size_t			slen2 = strlen(str2);
 	int			cmp = strcmp(str1, str2);
+	int lhsdb = ((const LibraryInfo *) p1)->dbnum;
+	int rhsdb = ((const LibraryInfo *) p2)->dbnum;
 
 	if (slen1 != slen2)
-		return slen1 - slen2;
+		return pg_cmp_size(slen1, slen2);
 	if (cmp != 0)
 		return cmp;
-	else
-		return ((const LibraryInfo *) p1)->dbnum -
-			((const LibraryInfo *) p2)->dbnum;
+	return pg_cmp_s32(lhsdb, rhsdb);
 }
 
 
diff --git a/src/bin/pg_walsummary/pg_walsummary.c b/src/bin/pg_walsummary/pg_walsummary.c
index 1341c83c69..39f77d159b 100644
--- a/src/bin/pg_walsummary/pg_walsummary.c
+++ b/src/bin/pg_walsummary/pg_walsummary.c
@@ -17,6 +17,7 @@
 
 #include "common/blkreftable.h"
 #include "common/logging.h"
+#include "common/int.h"
 #include "fe_utils/option_utils.h"
 #include "lib/stringinfo.h"
 #include "getopt_long.h"
@@ -219,12 +220,7 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return pg_cmp_u32(aa, bb);
 }
 
 /*
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index c6116b7238..305ed4ab0a 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -8,6 +8,7 @@
 #include "postgres_fe.h"
 
 #include "common.h"
+#include "common/int.h"
 #include "common/logging.h"
 #include "crosstabview.h"
 #include "pqexpbuffer.h"
@@ -709,5 +710,5 @@ pivotFieldCompare(const void *a, const void *b)
 static int
 rankCompare(const void *a, const void *b)
 {
-	return *((const int *) a) - *((const int *) b);
+	return pg_cmp_s32(*(const int *) a, *(const int *) b);
 }
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 51d0c74a6b..3013a44bae 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -14,6 +14,7 @@
 #include "access/gin.h"
 #include "access/ginblock.h"
 #include "access/itup.h"
+#include "common/int.h"
 #include "catalog/pg_am_d.h"
 #include "fmgr.h"
 #include "lib/rbtree.h"
@@ -489,12 +490,7 @@ ginCompareItemPointers(ItemPointer a, ItemPointer b)
 	uint64		ia = (uint64) GinItemPointerGetBlockNumber(a) << 32 | GinItemPointerGetOffsetNumber(a);
 	uint64		ib = (uint64) GinItemPointerGetBlockNumber(b) << 32 | GinItemPointerGetOffsetNumber(b);
 
-	if (ia == ib)
-		return 0;
-	else if (ia > ib)
-		return 1;
-	else
-		return -1;
+	return pg_cmp_u64(ia, ib);
 }
 
 extern int	ginTraverseLock(Buffer buffer, bool searchMode);
-- 
2.34.1

0001-Add-integer-comparison-functions.v2.patchtext/x-patch; charset=US-ASCII; name=0001-Add-integer-comparison-functions.v2.patchDownload
From c7e912f36f193b299a42c6e98dc31a78d9957395 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Fri, 9 Feb 2024 21:24:42 +0100
Subject: Add integer comparison functions

Introduce functions pg_cmp_xyz() that will standardize integer comparison
for implementing comparison function for qsort(). The functions will
returns negative, 0, or positive integer without risking overflow as a
result of subtracting the two integers, which is otherwise a common
pattern for implementing this.

For integer sizes less than sizeof(int) we can use normal subtraction,
which is faster, but for integers with size greater than or equal to
sizeof(int) we need to handle this differently.
---
 src/include/common/int.h | 55 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/include/common/int.h b/src/include/common/int.h
index fedf7b3999..03afc04b2e 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -1,7 +1,8 @@
 /*-------------------------------------------------------------------------
  *
  * int.h
- *	  Routines to perform integer math, while checking for overflows.
+ *	  Routines to perform signed and unsigned integer arithmetics, including
+ *	  comparisons, in an overflow-safe way.
  *
  * The routines in this file are intended to be well defined C, without
  * relying on compiler flags like -fwrapv.
@@ -438,4 +439,56 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
 #endif
 }
 
+/*------------------------------------------------------------------------
+ * Comparison routines for integers.
+ *
+ * These routines are used to implement comparison functions for, e.g.,
+ * qsort(). They are designed to be efficient and not risk overflows in
+ * internal computations that could cause strange results, such as INT_MIN >
+ * INT_MAX if you just return "lhs - rhs".
+ *------------------------------------------------------------------------
+ */
+
+static inline int
+pg_cmp_s16(int16 a, int16 b)
+{
+	return (int32)a - (int32)b;
+}
+
+static inline int
+pg_cmp_u16(uint16 a, uint16 b)
+{
+	return (int32)a - (int32)b;
+}
+
+static inline int
+pg_cmp_s32(int32 a, int32 b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
+static inline int
+pg_cmp_u32(uint32 a, uint32 b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
+static inline int
+pg_cmp_s64(int64 a, int64 b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
+static inline int
+pg_cmp_u64(uint64 a, uint64 b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
+static inline int
+pg_cmp_size(size_t a, size_t b)
+{
+	return (a > b) - (a < b);	/* Comparison operators return int 0 or 1 */
+}
+
 #endif							/* COMMON_INT_H */
-- 
2.34.1

#51Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#50)
Re: glibc qsort() vulnerability

On Mon, Feb 12, 2024 at 06:09:06PM +0100, Mats Kindahl wrote:

Here are the two fixed patches.

These patches look pretty good to me. Barring additional feedback, I'll
plan on committing them in the next few days.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#52Fabrízio de Royes Mello
fabriziomello@gmail.com
In reply to: Nathan Bossart (#51)
Re: glibc qsort() vulnerability

On Mon, Feb 12, 2024 at 5:51 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Mon, Feb 12, 2024 at 06:09:06PM +0100, Mats Kindahl wrote:

Here are the two fixed patches.

These patches look pretty good to me. Barring additional feedback, I'll
plan on committing them in the next few days.

Also did some tests locally and everything went well. Patches apply to the
main branch without issues and all regression (including checkworld) pass!!

Regards
--
Fabrízio de Royes Mello

#53Andres Freund
andres@anarazel.de
In reply to: Nathan Bossart (#51)
Re: glibc qsort() vulnerability

Hi,

On 2024-02-12 14:51:38 -0600, Nathan Bossart wrote:

On Mon, Feb 12, 2024 at 06:09:06PM +0100, Mats Kindahl wrote:

Here are the two fixed patches.

These patches look pretty good to me. Barring additional feedback, I'll
plan on committing them in the next few days.

One thing that's worth checking is if this ends up with *worse* code when the
comparators are inlined. I think none of the changed comparators will end up
getting used with an inlined sort, but ...

The reason we could end up with worse code is that when inlining the
comparisons would make less sense for the compiler. Consider e.g.
return DO_COMPARE(a, b) < 0 ?
(DO_COMPARE(b, c) < 0 ? b : (DO_COMPARE(a, c) < 0 ? c : a))
: (DO_COMPARE(b, c) > 0 ? b : (DO_COMPARE(a, c) < 0 ? a : c));

With a naive implementation the compiler will understand it only cares about
a < b, not about the other possibilities. I'm not sure that's still true with
the more complicated optimized version.

Greetings,

Andres Freund

#54Nathan Bossart
nathandbossart@gmail.com
In reply to: Andres Freund (#53)
Re: glibc qsort() vulnerability

On Mon, Feb 12, 2024 at 01:31:30PM -0800, Andres Freund wrote:

One thing that's worth checking is if this ends up with *worse* code when the
comparators are inlined. I think none of the changed comparators will end up
getting used with an inlined sort, but ...

Yeah, AFAICT the only inlined sorts are in tuplesort.c and bufmgr.c, and
the patches don't touch those files.

The reason we could end up with worse code is that when inlining the
comparisons would make less sense for the compiler. Consider e.g.
return DO_COMPARE(a, b) < 0 ?
(DO_COMPARE(b, c) < 0 ? b : (DO_COMPARE(a, c) < 0 ? c : a))
: (DO_COMPARE(b, c) > 0 ? b : (DO_COMPARE(a, c) < 0 ? a : c));

With a naive implementation the compiler will understand it only cares about
a < b, not about the other possibilities. I'm not sure that's still true with
the more complicated optimized version.

You aren't kidding [0]https://godbolt.org/z/bbTqK54zK. Besides perhaps adding a comment in
sort_template.h, is there anything else you think we should do about this
now?

[0]: https://godbolt.org/z/bbTqK54zK

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#55Andres Freund
andres@anarazel.de
In reply to: Nathan Bossart (#54)
Re: glibc qsort() vulnerability

Hi,

On 2024-02-12 17:04:23 -0600, Nathan Bossart wrote:

On Mon, Feb 12, 2024 at 01:31:30PM -0800, Andres Freund wrote:

One thing that's worth checking is if this ends up with *worse* code when the
comparators are inlined. I think none of the changed comparators will end up
getting used with an inlined sort, but ...

Yeah, AFAICT the only inlined sorts are in tuplesort.c and bufmgr.c, and
the patches don't touch those files.

The reason we could end up with worse code is that when inlining the
comparisons would make less sense for the compiler. Consider e.g.
return DO_COMPARE(a, b) < 0 ?
(DO_COMPARE(b, c) < 0 ? b : (DO_COMPARE(a, c) < 0 ? c : a))
: (DO_COMPARE(b, c) > 0 ? b : (DO_COMPARE(a, c) < 0 ? a : c));

With a naive implementation the compiler will understand it only cares about
a < b, not about the other possibilities. I'm not sure that's still true with
the more complicated optimized version.

You aren't kidding [0]. Besides perhaps adding a comment in
sort_template.h, is there anything else you think we should do about this
now?

I'd add also a comment to the new functions. I think it's fine otherwise. I
wish there were formulation that'd be optimal for both cases, but this way we
can at least adapt all places at once if either find a better formulation or
change all our sorts to happen via an inline implementation of qsort or such.

Greetings,

Andres

#56Mats Kindahl
mats@timescale.com
In reply to: Andres Freund (#55)
Re: glibc qsort() vulnerability

On Tue, Feb 13, 2024 at 12:41 AM Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2024-02-12 17:04:23 -0600, Nathan Bossart wrote:

On Mon, Feb 12, 2024 at 01:31:30PM -0800, Andres Freund wrote:

One thing that's worth checking is if this ends up with *worse* code

when the

comparators are inlined. I think none of the changed comparators will

end up

getting used with an inlined sort, but ...

Yeah, AFAICT the only inlined sorts are in tuplesort.c and bufmgr.c, and
the patches don't touch those files.

The reason we could end up with worse code is that when inlining the
comparisons would make less sense for the compiler. Consider e.g.
return DO_COMPARE(a, b) < 0 ?
(DO_COMPARE(b, c) < 0 ? b : (DO_COMPARE(a, c) < 0 ? c : a))
: (DO_COMPARE(b, c) > 0 ? b : (DO_COMPARE(a, c) < 0 ? a :

c));

With a naive implementation the compiler will understand it only cares

about

a < b, not about the other possibilities. I'm not sure that's still

true with

the more complicated optimized version.

You aren't kidding [0]. Besides perhaps adding a comment in
sort_template.h, is there anything else you think we should do about this
now?

I'd add also a comment to the new functions. I think it's fine otherwise. I
wish there were formulation that'd be optimal for both cases, but this way
we
can at least adapt all places at once if either find a better formulation
or
change all our sorts to happen via an inline implementation of qsort or
such.

I suspect that this has to do with the fact that we "abuse" the type system
by performing arithmetics on booleans converted to integers and the
compilers do not have rules for dealing with these.

For example, with the inline function "static inline cmp(a,b) { return a <
b ? -1 : a > b ? 1 : 0; }"

Which trivially can be rewritten by the compiler using very basic rules, as
follows:

DO_COMPARE(a,b)
(a < b ? -1 : a > b ? 1 : 0) < 0
a < b ? (-1 < 0) : a > b ? (1 < 0) : (0 < 0)
a < b ? true : a > b ? false : false
a < b ? true : a > b ? false : false
a < b ? true : false
a < b

When it comes to (a < b) - (a > b) there are no such rules added since it
is not a very common case.

Clang fares better for this case and at least shows the two alternatives as
equal.

Maybe we should change to use the original version equivalent to the inline
function above since that works better with surrounding code?

Best wishes,
Mats Kindahl

Show quoted text

Greetings,

Andres

#57Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#56)
Re: glibc qsort() vulnerability

On Tue, Feb 13, 2024 at 09:43:18AM +0100, Mats Kindahl wrote:

Maybe we should change to use the original version equivalent to the inline
function above since that works better with surrounding code?

I don't think that's necessary. We just need to be cognizant of it when
using inlined sorts, which are pretty rare at the moment. Your patches
should still be a net improvement in many cases because most qsorts use a
function pointer to the comparator.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#58Nathan Bossart
nathandbossart@gmail.com
In reply to: Nathan Bossart (#57)
3 attachment(s)
Re: glibc qsort() vulnerability

Here is what I have staged for commit.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

v8-0001-Remove-direct-calls-to-pg_qsort.patchtext/x-diff; charset=us-asciiDownload
From 8e5a66fb3a0787f15a900a89742862c89da38a1d Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 15 Feb 2024 10:53:11 -0600
Subject: [PATCH v8 1/3] Remove direct calls to pg_qsort().

Calls to this function might give the idea that pg_qsort() is
somehow different than qsort(), when in fact all calls to qsort()
in the Postgres tree are redirected to pg_qsort() via a macro.
This commit removes all direct calls to pg_qsort() in favor of
letting the macro handle the conversions.

Discussion: https://postgr.es/m/CA%2B14426g2Wa9QuUpmakwPxXFWG_1FaY0AsApkvcTBy-YfS6uaw%40mail.gmail.com
---
 contrib/pg_prewarm/autoprewarm.c            |  4 ++--
 src/backend/access/brin/brin_minmax_multi.c |  2 +-
 src/backend/optimizer/plan/analyzejoins.c   |  4 ++--
 src/backend/storage/buffer/bufmgr.c         |  4 ++--
 src/backend/utils/cache/syscache.c          | 10 +++++-----
 src/include/port.h                          |  3 +++
 src/port/qsort.c                            |  3 +++
 7 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 06ee21d496..248b9914a3 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -346,8 +346,8 @@ apw_load_buffers(void)
 	FreeFile(file);
 
 	/* Sort the blocks to be loaded. */
-	pg_qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
-			 apw_compare_blockinfo);
+	qsort(blkinfo, num_elements, sizeof(BlockInfoRecord),
+		  apw_compare_blockinfo);
 
 	/* Populate shared memory state. */
 	apw_state->block_info_handle = dsm_segment_handle(seg);
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 3ffaad3e42..2c29aa3d4e 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -1369,7 +1369,7 @@ build_distances(FmgrInfo *distanceFn, Oid colloid,
 	 * Sort the distances in descending order, so that the longest gaps are at
 	 * the front.
 	 */
-	pg_qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
+	qsort(distances, ndistances, sizeof(DistanceValue), compare_distances);
 
 	return distances;
 }
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 7dcb74572a..e494acd51a 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -2307,8 +2307,8 @@ remove_self_joins_recurse(PlannerInfo *root, List *joinlist, Relids toRemove)
 		j++;
 	}
 
-	pg_qsort(candidates, numRels, sizeof(SelfJoinCandidate),
-			 self_join_candidates_cmp);
+	qsort(candidates, numRels, sizeof(SelfJoinCandidate),
+		  self_join_candidates_cmp);
 
 	/*
 	 * Iteratively form a group of relation indexes with the same oid and
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index eb1ec3b86d..07575ef312 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3915,7 +3915,7 @@ DropRelationsAllBuffers(SMgrRelation *smgr_reln, int nlocators)
 
 	/* sort the list of rlocators if necessary */
 	if (use_bsearch)
-		pg_qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
+		qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
@@ -4269,7 +4269,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels)
 
 	/* sort the list of SMgrRelations if necessary */
 	if (use_bsearch)
-		pg_qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
+		qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator);
 
 	for (i = 0; i < NBuffers; i++)
 	{
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 162855b158..662f930864 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -146,14 +146,14 @@ InitCatalogCache(void)
 	Assert(SysCacheSupportingRelOidSize <= lengthof(SysCacheSupportingRelOid));
 
 	/* Sort and de-dup OID arrays, so we can use binary search. */
-	pg_qsort(SysCacheRelationOid, SysCacheRelationOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheRelationOid, SysCacheRelationOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheRelationOidSize =
 		qunique(SysCacheRelationOid, SysCacheRelationOidSize, sizeof(Oid),
 				oid_compare);
 
-	pg_qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
-			 sizeof(Oid), oid_compare);
+	qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
+		  sizeof(Oid), oid_compare);
 	SysCacheSupportingRelOidSize =
 		qunique(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize,
 				sizeof(Oid), oid_compare);
@@ -668,7 +668,7 @@ RelationSupportsSysCache(Oid relid)
 
 
 /*
- * OID comparator for pg_qsort
+ * OID comparator for qsort
  */
 static int
 oid_compare(const void *a, const void *b)
diff --git a/src/include/port.h b/src/include/port.h
index 263cf791a3..a2cebbbd68 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -438,6 +438,9 @@ extern bool pg_get_user_name(uid_t user_id, char *buffer, size_t buflen);
 extern bool pg_get_user_home_dir(uid_t user_id, char *buffer, size_t buflen);
 #endif
 
+/*
+ * NB: Callers should use qsort() instead of calling pg_qsort() directly.
+ */
 extern void pg_qsort(void *base, size_t nel, size_t elsize,
 					 int (*cmp) (const void *, const void *));
 extern int	pg_qsort_strcmp(const void *a, const void *b);
diff --git a/src/port/qsort.c b/src/port/qsort.c
index 7879e6cd56..78354584af 100644
--- a/src/port/qsort.c
+++ b/src/port/qsort.c
@@ -4,6 +4,9 @@
 
 #include "c.h"
 
+/*
+ * NB: Callers should use qsort() instead of calling pg_qsort() directly.
+ */
 #define ST_SORT pg_qsort
 #define ST_ELEMENT_TYPE_VOID
 #define ST_COMPARE_RUNTIME_POINTER
-- 
2.25.1

v8-0002-Introduce-efficient-overflow-aware-integer-compar.patchtext/x-diff; charset=iso-8859-1Download
From f91fe2d895bc19acccac06a098fe947df7d3ac41 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 15 Feb 2024 15:52:10 -0600
Subject: [PATCH v8 2/3] Introduce efficient, overflow-aware integer comparison
 functions.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This commit adds integer comparison functions that are designed to
be as efficient as possible while avoiding overflow.  A follow-up
commit will make use of these functions in many of the in-tree
qsort() comparators.  The new functions are not a clear win in all
cases (e.g., when the comparator function is inlined), so it is
important to consider the context before using them.

Author: Mats Kindahl
Reviewed-by: Tom Lane, Heikki Linnakangas, Andres Freund, Thomas Munro, Andrey Borodin, Fabrízio de Royes Mello
Discussion: https://postgr.es/m/CA%2B14426g2Wa9QuUpmakwPxXFWG_1FaY0AsApkvcTBy-YfS6uaw%40mail.gmail.com
---
 src/include/common/int.h        | 75 ++++++++++++++++++++++++++++++++-
 src/include/lib/sort_template.h | 13 ++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/src/include/common/int.h b/src/include/common/int.h
index fedf7b3999..fa2d6329f1 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -1,7 +1,7 @@
 /*-------------------------------------------------------------------------
  *
  * int.h
- *	  Routines to perform integer math, while checking for overflows.
+ *	  Overflow-aware integer math and integer comparison routines.
  *
  * The routines in this file are intended to be well defined C, without
  * relying on compiler flags like -fwrapv.
@@ -22,7 +22,7 @@
 
 
 /*---------
- * The following guidelines apply to all the routines:
+ * The following guidelines apply to all the overflow routines:
  * - If a + b overflows, return true, otherwise store the result of a + b
  * into *result. The content of *result is implementation defined in case of
  * overflow.
@@ -438,4 +438,75 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result)
 #endif
 }
 
+/*------------------------------------------------------------------------
+ *
+ * Comparison routines for integer types.
+ *
+ * These routines are primarily intended for use in qsort() comparator
+ * functions and therefore return a positive integer, 0, or a negative
+ * integer depending on whether "a" is greater than, equal to, or less
+ * than "b", respectively.  These functions are written to be as efficient
+ * as possible without introducing overflow risks, thereby helping ensure
+ * the comparators that use them are transitive.
+ *
+ * Types with fewer than 32 bits are cast to signed integers and
+ * subtracted.  Other types are compared using > and <, and the results of
+ * those comparisons (which are either (int) 0 or (int) 1 per the C
+ * standard) are subtracted.
+ *
+ * NB: If the comparator functions are inlined, some compilers may produce
+ * worse code with these helper functions than with code with the
+ * following form:
+ *
+ *    if (a < b)
+ *        return -1;
+ *    if (a > b)
+ *        return 1;
+ *    return 0;
+ *
+ *------------------------------------------------------------------------
+ */
+
+static inline int
+pg_cmp_s16(int16 a, int16 b)
+{
+	return (int32) a - (int32) b;
+}
+
+static inline int
+pg_cmp_u16(uint16 a, uint16 b)
+{
+	return (int32) a - (int32) b;
+}
+
+static inline int
+pg_cmp_s32(int32 a, int32 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u32(uint32 a, uint32 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_s64(int64 a, int64 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_u64(uint64 a, uint64 b)
+{
+	return (a > b) - (a < b);
+}
+
+static inline int
+pg_cmp_size(size_t a, size_t b)
+{
+	return (a > b) - (a < b);
+}
+
 #endif							/* COMMON_INT_H */
diff --git a/src/include/lib/sort_template.h b/src/include/lib/sort_template.h
index a470fa1103..6772bf5ee3 100644
--- a/src/include/lib/sort_template.h
+++ b/src/include/lib/sort_template.h
@@ -34,6 +34,16 @@
  *	  - ST_COMPARE(a, b, arg) - variant that takes an extra argument
  *	  - ST_COMPARE_RUNTIME_POINTER - sort function takes a function pointer
  *
+ *    NB: If the comparator function is inlined, some compilers may produce
+ *    worse code with the optimized comparison routines in common/int.h than
+ *    with code with the following form:
+ *
+ *        if (a < b)
+ *            return -1;
+ *        if (a > b)
+ *            return 1;
+ *        return 0;
+ *
  *	  To say that the comparator and therefore also sort function should
  *	  receive an extra pass-through argument, specify the type of the
  *	  argument.
@@ -243,6 +253,9 @@ ST_SCOPE void ST_SORT(ST_ELEMENT_TYPE * first, size_t n
  * Find the median of three values.  Currently, performance seems to be best
  * if the comparator is inlined here, but the med3 function is not inlined
  * in the qsort function.
+ *
+ * Refer to the comment at the top of this file for known caveats to consider
+ * when writing inlined comparator functions.
  */
 static pg_noinline ST_ELEMENT_TYPE *
 ST_MED3(ST_ELEMENT_TYPE * a,
-- 
2.25.1

v8-0003-Use-new-overflow-aware-integer-comparison-functio.patchtext/x-diff; charset=iso-8859-1Download
From ee28e9a09bd024e4fbe3f59a0d462d0f1dba3000 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 15 Feb 2024 17:28:50 -0600
Subject: [PATCH v8 3/3] Use new overflow-aware integer comparison functions.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit XXXXXXXXXX introduced integer comparison functions designed
to be as efficient as possible while avoiding overflow.  This
commit makes use of these functions in many of the in-tree qsort()
comparators, which helps ensure transitivity.  The comparator
functions that previously had trivial implementations (i.e., "if"
statements) should also see a small performance boost.

Author: Mats Kindahl
Reviewed-by: Tom Lane, Heikki Linnakangas, Andres Freund, Fabrízio de Royes Mello
Discussion: https://postgr.es/m/CA%2B14426g2Wa9QuUpmakwPxXFWG_1FaY0AsApkvcTBy-YfS6uaw%40mail.gmail.com
---
 contrib/hstore/hstore_gist.c                    |  4 +++-
 contrib/intarray/_int_tool.c                    |  9 +++------
 contrib/intarray/_intbig_gist.c                 |  4 +++-
 contrib/pg_stat_statements/pg_stat_statements.c |  8 ++------
 contrib/pg_trgm/trgm_op.c                       |  8 ++------
 src/backend/access/nbtree/nbtinsert.c           |  8 ++------
 src/backend/access/nbtree/nbtpage.c             | 10 +++-------
 src/backend/access/nbtree/nbtsplitloc.c         |  8 ++------
 src/backend/access/spgist/spgdoinsert.c         |  5 ++---
 src/backend/access/spgist/spgtextproc.c         |  3 ++-
 src/backend/backup/basebackup_incremental.c     |  8 ++------
 src/backend/backup/walsummary.c                 |  7 ++-----
 src/backend/catalog/heap.c                      |  7 ++-----
 src/backend/nodes/list.c                        | 13 +++----------
 src/backend/nodes/tidbitmap.c                   |  7 ++-----
 src/backend/parser/parse_agg.c                  |  3 ++-
 src/backend/postmaster/autovacuum.c             |  7 +++----
 src/backend/replication/logical/reorderbuffer.c |  7 ++-----
 src/backend/replication/syncrep.c               |  8 ++------
 src/backend/utils/adt/oid.c                     |  7 ++-----
 src/backend/utils/adt/tsgistidx.c               | 10 +++-------
 src/backend/utils/adt/tsquery_gist.c            |  7 +++----
 src/backend/utils/adt/tsvector.c                |  5 ++---
 src/backend/utils/adt/tsvector_op.c             |  5 ++---
 src/backend/utils/adt/xid.c                     |  7 ++-----
 src/backend/utils/cache/relcache.c              |  3 ++-
 src/backend/utils/cache/syscache.c              |  5 ++---
 src/backend/utils/cache/typcache.c              |  8 ++------
 src/backend/utils/resowner/resowner.c           | 10 ++--------
 src/bin/pg_dump/pg_dump_sort.c                  |  7 ++-----
 src/bin/pg_upgrade/function.c                   | 12 ++++++------
 src/bin/pg_walsummary/pg_walsummary.c           |  8 ++------
 src/bin/psql/crosstabview.c                     |  3 ++-
 src/include/access/gin_private.h                |  8 ++------
 34 files changed, 80 insertions(+), 159 deletions(-)

diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index fe343739eb..a3b08af385 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -7,6 +7,7 @@
 #include "access/reloptions.h"
 #include "access/stratnum.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "hstore.h"
 #include "utils/pg_crc.h"
 
@@ -356,7 +357,8 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost,
+					  ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/intarray/_int_tool.c b/contrib/intarray/_int_tool.c
index 68f624e085..c85280c842 100644
--- a/contrib/intarray/_int_tool.c
+++ b/contrib/intarray/_int_tool.c
@@ -7,6 +7,7 @@
 
 #include "_int.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 
 /* arguments are assumed sorted & unique-ified */
@@ -396,15 +397,11 @@ int_to_intset(int32 elem)
 int
 compASC(const void *a, const void *b)
 {
-	if (*(const int32 *) a == *(const int32 *) b)
-		return 0;
-	return (*(const int32 *) a > *(const int32 *) b) ? 1 : -1;
+	return pg_cmp_s32(*(const int32 *) a, *(const int32 *) b);
 }
 
 int
 compDESC(const void *a, const void *b)
 {
-	if (*(const int32 *) a == *(const int32 *) b)
-		return 0;
-	return (*(const int32 *) a < *(const int32 *) b) ? 1 : -1;
+	return pg_cmp_s32(*(const int32 *) b, *(const int32 *) a);
 }
diff --git a/contrib/intarray/_intbig_gist.c b/contrib/intarray/_intbig_gist.c
index 8c6c4b5d05..9699fbf3b4 100644
--- a/contrib/intarray/_intbig_gist.c
+++ b/contrib/intarray/_intbig_gist.c
@@ -9,6 +9,7 @@
 #include "access/gist.h"
 #include "access/reloptions.h"
 #include "access/stratnum.h"
+#include "common/int.h"
 #include "port/pg_bitutils.h"
 
 #define GETENTRY(vec,pos) ((GISTTYPE *) DatumGetPointer((vec)->vector[(pos)].key))
@@ -312,7 +313,8 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	return ((const SPLITCOST *) a)->cost - ((const SPLITCOST *) b)->cost;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost,
+					  ((const SPLITCOST *) b)->cost);
 }
 
 
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 8c6a3a2d08..67cec865ba 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -50,6 +50,7 @@
 #include "access/parallel.h"
 #include "catalog/pg_authid.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "executor/instrument.h"
 #include "funcapi.h"
 #include "jit/jit.h"
@@ -3007,10 +3008,5 @@ comp_location(const void *a, const void *b)
 	int			l = ((const LocationLen *) a)->location;
 	int			r = ((const LocationLen *) b)->location;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return +1;
-	else
-		return 0;
+	return pg_cmp_s32(l, r);
 }
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 49d4497b4f..c509d15ee4 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -6,6 +6,7 @@
 #include <ctype.h>
 
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "miscadmin.h"
 #include "trgm.h"
@@ -433,12 +434,7 @@ comp_ptrgm(const void *v1, const void *v2)
 	if (cmp != 0)
 		return cmp;
 
-	if (p1->index < p2->index)
-		return -1;
-	else if (p1->index == p2->index)
-		return 0;
-	else
-		return 1;
+	return pg_cmp_s32(p1->index, p2->index);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 709edd1c17..e9cfc13604 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -19,6 +19,7 @@
 #include "access/nbtxlog.h"
 #include "access/transam.h"
 #include "access/xloginsert.h"
+#include "common/int.h"
 #include "common/pg_prng.h"
 #include "lib/qunique.h"
 #include "miscadmin.h"
@@ -3013,10 +3014,5 @@ _bt_blk_cmp(const void *arg1, const void *arg2)
 	BlockNumber b1 = *((BlockNumber *) arg1);
 	BlockNumber b2 = *((BlockNumber *) arg2);
 
-	if (b1 < b2)
-		return -1;
-	else if (b1 > b2)
-		return 1;
-
-	return 0;
+	return pg_cmp_u32(b1, b2);
 }
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 567bade9f4..90990ea77a 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -28,6 +28,7 @@
 #include "access/transam.h"
 #include "access/xlog.h"
 #include "access/xloginsert.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "storage/indexfsm.h"
 #include "storage/lmgr.h"
@@ -1466,14 +1467,9 @@ _bt_delitems_cmp(const void *a, const void *b)
 	TM_IndexDelete *indexdelete1 = (TM_IndexDelete *) a;
 	TM_IndexDelete *indexdelete2 = (TM_IndexDelete *) b;
 
-	if (indexdelete1->id > indexdelete2->id)
-		return 1;
-	if (indexdelete1->id < indexdelete2->id)
-		return -1;
+	Assert(indexdelete1->id != indexdelete2->id);
 
-	Assert(false);
-
-	return 0;
+	return pg_cmp_s16(indexdelete1->id, indexdelete2->id);
 }
 
 /*
diff --git a/src/backend/access/nbtree/nbtsplitloc.c b/src/backend/access/nbtree/nbtsplitloc.c
index d0b1d82578..490e7bfd4d 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -15,6 +15,7 @@
 #include "postgres.h"
 
 #include "access/nbtree.h"
+#include "common/int.h"
 #include "storage/lmgr.h"
 
 typedef enum
@@ -596,12 +597,7 @@ _bt_splitcmp(const void *arg1, const void *arg2)
 	SplitPoint *split1 = (SplitPoint *) arg1;
 	SplitPoint *split2 = (SplitPoint *) arg2;
 
-	if (split1->curdelta > split2->curdelta)
-		return 1;
-	if (split1->curdelta < split2->curdelta)
-		return -1;
-
-	return 0;
+	return pg_cmp_s16(split1->curdelta, split2->curdelta);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index bb063c858d..a4995c168b 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -19,6 +19,7 @@
 #include "access/spgist_private.h"
 #include "access/spgxlog.h"
 #include "access/xloginsert.h"
+#include "common/int.h"
 #include "common/pg_prng.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
@@ -110,9 +111,7 @@ addNode(SpGistState *state, SpGistInnerTuple tuple, Datum label, int offset)
 static int
 cmpOffsetNumbers(const void *a, const void *b)
 {
-	if (*(const OffsetNumber *) a == *(const OffsetNumber *) b)
-		return 0;
-	return (*(const OffsetNumber *) a > *(const OffsetNumber *) b) ? 1 : -1;
+	return pg_cmp_u16(*(const OffsetNumber *) a, *(const OffsetNumber *) b);
 }
 
 /*
diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index b8fd0c2ad8..d5db5225a9 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -40,6 +40,7 @@
 #include "postgres.h"
 
 #include "access/spgist.h"
+#include "common/int.h"
 #include "catalog/pg_type.h"
 #include "mb/pg_wchar.h"
 #include "utils/builtins.h"
@@ -325,7 +326,7 @@ cmpNodePtr(const void *a, const void *b)
 	const spgNodePtr *aa = (const spgNodePtr *) a;
 	const spgNodePtr *bb = (const spgNodePtr *) b;
 
-	return aa->c - bb->c;
+	return pg_cmp_s16(aa->c, bb->c);
 }
 
 Datum
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 0504c465db..e994ee66bb 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -27,6 +27,7 @@
 #include "common/blkreftable.h"
 #include "common/parse_manifest.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "postmaster/walsummarizer.h"
 
 #define	BLOCKS_PER_READ			512
@@ -994,10 +995,5 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return pg_cmp_u32(aa, bb);
 }
diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 867870aaad..4d047e1c02 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -17,6 +17,7 @@
 
 #include "access/xlog_internal.h"
 #include "backup/walsummary.h"
+#include "common/int.h"
 #include "utils/wait_event.h"
 
 static bool IsWalSummaryFilename(char *filename);
@@ -355,9 +356,5 @@ ListComparatorForWalSummaryFiles(const ListCell *a, const ListCell *b)
 	WalSummaryFile *ws1 = lfirst(a);
 	WalSummaryFile *ws2 = lfirst(b);
 
-	if (ws1->start_lsn < ws2->start_lsn)
-		return -1;
-	if (ws1->start_lsn > ws2->start_lsn)
-		return 1;
-	return 0;
+	return pg_cmp_u64(ws1->start_lsn, ws2->start_lsn);
 }
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c73f7bcd01..03368b93d7 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -56,6 +56,7 @@
 #include "catalog/storage.h"
 #include "commands/tablecmds.h"
 #include "commands/typecmds.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -2759,11 +2760,7 @@ list_cookedconstr_attnum_cmp(const ListCell *p1, const ListCell *p2)
 	AttrNumber	v1 = ((CookedConstraint *) lfirst(p1))->attnum;
 	AttrNumber	v2 = ((CookedConstraint *) lfirst(p2))->attnum;
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_s16(v1, v2);
 }
 
 /*
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 957186bef5..e2615ab105 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -17,6 +17,7 @@
  */
 #include "postgres.h"
 
+#include "common/int.h"
 #include "nodes/pg_list.h"
 #include "port/pg_bitutils.h"
 #include "utils/memdebug.h"
@@ -1692,11 +1693,7 @@ list_int_cmp(const ListCell *p1, const ListCell *p2)
 	int			v1 = lfirst_int(p1);
 	int			v2 = lfirst_int(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_s32(v1, v2);
 }
 
 /*
@@ -1708,9 +1705,5 @@ list_oid_cmp(const ListCell *p1, const ListCell *p2)
 	Oid			v1 = lfirst_oid(p1);
 	Oid			v2 = lfirst_oid(p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_u32(v1, v2);
 }
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index 0f4850065f..e8ab5d78fc 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -42,6 +42,7 @@
 
 #include "access/htup_details.h"
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "nodes/bitmapset.h"
 #include "nodes/tidbitmap.h"
 #include "storage/lwlock.h"
@@ -1425,11 +1426,7 @@ tbm_comparator(const void *left, const void *right)
 	BlockNumber l = (*((PagetableEntry *const *) left))->blockno;
 	BlockNumber r = (*((PagetableEntry *const *) right))->blockno;
 
-	if (l < r)
-		return -1;
-	else if (l > r)
-		return 1;
-	return 0;
+	return pg_cmp_u32(l, r);
 }
 
 /*
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 7b211a7743..9d151a880b 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -18,6 +18,7 @@
 #include "catalog/pg_aggregate.h"
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
@@ -1760,7 +1761,7 @@ cmp_list_len_asc(const ListCell *a, const ListCell *b)
 	int			la = list_length((const List *) lfirst(a));
 	int			lb = list_length((const List *) lfirst(b));
 
-	return (la > lb) ? 1 : (la == lb) ? 0 : -1;
+	return pg_cmp_s32(la, lb);
 }
 
 /* list_sort comparator to sort sub-lists by length and contents */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 37998f7387..2ab344c1f8 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -78,6 +78,7 @@
 #include "catalog/pg_database.h"
 #include "commands/dbcommands.h"
 #include "commands/vacuum.h"
+#include "common/int.h"
 #include "lib/ilist.h"
 #include "libpq/pqsignal.h"
 #include "miscadmin.h"
@@ -1120,10 +1121,8 @@ rebuild_database_list(Oid newdb)
 static int
 db_comparator(const void *a, const void *b)
 {
-	if (((const avl_dbase *) a)->adl_score == ((const avl_dbase *) b)->adl_score)
-		return 0;
-	else
-		return (((const avl_dbase *) a)->adl_score < ((const avl_dbase *) b)->adl_score) ? 1 : -1;
+	return pg_cmp_s32(((const avl_dbase *) a)->adl_score,
+					  ((const avl_dbase *) b)->adl_score);
 }
 
 /*
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index bbf0966182..5446df3c64 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -91,6 +91,7 @@
 #include "access/xact.h"
 #include "access/xlog_internal.h"
 #include "catalog/catalog.h"
+#include "common/int.h"
 #include "lib/binaryheap.h"
 #include "miscadmin.h"
 #include "pgstat.h"
@@ -5119,11 +5120,7 @@ file_sort_by_lsn(const ListCell *a_p, const ListCell *b_p)
 	RewriteMappingFile *a = (RewriteMappingFile *) lfirst(a_p);
 	RewriteMappingFile *b = (RewriteMappingFile *) lfirst(b_p);
 
-	if (a->lsn < b->lsn)
-		return -1;
-	else if (a->lsn > b->lsn)
-		return 1;
-	return 0;
+	return pg_cmp_u64(a->lsn, b->lsn);
 }
 
 /*
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 2e6493aaaa..bfcd8fa13e 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -75,6 +75,7 @@
 #include <unistd.h>
 
 #include "access/xact.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "pgstat.h"
 #include "replication/syncrep.h"
@@ -698,12 +699,7 @@ cmp_lsn(const void *a, const void *b)
 	XLogRecPtr	lsn1 = *((const XLogRecPtr *) a);
 	XLogRecPtr	lsn2 = *((const XLogRecPtr *) b);
 
-	if (lsn1 > lsn2)
-		return -1;
-	else if (lsn1 == lsn2)
-		return 0;
-	else
-		return 1;
+	return pg_cmp_u64(lsn2, lsn1);
 }
 
 /*
diff --git a/src/backend/utils/adt/oid.c b/src/backend/utils/adt/oid.c
index 62bcfc5b56..56fb1fd77c 100644
--- a/src/backend/utils/adt/oid.c
+++ b/src/backend/utils/adt/oid.c
@@ -18,6 +18,7 @@
 #include <limits.h>
 
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
 #include "nodes/value.h"
@@ -259,11 +260,7 @@ oid_cmp(const void *p1, const void *p2)
 	Oid			v1 = *((const Oid *) p1);
 	Oid			v2 = *((const Oid *) p2);
 
-	if (v1 < v2)
-		return -1;
-	if (v1 > v2)
-		return 1;
-	return 0;
+	return pg_cmp_u32(v1, v2);
 }
 
 
diff --git a/src/backend/utils/adt/tsgistidx.c b/src/backend/utils/adt/tsgistidx.c
index a62b285365..3fb7696434 100644
--- a/src/backend/utils/adt/tsgistidx.c
+++ b/src/backend/utils/adt/tsgistidx.c
@@ -17,6 +17,7 @@
 #include "access/gist.h"
 #include "access/heaptoast.h"
 #include "access/reloptions.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "port/pg_bitutils.h"
 #include "tsearch/ts_utils.h"
@@ -136,9 +137,7 @@ compareint(const void *va, const void *vb)
 	int32		a = *((const int32 *) va);
 	int32		b = *((const int32 *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return pg_cmp_s32(a, b);
 }
 
 static void
@@ -598,10 +597,7 @@ comparecost(const void *va, const void *vb)
 	const SPLITCOST *a = (const SPLITCOST *) va;
 	const SPLITCOST *b = (const SPLITCOST *) vb;
 
-	if (a->cost == b->cost)
-		return 0;
-	else
-		return (a->cost > b->cost) ? 1 : -1;
+	return pg_cmp_s32(a->cost, b->cost);
 }
 
 
diff --git a/src/backend/utils/adt/tsquery_gist.c b/src/backend/utils/adt/tsquery_gist.c
index f0b1c81c81..2db304b10b 100644
--- a/src/backend/utils/adt/tsquery_gist.c
+++ b/src/backend/utils/adt/tsquery_gist.c
@@ -16,6 +16,7 @@
 
 #include "access/gist.h"
 #include "access/stratnum.h"
+#include "common/int.h"
 #include "tsearch/ts_utils.h"
 #include "utils/builtins.h"
 
@@ -156,10 +157,8 @@ typedef struct
 static int
 comparecost(const void *a, const void *b)
 {
-	if (((const SPLITCOST *) a)->cost == ((const SPLITCOST *) b)->cost)
-		return 0;
-	else
-		return (((const SPLITCOST *) a)->cost > ((const SPLITCOST *) b)->cost) ? 1 : -1;
+	return pg_cmp_s32(((const SPLITCOST *) a)->cost,
+					  ((const SPLITCOST *) b)->cost);
 }
 
 #define WISH_F(a,b,c) (double)( -(double)(((a)-(b))*((a)-(b))*((a)-(b)))*(c) )
diff --git a/src/backend/utils/adt/tsvector.c b/src/backend/utils/adt/tsvector.c
index fb7b7c712a..10bc4f2234 100644
--- a/src/backend/utils/adt/tsvector.c
+++ b/src/backend/utils/adt/tsvector.c
@@ -14,6 +14,7 @@
 
 #include "postgres.h"
 
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
 #include "tsearch/ts_locale.h"
@@ -37,9 +38,7 @@ compareWordEntryPos(const void *a, const void *b)
 	int			apos = WEP_GETPOS(*(const WordEntryPos *) a);
 	int			bpos = WEP_GETPOS(*(const WordEntryPos *) b);
 
-	if (apos == bpos)
-		return 0;
-	return (apos > bpos) ? 1 : -1;
+	return pg_cmp_s32(apos, bpos);
 }
 
 /*
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index 71386d0a1f..947a592ed2 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -19,6 +19,7 @@
 #include "catalog/namespace.h"
 #include "catalog/pg_type.h"
 #include "commands/trigger.h"
+#include "common/int.h"
 #include "executor/spi.h"
 #include "funcapi.h"
 #include "lib/qunique.h"
@@ -435,9 +436,7 @@ compare_int(const void *va, const void *vb)
 	int			a = *((const int *) va);
 	int			b = *((const int *) vb);
 
-	if (a == b)
-		return 0;
-	return (a > b) ? 1 : -1;
+	return pg_cmp_s32(a, b);
 }
 
 static int
diff --git a/src/backend/utils/adt/xid.c b/src/backend/utils/adt/xid.c
index 63adf5668b..ae273b1961 100644
--- a/src/backend/utils/adt/xid.c
+++ b/src/backend/utils/adt/xid.c
@@ -19,6 +19,7 @@
 #include "access/multixact.h"
 #include "access/transam.h"
 #include "access/xact.h"
+#include "common/int.h"
 #include "libpq/pqformat.h"
 #include "utils/builtins.h"
 #include "utils/xid8.h"
@@ -140,11 +141,7 @@ xidComparator(const void *arg1, const void *arg2)
 	TransactionId xid1 = *(const TransactionId *) arg1;
 	TransactionId xid2 = *(const TransactionId *) arg2;
 
-	if (xid1 > xid2)
-		return 1;
-	if (xid1 < xid2)
-		return -1;
-	return 0;
+	return pg_cmp_u32(xid1, xid2);
 }
 
 /*
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index ac106b40e3..50acae4529 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -69,6 +69,7 @@
 #include "commands/policy.h"
 #include "commands/publicationcmds.h"
 #include "commands/trigger.h"
+#include "common/int.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
@@ -4520,7 +4521,7 @@ AttrDefaultCmp(const void *a, const void *b)
 	const AttrDefault *ada = (const AttrDefault *) a;
 	const AttrDefault *adb = (const AttrDefault *) b;
 
-	return ada->adnum - adb->adnum;
+	return pg_cmp_s16(ada->adnum, adb->adnum);
 }
 
 /*
diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c
index 662f930864..2292237f85 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -29,6 +29,7 @@
 #include "catalog/pg_shdepend_d.h"
 #include "catalog/pg_shdescription_d.h"
 #include "catalog/pg_shseclabel_d.h"
+#include "common/int.h"
 #include "lib/qunique.h"
 #include "utils/catcache.h"
 #include "utils/lsyscache.h"
@@ -676,7 +677,5 @@ oid_compare(const void *a, const void *b)
 	Oid			oa = *((const Oid *) a);
 	Oid			ob = *((const Oid *) b);
 
-	if (oa == ob)
-		return 0;
-	return (oa > ob) ? 1 : -1;
+	return pg_cmp_u32(oa, ob);
 }
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 84fc83cb11..2842bde907 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -57,6 +57,7 @@
 #include "catalog/pg_range.h"
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
+#include "common/int.h"
 #include "executor/executor.h"
 #include "lib/dshash.h"
 #include "optimizer/optimizer.h"
@@ -2722,12 +2723,7 @@ enum_oid_cmp(const void *left, const void *right)
 	const EnumItem *l = (const EnumItem *) left;
 	const EnumItem *r = (const EnumItem *) right;
 
-	if (l->enum_oid < r->enum_oid)
-		return -1;
-	else if (l->enum_oid > r->enum_oid)
-		return 1;
-	else
-		return 0;
+	return pg_cmp_u32(l->enum_oid, r->enum_oid);
 }
 
 /*
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index aa199b23ff..ab9343bc5c 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -46,6 +46,7 @@
 #include "postgres.h"
 
 #include "common/hashfn.h"
+#include "common/int.h"
 #include "storage/ipc.h"
 #include "storage/predicate.h"
 #include "storage/proc.h"
@@ -264,14 +265,7 @@ resource_priority_cmp(const void *a, const void *b)
 
 	/* Note: reverse order */
 	if (ra->kind->release_phase == rb->kind->release_phase)
-	{
-		if (ra->kind->release_priority == rb->kind->release_priority)
-			return 0;
-		else if (ra->kind->release_priority > rb->kind->release_priority)
-			return -1;
-		else
-			return 1;
-	}
+		return pg_cmp_u32(rb->kind->release_priority, ra->kind->release_priority);
 	else if (ra->kind->release_phase > rb->kind->release_phase)
 		return -1;
 	else
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index f358dd22b9..8ee8a42781 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -16,6 +16,7 @@
 #include "postgres_fe.h"
 
 #include "catalog/pg_class_d.h"
+#include "common/int.h"
 #include "lib/binaryheap.h"
 #include "pg_backup_archiver.h"
 #include "pg_backup_utils.h"
@@ -1504,9 +1505,5 @@ int_cmp(void *a, void *b, void *arg)
 	int			ai = (int) (intptr_t) a;
 	int			bi = (int) (intptr_t) b;
 
-	if (ai < bi)
-		return -1;
-	if (ai > bi)
-		return 1;
-	return 0;
+	return pg_cmp_s32(ai, bi);
 }
diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c
index af998f74d3..d65153de81 100644
--- a/src/bin/pg_upgrade/function.c
+++ b/src/bin/pg_upgrade/function.c
@@ -11,6 +11,7 @@
 
 #include "access/transam.h"
 #include "catalog/pg_language_d.h"
+#include "common/int.h"
 #include "pg_upgrade.h"
 
 /*
@@ -29,17 +30,16 @@ library_name_compare(const void *p1, const void *p2)
 {
 	const char *str1 = ((const LibraryInfo *) p1)->name;
 	const char *str2 = ((const LibraryInfo *) p2)->name;
-	int			slen1 = strlen(str1);
-	int			slen2 = strlen(str2);
+	size_t		slen1 = strlen(str1);
+	size_t		slen2 = strlen(str2);
 	int			cmp = strcmp(str1, str2);
 
 	if (slen1 != slen2)
-		return slen1 - slen2;
+		return pg_cmp_size(slen1, slen2);
 	if (cmp != 0)
 		return cmp;
-	else
-		return ((const LibraryInfo *) p1)->dbnum -
-			((const LibraryInfo *) p2)->dbnum;
+	return pg_cmp_s32(((const LibraryInfo *) p1)->dbnum,
+					  ((const LibraryInfo *) p2)->dbnum);
 }
 
 
diff --git a/src/bin/pg_walsummary/pg_walsummary.c b/src/bin/pg_walsummary/pg_walsummary.c
index 1341c83c69..485c72d939 100644
--- a/src/bin/pg_walsummary/pg_walsummary.c
+++ b/src/bin/pg_walsummary/pg_walsummary.c
@@ -16,6 +16,7 @@
 #include <limits.h>
 
 #include "common/blkreftable.h"
+#include "common/int.h"
 #include "common/logging.h"
 #include "fe_utils/option_utils.h"
 #include "lib/stringinfo.h"
@@ -219,12 +220,7 @@ compare_block_numbers(const void *a, const void *b)
 	BlockNumber aa = *(BlockNumber *) a;
 	BlockNumber bb = *(BlockNumber *) b;
 
-	if (aa > bb)
-		return 1;
-	else if (aa == bb)
-		return 0;
-	else
-		return -1;
+	return pg_cmp_u32(aa, bb);
 }
 
 /*
diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c
index c6116b7238..305ed4ab0a 100644
--- a/src/bin/psql/crosstabview.c
+++ b/src/bin/psql/crosstabview.c
@@ -8,6 +8,7 @@
 #include "postgres_fe.h"
 
 #include "common.h"
+#include "common/int.h"
 #include "common/logging.h"
 #include "crosstabview.h"
 #include "pqexpbuffer.h"
@@ -709,5 +710,5 @@ pivotFieldCompare(const void *a, const void *b)
 static int
 rankCompare(const void *a, const void *b)
 {
-	return *((const int *) a) - *((const int *) b);
+	return pg_cmp_s32(*(const int *) a, *(const int *) b);
 }
diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 51d0c74a6b..3013a44bae 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -14,6 +14,7 @@
 #include "access/gin.h"
 #include "access/ginblock.h"
 #include "access/itup.h"
+#include "common/int.h"
 #include "catalog/pg_am_d.h"
 #include "fmgr.h"
 #include "lib/rbtree.h"
@@ -489,12 +490,7 @@ ginCompareItemPointers(ItemPointer a, ItemPointer b)
 	uint64		ia = (uint64) GinItemPointerGetBlockNumber(a) << 32 | GinItemPointerGetOffsetNumber(a);
 	uint64		ib = (uint64) GinItemPointerGetBlockNumber(b) << 32 | GinItemPointerGetOffsetNumber(b);
 
-	if (ia == ib)
-		return 0;
-	else if (ia > ib)
-		return 1;
-	else
-		return -1;
+	return pg_cmp_u64(ia, ib);
 }
 
 extern int	ginTraverseLock(Buffer buffer, bool searchMode);
-- 
2.25.1

#59Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#58)
Re: glibc qsort() vulnerability

On Fri, Feb 16, 2024 at 12:32 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:

Here is what I have staged for commit.

Looks good to me.

Checked that all of the comparisons are in the expected order, except
inside compDESC, cmp_lsn, and resource_priority_cmp, where the order is
reversed.

Best wishes,
Mats Kindahl

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#60Nathan Bossart
nathandbossart@gmail.com
In reply to: Mats Kindahl (#59)
Re: glibc qsort() vulnerability

On Fri, Feb 16, 2024 at 01:45:52PM +0100, Mats Kindahl wrote:

Looks good to me.

Committed.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

#61Mats Kindahl
mats@timescale.com
In reply to: Nathan Bossart (#60)
Re: glibc qsort() vulnerability

On Fri, Feb 16, 2024 at 9:09 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Fri, Feb 16, 2024 at 01:45:52PM +0100, Mats Kindahl wrote:

Looks good to me.

Committed.

Thanks Nathan!

Show quoted text

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com