Allow btree comparison functions to return INT_MIN.

Historically we forbade datatype-specific comparison functions from
returning INT_MIN, so that it would be safe to invert the sort order
just by negating the comparison result.  However, this was never
really safe for comparison functions that directly return the result
of memcmp(), strcmp(), etc, as POSIX doesn't place any such restriction
on those library functions.  Buildfarm results show that at least on
recent Linux on s390x, memcmp() actually does return INT_MIN sometimes,
causing sort failures.

The agreed-on answer is to remove this restriction and fix relevant
call sites to not make such an assumption; code such as "res = -res"
should be replaced by "INVERT_COMPARE_RESULT(res)".  The same is needed
in a few places that just directly negated the result of memcmp or
strcmp.

To help find places having this problem, I've also added a compile option
to nbtcompare.c that causes some of the commonly used comparators to
return INT_MIN/INT_MAX instead of their usual -1/+1.  It'd likely be
a good idea to have at least one buildfarm member running with
"-DSTRESS_SORT_INT_MIN".  That's far from a complete test of course,
but it should help to prevent fresh introductions of such bugs.

This is a longstanding portability hazard, so back-patch to all supported
branches.

Discussion: https://postgr.es/m/20180928185215.ffoq2xrq5d3pafna@alap3.anarazel.de

3 files changed, 11 insertions, 5 deletions

diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index 09708c01c81..61f4574948d 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -449,8 +449,7 @@ typedef struct xl_btree_newroot
  *	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 6836b0a0f56..aad18a7c651 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -868,6 +868,14 @@ typedef NameData *Name;
  */
 
 /*
+ * 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_COMPARE_RESULT(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 8b6b0de2e8b..2e4dba334d2 100644
--- a/src/include/utils/sortsupport.h
+++ b/src/include/utils/sortsupport.h
@@ -90,8 +90,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.
 	 */
 	int			(*comparator) (Datum x, Datum y, SortSupport ssup);
 
@@ -142,7 +141,7 @@ ApplySortComparator(Datum datum1, bool isNull1,
 	{
 		compare = (*ssup->comparator) (datum1, datum2, ssup);
 		if (ssup->ssup_reverse)
-			compare = -compare;
+			INVERT_COMPARE_RESULT(compare);
 	}
 
 	return compare;