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

I wrote:
> 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 thought of another, uglier way to stress things: make wrappers around
memcmp, strcmp, and strncmp to force those to return INT_MIN/0/INT_MAX,
thereby modeling what we see is happening on Mark's machine.

This successfully exposed the bug I'd already found by grepping in
pg_rewind/filemap.c, as well as some astonishingly unportable code
in contrib/ltree.

The attached update incorporates Thomas' suggestion for the macro
name, as well as the ltree fix.  For completeness, I also show the
very quick-hacky way I changed memcmp et al, but I don't propose
committing that.

I'm inclined to just go ahead and apply/backpatch this.  It's certainly
possible that more bugs remain to be found, but I have no good ideas
about how to search for them, and in any case that wouldn't invalidate
the patch as it stands.

            regards, tom lane

diff --git a/contrib/ltree/ltree_op.c b/contrib/ltree/ltree_op.c
index 759f1f8..df61c63 100644
--- a/contrib/ltree/ltree_op.c
+++ b/contrib/ltree/ltree_op.c
@@ -45,17 +45,24 @@ ltree_compare(const ltree *a, const ltree *b)
     ltree_level *bl = LTREE_FIRST(b);
     int            an = a->numlevel;
     int            bn = b->numlevel;
-    int            res = 0;

     while (an > 0 && bn > 0)
     {
+        int            res;
+
         if ((res = memcmp(al->name, bl->name, Min(al->len, bl->len))) == 0)
         {
             if (al->len != bl->len)
                 return (al->len - bl->len) * 10 * (an + 1);
         }
         else
+        {
+            if (res < 0)
+                res = -1;
+            else
+                res = 1;
             return res * 10 * (an + 1);
+        }

         an--;
         bn--;
@@ -146,7 +153,7 @@ inner_isparent(const ltree *c, const ltree *p)
     {
         if (cl->len != pl->len)
             return false;
-        if (memcmp(cl->name, pl->name, cl->len))
+        if (memcmp(cl->name, pl->name, cl->len) != 0)
             return false;

         pn--;
diff --git a/contrib/pgcrypto/imath.c b/contrib/pgcrypto/imath.c
index cd528bf..b94a51b 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_COMPARE_RESULT(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_COMPARE_RESULT(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..8b2772c 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_COMPARE_RESULT(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..205457e 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_COMPARE_RESULT(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_COMPARE_RESULT(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..6362e44 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_COMPARE_RESULT(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..f8aa50f 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_COMPARE_RESULT(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..86fbb63 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_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 53b692e..818e0b1 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_COMPARE_RESULT(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_COMPARE_RESULT(compare);
     }

     return compare;
diff --git a/src/include/port.h b/src/include/port.h
index e654d5c..e5eda63 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -196,6 +196,17 @@ extern char *pg_strerror_r(int errnum, char *buf, size_t buflen);
 #define strerror_r pg_strerror_r
 #define PG_STRERROR_R_BUFLEN 256    /* Recommended buffer size for strerror_r */

+/* Override these too for sort testing */
+extern int pg_memcmp(const void *s1, const void *s2, size_t n);
+extern int pg_strcmp(const char *s1, const char *s2);
+extern int pg_strncmp(const char *s1, const char *s2, size_t n);
+#undef memcmp
+#undef strcmp
+#undef strncmp
+#define memcmp pg_memcmp
+#define strcmp pg_strcmp
+#define strncmp pg_strncmp
+
 /* Portable prompt handling */
 extern void simple_prompt(const char *prompt, char *destination, size_t destlen,
               bool echo);
diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index ef496fa..be404b1 100644
--- a/src/port/snprintf.c
+++ b/src/port/snprintf.c
@@ -1357,3 +1357,40 @@ trailing_pad(int padlen, PrintfTarget *target)
     if (padlen < 0)
         dopr_outchmulti(' ', -padlen, target);
 }
+
+#undef memcmp
+#undef strcmp
+#undef strncmp
+
+int pg_memcmp(const void *s1, const void *s2, size_t n)
+{
+    int res = memcmp(s1, s2, n);
+
+    if (res < 0)
+        return INT_MIN;
+    if (res > 0)
+        return INT_MAX;
+    return 0;
+}
+
+int pg_strcmp(const char *s1, const char *s2)
+{
+    int res = strcmp(s1, s2);
+
+    if (res < 0)
+        return INT_MIN;
+    if (res > 0)
+        return INT_MAX;
+    return 0;
+}
+
+int pg_strncmp(const char *s1, const char *s2, size_t n)
+{
+    int res = strncmp(s1, s2, n);
+
+    if (res < 0)
+        return INT_MIN;
+    if (res > 0)
+        return INT_MAX;
+    return 0;
+}