I wrote:
> Andres Freund <and...@anarazel.de> writes:
>> On 2018-10-01 12:13:57 -0400, Tom Lane wrote:
>>> (2) Drop the restriction. This'd require at least changing the
>>> DESC correction, and maybe other things. I'm not sure what the
>>> odds would be of finding everyplace we need to check.
>> (2) seems more maintainable to me (or perhaps less unmaintainable). It's
>> infrastructure, rather than every datatype + support out there...
> I guess we could set up some testing infrastructure: hack int4cmp
> and/or a couple other popular comparators so that they *always*
> return INT_MIN, 0, or INT_MAX, and then see what falls over.
Here's a draft patch against HEAD for this.
I looked for problem spots by (a) testing with the STRESS_SORT_INT_MIN
option I added in nbtcompare.c, (b) grepping for "x = -x" type code,
and (c) grepping for "return -x" type code. (b) and (c) found several
places that (a) didn't, which does not give me a warm feeling about
whether I have found quite everything.
I changed a couple of places where things might've been safe but
I didn't feel like chasing the calls to prove it (e.g. imath.c),
and contrariwise I left a *very* small number of places alone
because they were inverting the result of a specific function
that is defined to return 1/0/-1 and nothing else.
regards, tom lane
diff --git a/contrib/pgcrypto/imath.c b/contrib/pgcrypto/imath.c
index cd528bf..ca4995a 100644
--- a/contrib/pgcrypto/imath.c
+++ b/contrib/pgcrypto/imath.c
@@ -1254,11 +1254,9 @@ mp_int_compare(mp_int a, mp_int b)
* If they're both zero or positive, the normal comparison applies; if
* both negative, the sense is reversed.
*/
- if (sa == MP_ZPOS)
- return cmp;
- else
- return -cmp;
-
+ if (sa != MP_ZPOS)
+ INVERT_SIGN(cmp);
+ return cmp;
}
else
{
@@ -1314,10 +1312,9 @@ mp_int_compare_value(mp_int z, int value)
{
cmp = s_vcmp(z, value);
- if (vsign == MP_ZPOS)
- return cmp;
- else
- return -cmp;
+ if (vsign != MP_ZPOS)
+ INVERT_SIGN(cmp);
+ return cmp;
}
else
{
diff --git a/doc/src/sgml/btree.sgml b/doc/src/sgml/btree.sgml
index 8bd0bad..c16825e 100644
--- a/doc/src/sgml/btree.sgml
+++ b/doc/src/sgml/btree.sgml
@@ -228,11 +228,8 @@
<replaceable>B</replaceable>, <replaceable>A</replaceable>
<literal>=</literal> <replaceable>B</replaceable>,
or <replaceable>A</replaceable> <literal>></literal>
- <replaceable>B</replaceable>, respectively. The function must not
- return <literal>INT_MIN</literal> for the <replaceable>A</replaceable>
- <literal><</literal> <replaceable>B</replaceable> case,
- since the value may be negated before being tested for sign. A null
- result is disallowed, too.
+ <replaceable>B</replaceable>, respectively.
+ A null result is disallowed: all values of the data type must be comparable.
See <filename>src/backend/access/nbtree/nbtcompare.c</filename> for
examples.
</para>
diff --git a/src/backend/access/nbtree/nbtcompare.c b/src/backend/access/nbtree/nbtcompare.c
index b1855e8..6f2ad23 100644
--- a/src/backend/access/nbtree/nbtcompare.c
+++ b/src/backend/access/nbtree/nbtcompare.c
@@ -22,11 +22,10 @@
*
* The result is always an int32 regardless of the input datatype.
*
- * Although any negative int32 (except INT_MIN) is acceptable for reporting
- * "<", and any positive int32 is acceptable for reporting ">", routines
+ * Although any negative int32 is acceptable for reporting "<",
+ * and any positive int32 is acceptable for reporting ">", routines
* that work on 32-bit or wider datatypes can't just return "a - b".
- * That could overflow and give the wrong answer. Also, one must not
- * return INT_MIN to report "<", since some callers will negate the result.
+ * That could overflow and give the wrong answer.
*
* NOTE: it is critical that the comparison function impose a total order
* on all non-NULL values of the data type, and that the datatype's
@@ -44,13 +43,31 @@
* during an index access won't be recovered till end of query. This
* primarily affects comparison routines for toastable datatypes;
* they have to be careful to free any detoasted copy of an input datum.
+ *
+ * NOTE: we used to forbid comparison functions from returning INT_MIN,
+ * but that proves to be too error-prone because some platforms' versions
+ * of memcmp() etc can return INT_MIN. As a means of stress-testing
+ * callers, this file can be compiled with STRESS_SORT_INT_MIN defined
+ * to cause many of these functions to return INT_MIN or INT_MAX instead of
+ * their customary -1/+1. For production, though, that's not a good idea
+ * since users or third-party code might expect the traditional results.
*-------------------------------------------------------------------------
*/
#include "postgres.h"
+#include <limits.h>
+
#include "utils/builtins.h"
#include "utils/sortsupport.h"
+#ifdef STRESS_SORT_INT_MIN
+#define A_LESS_THAN_B INT_MIN
+#define A_GREATER_THAN_B INT_MAX
+#else
+#define A_LESS_THAN_B (-1)
+#define A_GREATER_THAN_B 1
+#endif
+
Datum
btboolcmp(PG_FUNCTION_ARGS)
@@ -95,11 +112,11 @@ btint4cmp(PG_FUNCTION_ARGS)
int32 b = PG_GETARG_INT32(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
static int
@@ -109,11 +126,11 @@ btint4fastcmp(Datum x, Datum y, SortSupport ssup)
int32 b = DatumGetInt32(y);
if (a > b)
- return 1;
+ return A_GREATER_THAN_B;
else if (a == b)
return 0;
else
- return -1;
+ return A_LESS_THAN_B;
}
Datum
@@ -132,11 +149,11 @@ btint8cmp(PG_FUNCTION_ARGS)
int64 b = PG_GETARG_INT64(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
static int
@@ -146,11 +163,11 @@ btint8fastcmp(Datum x, Datum y, SortSupport ssup)
int64 b = DatumGetInt64(y);
if (a > b)
- return 1;
+ return A_GREATER_THAN_B;
else if (a == b)
return 0;
else
- return -1;
+ return A_LESS_THAN_B;
}
Datum
@@ -169,11 +186,11 @@ btint48cmp(PG_FUNCTION_ARGS)
int64 b = PG_GETARG_INT64(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
Datum
@@ -183,11 +200,11 @@ btint84cmp(PG_FUNCTION_ARGS)
int32 b = PG_GETARG_INT32(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
Datum
@@ -197,11 +214,11 @@ btint24cmp(PG_FUNCTION_ARGS)
int32 b = PG_GETARG_INT32(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
Datum
@@ -211,11 +228,11 @@ btint42cmp(PG_FUNCTION_ARGS)
int16 b = PG_GETARG_INT16(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
Datum
@@ -225,11 +242,11 @@ btint28cmp(PG_FUNCTION_ARGS)
int64 b = PG_GETARG_INT64(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
Datum
@@ -239,11 +256,11 @@ btint82cmp(PG_FUNCTION_ARGS)
int16 b = PG_GETARG_INT16(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
Datum
@@ -253,11 +270,11 @@ btoidcmp(PG_FUNCTION_ARGS)
Oid b = PG_GETARG_OID(1);
if (a > b)
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else if (a == b)
PG_RETURN_INT32(0);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
static int
@@ -267,11 +284,11 @@ btoidfastcmp(Datum x, Datum y, SortSupport ssup)
Oid b = DatumGetObjectId(y);
if (a > b)
- return 1;
+ return A_GREATER_THAN_B;
else if (a == b)
return 0;
else
- return -1;
+ return A_LESS_THAN_B;
}
Datum
@@ -299,9 +316,9 @@ btoidvectorcmp(PG_FUNCTION_ARGS)
if (a->values[i] != b->values[i])
{
if (a->values[i] > b->values[i])
- PG_RETURN_INT32(1);
+ PG_RETURN_INT32(A_GREATER_THAN_B);
else
- PG_RETURN_INT32(-1);
+ PG_RETURN_INT32(A_LESS_THAN_B);
}
}
PG_RETURN_INT32(0);
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index d3700bd..8091db3 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -530,7 +530,7 @@ _bt_compare(Relation rel,
scankey->sk_argument));
if (!(scankey->sk_flags & SK_BT_DESC))
- result = -result;
+ INVERT_SIGN(result);
}
/* if the keys are unequal, return the difference */
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 4528e87..365153b 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -518,7 +518,7 @@ _bt_compare_array_elements(const void *a, const void *b, void *arg)
cxt->collation,
da, db));
if (cxt->reverse)
- compare = -compare;
+ INVERT_SIGN(compare);
return compare;
}
@@ -1652,7 +1652,7 @@ _bt_check_rowcompare(ScanKey skey, IndexTuple tuple, TupleDesc tupdesc,
subkey->sk_argument));
if (subkey->sk_flags & SK_BT_DESC)
- cmpresult = -cmpresult;
+ INVERT_SIGN(cmpresult);
/* Done comparing if unequal, else advance to next column */
if (cmpresult != 0)
diff --git a/src/backend/executor/nodeGatherMerge.c b/src/backend/executor/nodeGatherMerge.c
index a1a11df..32f67f7 100644
--- a/src/backend/executor/nodeGatherMerge.c
+++ b/src/backend/executor/nodeGatherMerge.c
@@ -755,7 +755,10 @@ heap_compare_slots(Datum a, Datum b, void *arg)
datum2, isNull2,
sortKey);
if (compare != 0)
- return -compare;
+ {
+ INVERT_SIGN(compare);
+ return compare;
+ }
}
return 0;
}
diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c
index d74499f..d952766 100644
--- a/src/backend/executor/nodeIndexscan.c
+++ b/src/backend/executor/nodeIndexscan.c
@@ -467,9 +467,10 @@ reorderqueue_cmp(const pairingheap_node *a, const pairingheap_node *b,
ReorderTuple *rtb = (ReorderTuple *) b;
IndexScanState *node = (IndexScanState *) arg;
- return -cmp_orderbyvals(rta->orderbyvals, rta->orderbynulls,
- rtb->orderbyvals, rtb->orderbynulls,
- node);
+ /* exchange argument order to invert the sort order */
+ return cmp_orderbyvals(rtb->orderbyvals, rtb->orderbynulls,
+ rta->orderbyvals, rta->orderbynulls,
+ node);
}
/*
diff --git a/src/backend/executor/nodeMergeAppend.c b/src/backend/executor/nodeMergeAppend.c
index 6daf60a..a2b3856 100644
--- a/src/backend/executor/nodeMergeAppend.c
+++ b/src/backend/executor/nodeMergeAppend.c
@@ -338,7 +338,10 @@ heap_compare_slots(Datum a, Datum b, void *arg)
datum2, isNull2,
sortKey);
if (compare != 0)
- return -compare;
+ {
+ INVERT_SIGN(compare);
+ return compare;
+ }
}
return 0;
}
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 4ad7b2f..222b56f 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -810,7 +810,7 @@ final_filemap_cmp(const void *a, const void *b)
return -1;
if (fa->action == FILE_ACTION_REMOVE)
- return -strcmp(fa->path, fb->path);
+ return strcmp(fb->path, fa->path);
else
return strcmp(fa->path, fb->path);
}
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 04ecb4c..ea495f1 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -274,8 +274,7 @@ typedef struct BTMetaPageData
* When a new operator class is declared, we require that the user
* supply us with an amproc procedure (BTORDER_PROC) for determining
* whether, for two keys a and b, a < b, a = b, or a > b. This routine
- * must return < 0, 0, > 0, respectively, in these three cases. (It must
- * not return INT_MIN, since we may negate the result before using it.)
+ * must return < 0, 0, > 0, respectively, in these three cases.
*
* To facilitate accelerated sorting, an operator class may choose to
* offer a second procedure (BTSORTSUPPORT_PROC). For full details, see
diff --git a/src/include/c.h b/src/include/c.h
index 25d7d60..a3bce9c 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1023,6 +1023,14 @@ extern void ExceptionalCondition(const char *conditionName,
*/
/*
+ * Invert the sign of a qsort-style comparison result, ie, exchange negative
+ * and positive integer values, being careful not to get the wrong answer
+ * for INT_MIN. The argument should be an integral variable.
+ */
+#define INVERT_SIGN(var) \
+ ((var) = ((var) < 0) ? 1 : -(var))
+
+/*
* Use this, not "char buf[BLCKSZ]", to declare a field or local variable
* holding a page buffer, if that page might be accessed as a page and not
* just a string of bytes. Otherwise the variable might be under-aligned,
diff --git a/src/include/utils/sortsupport.h b/src/include/utils/sortsupport.h
index 53b692e..4166fb9 100644
--- a/src/include/utils/sortsupport.h
+++ b/src/include/utils/sortsupport.h
@@ -96,8 +96,7 @@ typedef struct SortSupportData
* Comparator function has the same API as the traditional btree
* comparison function, ie, return <0, 0, or >0 according as x is less
* than, equal to, or greater than y. Note that x and y are guaranteed
- * not null, and there is no way to return null either. Do not return
- * INT_MIN, as callers are allowed to negate the result before using it.
+ * not null, and there is no way to return null either.
*
* This may be either the authoritative comparator, or the abbreviated
* comparator. Core code may switch this over the initial preference of
@@ -224,7 +223,7 @@ ApplySortComparator(Datum datum1, bool isNull1,
{
compare = ssup->comparator(datum1, datum2, ssup);
if (ssup->ssup_reverse)
- compare = -compare;
+ INVERT_SIGN(compare);
}
return compare;
@@ -262,7 +261,7 @@ ApplySortAbbrevFullComparator(Datum datum1, bool isNull1,
{
compare = ssup->abbrev_full_comparator(datum1, datum2, ssup);
if (ssup->ssup_reverse)
- compare = -compare;
+ INVERT_SIGN(compare);
}
return compare;
