Re: Odd 9.4, 9.3 buildfarm failure on s390x - Mailing list pgsql-hackers

I wrote:
> Andres Freund <andres@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;