8000 Fix assorted issues in convert_to_scalar(). · divag711/postgres@e2108f5 · GitHub
[go: up one dir, main page]
Skip to content

Commit e2108f5

Browse files
tglsfdctglsfdc
committed
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
1 parent 79300d0 commit e2108f5

File tree

4 files changed

+90
-57
lines changed

4 files changed

+90
-57
lines changed

‎contrib/btree_gist/btree_inet.c

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,11 @@ gbt_inet_compress(PG_FUNCTION_ARGS)
9999
if (entry->leafkey)
100100
{
101101
inetKEY *r = (inetKEY *) palloc(sizeof(inetKEY));
102+
bool failure = false;
102103

103104
retval = palloc(sizeof(GISTENTRY));
104-
r->lower = convert_network_to_scalar(entry->key, INETOID);
105+
r->lower = convert_network_to_scalar(entry->key, INETOID, &failure);
106+
Assert(!failure);
105107
r->upper = r->lower;
106108
gistentryinit(*retval, PointerGetDatum(r),
107109
entry->rel, entry->page,
@@ -118,13 +120,18 @@ Datum
118120
gbt_inet_consistent(PG_FUNCTION_ARGS)
119121
{
120122
GISTENTRY *entry = (GISTENTRY *) PG_GETARG_POINTER(0);
121-
double query = convert_network_to_scalar(PG_GETARG_DATUM(1), INETOID);
123+
Datum dquery = PG_GETARG_DATUM(1);
122124
StrategyNumber strategy = (StrategyNumber) PG_GETARG_UINT16(2);
123125

124126
/* Oid subtype = PG_GETARG_OID(3); */
125127
bool *recheck = (bool *) PG_GETARG_POINTER(4);
126128
inetKEY *kkk = (inetKEY *) DatumGetPointer(entry->key);
127129
GBT_NUMKEY_R key;
130+
double query;
131+
bool failure = false;
132+
133+
query = convert_network_to_scalar(dquery, INETOID, &failure);
134+
Assert(!failure);
128135

129136
/* All cases served by this function are inexact */
130137
*recheck = true;

‎src/backend/utils/adt/network.c

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -943,9 +943,12 @@ inet_merge(PG_FUNCTION_ARGS)
943943
* Convert a value of a network datatype to an approximate scalar value.
944944
* This is used for estimating selectivities of inequality operators
945945
* involving network types.
946+
*
947+
* On failure (e.g., unsupported typid), set *failure to true;
948+
* otherwise, that variable is not changed.
946949
*/
947950
double
948-
convert_network_to_scalar(Datum value, Oid typid)
951+
convert_network_to_scalar(Datum value, Oid typid, bool *failure)
949952
{
950953
switch (typid)
951954
{
@@ -972,8 +975,6 @@ convert_network_to_scalar(Datum value, Oid typid)
972975
res += ip_addr(ip)[i];
973976
}
974977
return res;
975-
976-
break;
977978
}
978979
case MACADDROID:
979980
{
@@ -987,11 +988,7 @@ convert_network_to_scalar(Datum value, Oid typid)
987988
}
988989
}
989990

990-
/*
991-
* Can't get here unless someone tries to use scalarltsel/scalargtsel on
992-
* an operator with one network and one non-network operand.
993-
*/
994-
elog(ERROR, "unsupported type: %u", typid);
991+
*failure = true;
995992
return 0;
996993
}
997994

‎src/backend/utils/adt/selfuncs.c

Lines changed: 75 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ static double eqjoinsel_semi(Oid operator,
167167
static bool convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
168168
Datum lobound, Datum hibound, Oid boundstypid,
169169
double *scaledlobound, double *scaledhibound);
170-
static double convert_numeric_to_scalar(Datum value, Oid typid);
170+
static double convert_numeric_to_scalar(Datum value, Oid typid, bool *failure);
171171
static void convert_string_to_scalar(char *value,
172172
double *scaledvalue,
173173
char *lobound,
@@ -184,8 +184,9 @@ static double convert_one_string_to_scalar(char *value,
184184
int rangelo, int rangehi);
185185
static double convert_one_bytea_to_scalar(unsigned char *value, int valuelen,
186186
int rangelo, int rangehi);
187-
static char *convert_string_datum(Datum value, Oid typid);
188-
static double convert_timevalue_to_scalar(Datum value, Oid typid);
187+
static char *convert_string_datum(Datum value, Oid typid, bool *failure);
188+
static double convert_timevalue_to_scalar(Datum value, Oid typid,
189+
bool *failure);
189190
static void examine_simple_variable(PlannerInfo *root, Var *var,
190191
VariableStatData *vardata);
191192
static bool get_variable_range(PlannerInfo *root, VariableStatData *vardata,
@@ -530,7 +531,8 @@ neqsel(PG_FUNCTION_ARGS)
530531
*
531532
* This routine works for any datatype (or pair of datatypes) known to
532533
* convert_to_scalar(). If it is applied to some other datatype,
533-
* it will return a default estimate.
534+
* it will return an approximate estimate based on assuming that the constant
535+
* value falls in the middle of the bin identified by binary search.
534536
*/
535537
static double
536538
scalarineqsel(PlannerInfo *root, Oid operator, bool isgt,
@@ -3707,10 +3709,15 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
37073709
Datum lobound, Datum hibound, Oid boundstypid,
37083710
double *scaledlobound, double *scaledhibound)
37093711
{
3712+
bool failure = false;
3713+
37103714
/*
37113715
* Both the valuetypid and the boundstypid should exactly match the
3712-
* declared input type(s) of the operator we are invoked for, so we just
3713-
* error out if either is not recognized.
3716+
* declared input type(s) of the operator we are invoked for. However,
3717+
* extensions might try to use scalarineqsel as estimator for operators
3718+
* with input type(s) we don't handle here; in such cases, we want to
3719+
* return false, not fail. In any case, we mustn't assume that valuetypid
3720+
* and boundstypid are identical.
37143721
*
37153722
* XXX The histogram we are interpolating between points of could belong
37163723
* 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,
37453752
case REGDICTIONARYOID:
37463753
case REGROLEOID:
37473754
case REGNAMESPACEOID:
3748-
*scaledvalue = convert_numeric_to_scalar(value, valuetypid);
3749-
*scaledlobound = convert_numeric_to_scalar(lobound, boundstypid);
3750-
*scaledhibound = convert_numeric_to_scalar(hibound, boundstypid);
3751-
return true;
3755+
*scaledvalue = convert_numeric_to_scalar(value, valuetypid,
3756+
&failure);
3757+
*scaledlobound = convert_numeric_to_scalar(lobound, boundstypid,
3758+
&failure);
3759+
*scaledhibound = convert_numeric_to_scalar(hibound, boundstypid,
3760+
&failure);
3761+
return !failure;
37523762

37533763
/*
37543764
* Built-in string types
@@ -3759,9 +3769,20 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
37593769
case TEXTOID:
37603770
case NAMEOID:
37613771
{
3762-
char *valstr = convert_string_datum(value, valuetypid);
3763-
char *lostr = convert_string_datum(lobound, boundstypid);
3764-
char *histr = convert_string_datum(hibound, boundstypid);
3772+
char *valstr = convert_string_datum(value, valuetypid,
3773+
&failure);
3774+
char *lostr = convert_string_datum(lobound, boundstypid,
3775+
&failure);
3776+
char *histr = convert_string_datum(hibound, boundstypid,
3777+
&failure);
3778+
3779+
/*
3780+
* Bail out if any of the values is not of string type. We
3781+
* might leak converted strings for the other value(s), but
3782+
* that's not worth troubling over.
3783+
*/
3784+
if (failure)
3785+
return false;
37653786

37663787
convert_string_to_scalar(valstr, scaledvalue,
37673788
lostr, scaledlobound,
@@ -3777,6 +3798,9 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
37773798
*/
37783799
case BYTEAOID:
37793800
{
3801+
/* We only support bytea vs bytea comparison */
3802+
if (boundstypid != BYTEAOID)
3803+
return false;
37803804
convert_bytea_to_scalar(value, scaledvalue,
37813805
lobound, scaledlobound,
37823806
hibound, scaledhibound);
@@ -3795,21 +3819,27 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
37953819
case TINTERVALOID:
37963820
case TIMEOID:
37973821
case TIMETZOID:
3798-
*scaledvalue = convert_timevalue_to_scalar(value, valuetypid);
3799-
*scaledlobound = convert_timevalue_to_scalar(lobound, boundstypid);
3800-
*scaledhibound = convert_timevalue_to_scalar(hibound, boundstypid);
3801-
return true;
3822+
*scaledvalue = convert_timevalue_to_scalar(value, valuetypid,
3823+
&failure);
3824+
*scaledlobound = convert_timevalue_to_scalar(lobound, boundstypid,
3825+
&failure);
3826+
*scaledhibound = convert_timevalue_to_scalar(hibound, boundstypid,
3827+
&failure);
3828+
return !failure;
38023829

38033830
/*
38043831
* Built-in network types
38053832
*/
38063833
case INETOID:
38073834
case CIDROID:
38083835
case MACADDROID:
3809-
*scaledvalue = convert_network_to_scalar(value, valuetypid);
3810-
*scaledlobound = convert_network_to_scalar(lobound, boundstypid);
3811-
*scaledhibound = convert_network_to_scalar(hibound, boundstypid);
3812-
return true;
3836+
*scaledvalue = convert_network_to_scalar(value, valuetypid,
3837+
&failure);
3838+
*scaledlobound = convert_network_to_scalar(lobound, boundstypid,
3839+
&failure);
3840+
*scaledhibound = convert_network_to_scalar(hibound, boundstypid,
3841+
&failure);
3842+
return !failure;
38133843
}
38143844
/* Don't know how to convert */
38153845
*scaledvalue = *scaledlobound = *scaledhibound = 0;
@@ -3818,9 +3848,12 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
38183848

38193849
/*
38203850
* Do convert_to_scalar()'s work for any numeric data type.
3851+
*
3852+
* On failure (e.g., unsupported typid), set *failure to true;
3853+
* otherwise, that variable is not changed.
38213854
*/
38223855
static double
3823-
convert_numeric_to_scalar(Datum value, Oid typid)
3856+
convert_numeric_to_scalar(Datum value, Oid typid, bool *failure)
38243857
{
38253858
switch (typid)
38263859
{
@@ -3856,11 +3889,7 @@ convert_numeric_to_scalar(Datum value, Oid typid)
38563889
return (double) DatumGetObjectId(value);
38573890
}
38583891

3859-
/*
3860-
* Can't get here unless someone tries to use scalarltsel/scalargtsel on
3861-
* an operator with one numeric and one non-numeric operand.
3862-
*/
3863-
elog(ERROR, "unsupported type: %u", typid);
3892+
*failure = true;
38643893
return 0;
38653894
}
38663895

@@ -4009,11 +4038,14 @@ convert_one_string_to_scalar(char *value, int rangelo, int rangehi)
40094038
/*
40104039
* Convert a string-type Datum into a palloc'd, null-terminated string.
40114040
*
4041+
* On failure (e.g., unsupported typid), set *failure to true;
4042+
* otherwise, that variable is not changed. (We'll return NULL on failure.)
4043+
*
40124044
* When using a non-C locale, we must pass the string through strxfrm()
40134045
* before continuing, so as to generate correct locale-specific results.
40144046
*/
40154047
static char *
4016-
convert_string_datum(Datum value, Oid typid)
4048+
convert_string_datum(Datum value, Oid typid, bool *failure)
40174049
{
40184050
char *val;
40194051

@@ -4037,12 +4069,7 @@ convert_string_datum(Datum value, Oid typid)
40374069
break;
40384070
}
40394071
default:
4040-
4041-
/*
4042-
* Can't get here unless someone tries to use scalarltsel on an
4043-
* operator with one string and one non-string operand.
4044-
*/
4045-
elog(ERROR, "unsupported type: %u", typid);
4072+
*failure = true;
40464073
return NULL;
40474074
}
40484075

@@ -4119,16 +4146,19 @@ convert_bytea_to_scalar(Datum value,
41194146
Datum hibound,
41204147
double *scaledhibound)
41214148
{
4149+
bytea *valuep = DatumGetByteaPP(value);
4150+
bytea *loboundp = DatumGetByteaPP(lobound);
4151+
bytea *hiboundp = DatumGetByteaPP(hibound);
41224152
int rangelo,
41234153
rangehi,
4124-
valuelen = VARSIZE(DatumGetPointer(value)) - VARHDRSZ,
4125-
loboundlen = VARSIZE(DatumGetPointer(lobound)) - VARHDRSZ,
4126-
hiboundlen = VARSIZE(DatumGetPointer(hibound)) - VARHDRSZ,
4154+
valuelen = VARSIZE_ANY_EXHDR(valuep),
4155+
loboundlen = VARSIZE_ANY_EXHDR(loboundp),
4156+
hiboundlen = VARSIZE_ANY_EXHDR(hiboundp),
41274157
i,
41284158
minlen;
4129-
unsigned char *valstr = (unsigned char *) VARDATA(DatumGetPointer(value)),
4130-
*lostr = (unsigned char *) VARDATA(DatumGetPointer(lobound)),
4131-
*histr = (unsigned char *) VARDATA(DatumGetPointer(hibound));
4159+
unsigned char *valstr = (unsigned char *) VARDATA_ANY(valuep);
4160+
unsigned char *lostr = (unsigned char *) VARDATA_ANY(loboundp);
4161+
unsigned char *histr = (unsigned char *) VARDATA_ANY(hiboundp);
41324162

41334163
/*
41344164
* Assume bytea data is uniformly distributed across all byte values.
@@ -4195,9 +4225,12 @@ convert_one_bytea_to_scalar(unsigned char *value, int valuelen,
41954225

41964226
/*
41974227
* Do convert_to_scalar()'s work for any timevalue data type.
4228+
*
4229+
* On failure (e.g., unsupported typid), set *failure to true;
4230+
* otherwise, that variable is not changed.
41984231
*/
41994232
static double
4200-
convert_timevalue_to_scalar(Datum value, Oid typid)
4233+
convert_timevalue_to_scalar(Datum value, Oid typid, bool *failure)
42014234
{
42024235
switch (typid)
42034236
{
@@ -4261,11 +4294,7 @@ convert_timevalue_to_scalar(Datum value, Oid typid)
42614294
}
42624295
}
42634296

4264-
/*
4265-
* Can't get here unless someone tries to use scalarltsel/scalargtsel on
4266-
* an operator with one timevalue and one non-timevalue operand.
4267-
*/
4268-
elog(ERROR, "unsupported type: %u", typid);
4297+
*failure = true;
42694298
return 0;
42704299
}
42714300

‎src/include/utils/builtins.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -982,7 +982,7 @@ extern Datum network_host(PG_FUNCTION_ARGS);
982982
extern Datum network_show(PG_FUNCTION_ARGS);
983983
extern Datum inet_abbrev(PG_FUNCTION_ARGS);
984984
extern Datum cidr_abbrev(PG_FUNCTION_ARGS);
985-
extern double convert_network_to_scalar(Datum value, Oid typid);
985+
extern double convert_network_to_scalar(Datum value, Oid typid, bool *failure);
986986
extern Datum inet_to_cidr(PG_FUNCTION_ARGS);
987987
extern Datum inet_set_masklen(PG_FUNCTION_ARGS);
988988
extern Datum cidr_set_masklen(PG_FUNCTION_ARGS);

0 commit comments

Comments
 (0)
0