Fix assorted issues in convert_to_scalar().

If convert_to_scalar is passed a pair of datatypes it can't cope with,
its former behavior was just to elog(ERROR).  While this is OK so far as
the core code is concerned, there's extension code that would like to use
scalarltsel/scalargtsel/etc as selectivity estimators for operators that
work on non-core datatypes, and this behavior is a show-stopper for that
use-case.  If we simply allow convert_to_scalar to return FALSE instead of
outright failing, then the main logic of scalarltsel/scalargtsel will work
fine for any operator that behaves like a scalar inequality comparison.
The lack of conversion capability will mean that we can't estimate to
better than histogram-bin-width precision, since the code will effectively
assume that the comparison constant falls at the middle of its bin.  But
that's still a lot better than nothing.  (Someday we should provide a way
for extension code to supply a custom version of convert_to_scalar, but
today is not that day.)

While poking at this issue, we noted that the existing code for handling
type bytea in convert_to_scalar is several bricks shy of a load.
It assumes without checking that if the comparison value is type bytea,
the bounds values are too; in the worst case this could lead to a crash.
It also fails to detoast the input values, so that the comparison result is
complete garbage if any input is toasted out-of-line, compressed, or even
just short-header.  I'm not sure how often such cases actually occur ---
the bounds values, at least, are probably safe since they are elements of
an array and hence can't be toasted.  But that doesn't make this code OK.

Back-patch to all supported branches, partly because author requested that,
but mostly because of the bytea bugs.  The change in API for the exposed
routine convert_network_to_scalar() is theoretically a back-patch hazard,
but it seems pretty unlikely that any third-party code is calling that
function directly.

Tomas Vondra, with some adjustments by me

Discussion: https://postgr.es/m/b68441b6-d18f-13ab-b43b-9a72188a4e02@2ndquadrant.com

diff --git a/contrib/btree_gist/btree_inet.c b/contrib/btree_gist/btree_inet.c
index 822786125d99b12cef2aa5cb8f9db88c120e811a..356653fa4c2a220c85bdf03efa5d9b0abe152a49 100644 (file)
--- a/contrib/btree_gist/btree_inet.c
+++ b/contrib/btree_gist/btree_inet.c
@@ -99,9 +99,11 @@ gbt_inet_compress(PG_FUNCTION_ARGS)
        if (entry->leafkey)
        {
                inetKEY    *r = (inetKEY *) palloc(sizeof(inetKEY));
+               bool            failure = false;
 
                retval = palloc(sizeof(GISTENTRY));
-               r->lower = convert_network_to_scalar(entry->key, INETOID);
+               r->lower = convert_network_to_scalar(entry->key, INETOID, &failure);
+               Assert(!failure);
                r->upper = r->lower;
                gistentryinit(*retval, PointerGetDatum(r),
                                          entry->rel, entry->page,
@@ -118,13 +120,18 @@ Datum
 gbt_inet_consistent(PG_FUNCTION_ARGS)
 {
        GISTENTRY  *entry = (GISTENTRY *) PG_GETARG_POINTER(0);
-       double          query = convert_network_to_scalar(PG_GETARG_DATUM(1), INETOID);
+       Datum           dquery = PG_GETARG_DATUM(1);
        StrategyNumber strategy = (StrategyNumber) PG_GETARG_UINT16(2);
 
        /* Oid          subtype = PG_GETARG_OID(3); */
        bool       *recheck = (bool *) PG_GETARG_POINTER(4);
        inetKEY    *kkk = (inetKEY *) DatumGetPointer(entry->key);
        GBT_NUMKEY_R key;
+       double          query;
+       bool            failure = false;
+
+       query = convert_network_to_scalar(dquery, INETOID, &failure);
+       Assert(!failure);
 
        /* All cases served by this function are inexact */
        *recheck = true;

diff --git a/src/backend/utils/adt/network.c b/src/backend/utils/adt/network.c
index 1f8469a2cbcd222ed08f21ff29f1e7e78b778267..d39017da75df5afdf3975f4866db0b11050297ac 100644 (file)
--- a/src/backend/utils/adt/network.c
+++ b/src/backend/utils/adt/network.c
@@ -943,9 +943,12 @@ inet_merge(PG_FUNCTION_ARGS)
  * Convert a value of a network datatype to an approximate scalar value.
  * This is used for estimating selectivities of inequality operators
  * involving network types.
+ *
+ * On failure (e.g., unsupported typid), set *failure to true;
+ * otherwise, that variable is not changed.
  */
 double
-convert_network_to_scalar(Datum value, Oid typid)
+convert_network_to_scalar(Datum value, Oid typid, bool *failure)
 {
        switch (typid)
        {
@@ -972,8 +975,6 @@ convert_network_to_scalar(Datum value, Oid typid)
                                        res += ip_addr(ip)[i];
                                }
                                return res;
-
-                               break;
                        }
                case MACADDROID:
                        {
@@ -987,11 +988,7 @@ convert_network_to_scalar(Datum value, Oid typid)
                        }
        }
 
-       /*
-        * Can't get here unless someone tries to use scalarltsel/scalargtsel on
-        * an operator with one network and one non-network operand.
-        */
-       elog(ERROR, "unsupported type: %u", typid);
+       *failure = true;
        return 0;
 }
 

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 6a4f7b19ebaa875d40abb8e80ba7080b71dc41f5..4dc54eaf0e4922450af58b52c0e19e6f8d95be38 100644 (file)
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -167,7 +167,7 @@ static double eqjoinsel_semi(Oid operator,
 static bool convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
                                  Datum lobound, Datum hibound, Oid boundstypid,
                                  double *scaledlobound, double *scaledhibound);
-static double convert_numeric_to_scalar(Datum value, Oid typid);
+static double convert_numeric_to_scalar(Datum value, Oid typid, bool *failure);
 static void convert_string_to_scalar(char *value,
                                                 double *scaledvalue,
                                                 char *lobound,
@@ -184,8 +184,9 @@ static double convert_one_string_to_scalar(char *value,
                                                         int rangelo, int rangehi);
 static double convert_one_bytea_to_scalar(unsigned char *value, int valuelen,
                                                        int rangelo, int rangehi);
-static char *convert_string_datum(Datum value, Oid typid);
-static double convert_timevalue_to_scalar(Datum value, Oid typid);
+static char *convert_string_datum(Datum value, Oid typid, bool *failure);
+static double convert_timevalue_to_scalar(Datum value, Oid typid,
+                                                       bool *failure);
 static void examine_simple_variable(PlannerInfo *root, Var *var,
                                                VariableStatData *vardata);
 static bool get_variable_range(PlannerInfo *root, VariableStatData *vardata,
@@ -530,7 +531,8 @@ neqsel(PG_FUNCTION_ARGS)
  *
  * This routine works for any datatype (or pair of datatypes) known to
  * convert_to_scalar().  If it is applied to some other datatype,
- * it will return a default estimate.
+ * it will return an approximate estimate based on assuming that the constant
+ * value falls in the middle of the bin identified by binary search.
  */
 static double
 scalarineqsel(PlannerInfo *root, Oid operator, bool isgt,
@@ -3707,10 +3709,15 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
                                  Datum lobound, Datum hibound, Oid boundstypid,
                                  double *scaledlobound, double *scaledhibound)
 {
+       bool            failure = false;
+
        /*
         * Both the valuetypid and the boundstypid should exactly match the
-        * declared input type(s) of the operator we are invoked for, so we just
-        * error out if either is not recognized.
+        * declared input type(s) of the operator we are invoked for.  However,
+        * extensions might try to use scalarineqsel as estimator for operators
+        * with input type(s) we don't handle here; in such cases, we want to
+        * return false, not fail.  In any case, we mustn't assume that valuetypid
+        * and boundstypid are identical.
         *
         * XXX The histogram we are interpolating between points of could belong
         * to a column that's only binary-compatible with the declared type. In
@@ -3745,10 +3752,13 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
                case REGDICTIONARYOID:
                case REGROLEOID:
                case REGNAMESPACEOID:
-                       *scaledvalue = convert_numeric_to_scalar(value, valuetypid);
-                       *scaledlobound = convert_numeric_to_scalar(lobound, boundstypid);
-                       *scaledhibound = convert_numeric_to_scalar(hibound, boundstypid);
-                       return true;
+                       *scaledvalue = convert_numeric_to_scalar(value, valuetypid,
+                                                                                                        &failure);
+                       *scaledlobound = convert_numeric_to_scalar(lobound, boundstypid,
+                                                                                                          &failure);
+                       *scaledhibound = convert_numeric_to_scalar(hibound, boundstypid,
+                                                                                                          &failure);
+                       return !failure;
 
                        /*
                         * Built-in string types
@@ -3759,9 +3769,20 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
                case TEXTOID:
                case NAMEOID:
                        {
-                               char       *valstr = convert_string_datum(value, valuetypid);
-                               char       *lostr = convert_string_datum(lobound, boundstypid);
-                               char       *histr = convert_string_datum(hibound, boundstypid);
+                               char       *valstr = convert_string_datum(value, valuetypid,
+                                                                                                                 &failure);
+                               char       *lostr = convert_string_datum(lobound, boundstypid,
+                                                                                                                &failure);
+                               char       *histr = convert_string_datum(hibound, boundstypid,
+                                                                                                                &failure);
+
+                               /*
+                                * Bail out if any of the values is not of string type.  We
+                                * might leak converted strings for the other value(s), but
+                                * that's not worth troubling over.
+                                */
+                               if (failure)
+                                       return false;
 
                                convert_string_to_scalar(valstr, scaledvalue,
                                                                                 lostr, scaledlobound,
@@ -3777,6 +3798,9 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
                         */
                case BYTEAOID:
                        {
+                               /* We only support bytea vs bytea comparison */
+                               if (boundstypid != BYTEAOID)
+                                       return false;
                                convert_bytea_to_scalar(value, scaledvalue,
                                                                                lobound, scaledlobound,
                                                                                hibound, scaledhibound);
@@ -3795,10 +3819,13 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
                case TINTERVALOID:
                case TIMEOID:
                case TIMETZOID:
-                       *scaledvalue = convert_timevalue_to_scalar(value, valuetypid);
-                       *scaledlobound = convert_timevalue_to_scalar(lobound, boundstypid);
-                       *scaledhibound = convert_timevalue_to_scalar(hibound, boundstypid);
-                       return true;
+                       *scaledvalue = convert_timevalue_to_scalar(value, valuetypid,
+                                                                                                          &failure);
+                       *scaledlobound = convert_timevalue_to_scalar(lobound, boundstypid,
+                                                                                                                &failure);
+                       *scaledhibound = convert_timevalue_to_scalar(hibound, boundstypid,
+                                                                                                                &failure);
+                       return !failure;
 
                        /*
                         * Built-in network types
@@ -3806,10 +3833,13 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
                case INETOID:
                case CIDROID:
                case MACADDROID:
-                       *scaledvalue = convert_network_to_scalar(value, valuetypid);
-                       *scaledlobound = convert_network_to_scalar(lobound, boundstypid);
-                       *scaledhibound = convert_network_to_scalar(hibound, boundstypid);
-                       return true;
+                       *scaledvalue = convert_network_to_scalar(value, valuetypid,
+                                                                                                        &failure);
+                       *scaledlobound = convert_network_to_scalar(lobound, boundstypid,
+                                                                                                          &failure);
+                       *scaledhibound = convert_network_to_scalar(hibound, boundstypid,
+                                                                                                          &failure);
+                       return !failure;
        }
        /* Don't know how to convert */
        *scaledvalue = *scaledlobound = *scaledhibound = 0;
@@ -3818,9 +3848,12 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
 
 /*
  * Do convert_to_scalar()'s work for any numeric data type.
+ *
+ * On failure (e.g., unsupported typid), set *failure to true;
+ * otherwise, that variable is not changed.
  */
 static double
-convert_numeric_to_scalar(Datum value, Oid typid)
+convert_numeric_to_scalar(Datum value, Oid typid, bool *failure)
 {
        switch (typid)
        {
@@ -3856,11 +3889,7 @@ convert_numeric_to_scalar(Datum value, Oid typid)
                        return (double) DatumGetObjectId(value);
        }
 
-       /*
-        * Can't get here unless someone tries to use scalarltsel/scalargtsel on
-        * an operator with one numeric and one non-numeric operand.
-        */
-       elog(ERROR, "unsupported type: %u", typid);
+       *failure = true;
        return 0;
 }
 
@@ -4009,11 +4038,14 @@ convert_one_string_to_scalar(char *value, int rangelo, int rangehi)
 /*
  * Convert a string-type Datum into a palloc'd, null-terminated string.
  *
+ * On failure (e.g., unsupported typid), set *failure to true;
+ * otherwise, that variable is not changed.  (We'll return NULL on failure.)
+ *
  * When using a non-C locale, we must pass the string through strxfrm()
  * before continuing, so as to generate correct locale-specific results.
  */
 static char *
-convert_string_datum(Datum value, Oid typid)
+convert_string_datum(Datum value, Oid typid, bool *failure)
 {
        char       *val;
 
@@ -4037,12 +4069,7 @@ convert_string_datum(Datum value, Oid typid)
                                break;
                        }
                default:
-
-                       /*
-                        * Can't get here unless someone tries to use scalarltsel on an
-                        * operator with one string and one non-string operand.
-                        */
-                       elog(ERROR, "unsupported type: %u", typid);
+                       *failure = true;
                        return NULL;
        }
 
@@ -4119,16 +4146,19 @@ convert_bytea_to_scalar(Datum value,
                                                Datum hibound,
                                                double *scaledhibound)
 {
+       bytea      *valuep = DatumGetByteaPP(value);
+       bytea      *loboundp = DatumGetByteaPP(lobound);
+       bytea      *hiboundp = DatumGetByteaPP(hibound);
        int                     rangelo,
                                rangehi,
-                               valuelen = VARSIZE(DatumGetPointer(value)) - VARHDRSZ,
-                               loboundlen = VARSIZE(DatumGetPointer(lobound)) - VARHDRSZ,
-                               hiboundlen = VARSIZE(DatumGetPointer(hibound)) - VARHDRSZ,
+                               valuelen = VARSIZE_ANY_EXHDR(valuep),
+                               loboundlen = VARSIZE_ANY_EXHDR(loboundp),
+                               hiboundlen = VARSIZE_ANY_EXHDR(hiboundp),
                                i,
                                minlen;
-       unsigned char *valstr = (unsigned char *) VARDATA(DatumGetPointer(value)),
-                          *lostr = (unsigned char *) VARDATA(DatumGetPointer(lobound)),
-                          *histr = (unsigned char *) VARDATA(DatumGetPointer(hibound));
+       unsigned char *valstr = (unsigned char *) VARDATA_ANY(valuep);
+       unsigned char *lostr = (unsigned char *) VARDATA_ANY(loboundp);
+       unsigned char *histr = (unsigned char *) VARDATA_ANY(hiboundp);
 
        /*
         * Assume bytea data is uniformly distributed across all byte values.
@@ -4195,9 +4225,12 @@ convert_one_bytea_to_scalar(unsigned char *value, int valuelen,
 
 /*
  * Do convert_to_scalar()'s work for any timevalue data type.
+ *
+ * On failure (e.g., unsupported typid), set *failure to true;
+ * otherwise, that variable is not changed.
  */
 static double
-convert_timevalue_to_scalar(Datum value, Oid typid)
+convert_timevalue_to_scalar(Datum value, Oid typid, bool *failure)
 {
        switch (typid)
        {
@@ -4261,11 +4294,7 @@ convert_timevalue_to_scalar(Datum value, Oid typid)
                        }
        }
 
-       /*
-        * Can't get here unless someone tries to use scalarltsel/scalargtsel on
-        * an operator with one timevalue and one non-timevalue operand.
-        */
-       elog(ERROR, "unsupported type: %u", typid);
+       *failure = true;
        return 0;
 }
 

diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index a91be981b9873bd114c16d1459b7df7be02c663e..b6711df31d47d2c85cf4688561de6a0162318268 100644 (file)
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -982,7 +982,7 @@ extern Datum network_host(PG_FUNCTION_ARGS);
 extern Datum network_show(PG_FUNCTION_ARGS);
 extern Datum inet_abbrev(PG_FUNCTION_ARGS);
 extern Datum cidr_abbrev(PG_FUNCTION_ARGS);
-extern double convert_network_to_scalar(Datum value, Oid typid);
+extern double convert_network_to_scalar(Datum value, Oid typid, bool *failure);
 extern Datum inet_to_cidr(PG_FUNCTION_ARGS);
 extern Datum inet_set_masklen(PG_FUNCTION_ARGS);
 extern Datum cidr_set_masklen(PG_FUNCTION_ARGS);