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 c136296ab536e2262628fb182eab9d3debe960d0..ec66d029ce75af2dac6c3d50b349cfa862af0451 100644 (file)
--- a/contrib/btree_gist/btree_inet.c
+++ b/contrib/btree_gist/btree_inet.c
@@ -105,9 +105,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,
@@ -124,13 +126,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 737eeff86da0ca179cc377681a87c9f13a3ac43e..b895368d73301a20d024873b4b8561b18d59ebb4 100644 (file)
--- a/src/backend/utils/adt/network.c
+++ b/src/backend/utils/adt/network.c
@@ -898,9 +898,12 @@ network_hostmask(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)
        {
@@ -927,8 +930,6 @@ convert_network_to_scalar(Datum value, Oid typid)
                                        res += ip_addr(ip)[i];
                                }
                                return res;
-
-                               break;
                        }
                case MACADDROID:
                        {
@@ -942,11 +943,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 da963689c56de77cb4752fd3486b5dec33681191..019d4b619e0be4da2299bda6edd5f4d42ef83e5f 100644 (file)
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -164,7 +164,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,
@@ -181,8 +181,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,
@@ -527,7 +528,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,
@@ -3605,10 +3607,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
@@ -3641,10 +3648,13 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
                case REGTYPEOID:
                case REGCONFIGOID:
                case REGDICTIONARYOID:
-                       *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
@@ -3655,9 +3665,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,
@@ -3673,6 +3694,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);
@@ -3691,10 +3715,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
@@ -3702,10 +3729,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;
@@ -3714,9 +3744,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)
        {
@@ -3750,11 +3783,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;
 }
 
@@ -3897,11 +3926,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;
 
@@ -3925,12 +3957,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;
        }
 
@@ -4010,16 +4037,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.
@@ -4086,9 +4116,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)
        {
@@ -4152,11 +4185,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 1290438df7c1ff5efe92f741c882eb78ab915f0a..03d93d18fbf82e743587f63f230b086f8d4df59a 100644 (file)
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -891,7 +891,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);