Skip to content

Commit f3b0897

Browse files
robertmhaasrobertmhaas
committed
Fix multiple problems with satisfies_hash_partition.
Fix the function header comment to describe the actual behavior. Check that table OID, modulus, and remainder arguments are not NULL before accessing them. Check that the modulus and remainder are sensible. If the table OID doesn't exist, return NULL instead of emitting an internal error, similar to what we do elsewhere. Check that the actual argument types match, or at least are binary coercible to, the expected argument types. Correctly handle invocation of this function using the VARIADIC syntax. Add regression tests. Robert Haas and Amul Sul, per a report by Andreas Seltenreich and subsequent followup investigation. Discussion: http://postgr.es/m/871sl4sdrv.fsf@ansel.ydns.eu
1 parent de1d042 commit f3b0897

File tree

5 files changed

+377
-31
lines changed

5 files changed

+377
-31
lines changed

‎src/backend/catalog/partition.c

Lines changed: 172 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
#include "optimizer/planmain.h"
4141
#include "optimizer/prep.h"
4242
#include "optimizer/var.h"
43+
#include "parser/parse_coerce.h"
4344
#include "rewrite/rewriteManip.h"
4445
#include "storage/lmgr.h"
4546
#include "utils/array.h"
@@ -3085,9 +3086,11 @@ compute_hash_value(PartitionKey key, Datum *values, bool *isnull)
30853086
/*
30863087
* satisfies_hash_partition
30873088
*
3088-
* This is a SQL-callable function for use in hash partition constraints takes
3089-
* an already computed hash values of each partition key attribute, and combine
3090-
* them into a single hash value by calling hash_combine64.
3089+
* This is an SQL-callable function for use in hash partition constraints.
3090+
* The first three arguments are the parent table OID, modulus, and remainder.
3091+
* The remaining arguments are the value of the partitioning columns (or
3092+
* expressions); these are hashed and the results are combined into a single
3093+
* hash value by calling hash_combine64.
30913094
*
30923095
* Returns true if remainder produced when this computed single hash value is
30933096
* divided by the given modulus is equal to given remainder, otherwise false.
@@ -3100,60 +3103,160 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
31003103
typedef struct ColumnsHashData
31013104
{
31023105
Oid relid;
3103-
int16 nkeys;
3106+
int nkeys;
3107+
Oid variadic_type;
3108+
int16 variadic_typlen;
3109+
bool variadic_typbyval;
3110+
char variadic_typalign;
31043111
FmgrInfo partsupfunc[PARTITION_MAX_KEYS];
31053112
} ColumnsHashData;
3106-
Oid parentId = PG_GETARG_OID(0);
3107-
int modulus = PG_GETARG_INT32(1);
3108-
int remainder = PG_GETARG_INT32(2);
3109-
short nkeys = PG_NARGS() - 3;
3110-
int i;
3113+
Oid parentId;
3114+
int modulus;
3115+
int remainder;
31113116
Datum seed = UInt64GetDatum(HASH_PARTITION_SEED);
31123117
ColumnsHashData *my_extra;
31133118
uint64 rowHash = 0;
31143119

3120+
/* Return null if the parent OID, modulus, or remainder is NULL. */
3121+
if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2))
3122+
PG_RETURN_NULL();
3123+
parentId = PG_GETARG_OID(0);
3124+
modulus = PG_GETARG_INT32(1);
3125+
remainder = PG_GETARG_INT32(2);
3126+
3127+
/* Sanity check modulus and remainder. */
3128+
if (modulus <= 0)
3129+
ereport(ERROR,
3130+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
3131+
errmsg("modulus for hash partition must be a positive integer")));
3132+
if (remainder < 0)
3133+
ereport(ERROR,
3134+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
3135+
errmsg("remainder for hash partition must be a non-negative integer")));
3136+
if (remainder >= modulus)
3137+
ereport(ERROR,
3138+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
3139+
errmsg("remainder for hash partition must be less than modulus")));
3140+
31153141
/*
31163142
* Cache hash function information.
31173143
*/
31183144
my_extra = (ColumnsHashData *) fcinfo->flinfo->fn_extra;
3119-
if (my_extra == NULL || my_extra->nkeys != nkeys ||
3120-
my_extra->relid != parentId)
3145+
if (my_extra == NULL || my_extra->relid != parentId)
31213146
{
31223147
Relation parent;
31233148
PartitionKey key;
3124-
int j;
3125-
3126-
fcinfo->flinfo->fn_extra =
3127-
MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
3128-
offsetof(ColumnsHashData, partsupfunc) +
3129-
sizeof(FmgrInfo) * nkeys);
3130-
my_extra = (ColumnsHashData *) fcinfo->flinfo->fn_extra;
3131-
my_extra->nkeys = nkeys;
3132-
my_extra->relid = parentId;
3149+
int j;
31333150

31343151
/* Open parent relation and fetch partition keyinfo */
3135-
parent = heap_open(parentId, AccessShareLock);
3152+
parent = try_relation_open(parentId, AccessShareLock);
3153+
if (parent == NULL)
3154+
PG_RETURN_NULL();
31363155
key = RelationGetPartitionKey(parent);
31373156

3138-
Assert(key->partnatts == nkeys);
3139-
for (j = 0; j < nkeys; ++j)
3140-
fmgr_info_copy(&my_extra->partsupfunc[j],
3141-
key->partsupfunc,
3157+
/* Reject parent table that is not hash-partitioned. */
3158+
if (parent->rd_rel->relkind != RELKIND_PARTITIONED_TABLE ||
3159+
key->strategy != PARTITION_STRATEGY_HASH)
3160+
ereport(ERROR,
3161+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
3162+
errmsg("\"%s\" is not a hash partitioned table",
3163+
get_rel_name(parentId))));
3164+
3165+
if (!get_fn_expr_variadic(fcinfo->flinfo))
3166+
{
3167+
int nargs = PG_NARGS() - 3;
3168+
3169+
/* complain if wrong number of column values */
3170+
if (key->partnatts != nargs)
3171+
ereport(ERROR,
3172+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
3173+
errmsg("number of partitioning columns (%d) does not match number of partition keys provided (%d)",
3174+
key->partnatts, nargs)));
3175+
3176+
/* allocate space for our cache */
3177+
fcinfo->flinfo->fn_extra =
3178+
MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
3179+
offsetof(ColumnsHashData, partsupfunc) +
3180+
sizeof(FmgrInfo) * nargs);
3181+
my_extra = (ColumnsHashData *) fcinfo->flinfo->fn_extra;
3182+
my_extra->relid = parentId;
3183+
my_extra->nkeys = key->partnatts;
3184+
3185+
/* check argument types and save fmgr_infos */
3186+
for (j = 0; j < key->partnatts; ++j)
3187+
{
3188+
Oid argtype = get_fn_expr_argtype(fcinfo->flinfo, j + 3);
3189+
3190+
if (argtype != key->parttypid[j] && !IsBinaryCoercible(argtype, key->parttypid[j]))
3191+
ereport(ERROR,
3192+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
3193+
errmsg("column %d of the partition key has type \"%s\", but supplied value is of type \"%s\"",
3194+
j + 1, format_type_be(key->parttypid[j]), format_type_be(argtype))));
3195+
3196+
fmgr_info_copy(&my_extra->partsupfunc[j],
3197+
&key->partsupfunc[j],
3198+
fcinfo->flinfo->fn_mcxt);
3199+
}
3200+
3201+
}
3202+
else
3203+
{
3204+
ArrayType *variadic_array = PG_GETARG_ARRAYTYPE_P(3);
3205+
3206+
/* allocate space for our cache -- just one FmgrInfo in this case */
3207+
fcinfo->flinfo->fn_extra =
3208+
MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
3209+
offsetof(ColumnsHashData, partsupfunc) +
3210+
sizeof(FmgrInfo));
3211+
my_extra = (ColumnsHashData *) fcinfo->flinfo->fn_extra;
3212+
my_extra->relid = parentId;
3213+
my_extra->nkeys = key->partnatts;
3214+
my_extra->variadic_type = ARR_ELEMTYPE(variadic_array);
3215+
get_typlenbyvalalign(my_extra->variadic_type,
3216+
&my_extra->variadic_typlen,
3217+
&my_extra->variadic_typbyval,
3218+
&my_extra->variadic_typalign);
3219+
3220+
/* check argument types */
3221+
for (j = 0; j < key->partnatts; ++j)
3222+
if (key->parttypid[j] != my_extra->variadic_type)
3223+
ereport(ERROR,
3224+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
3225+
errmsg("column %d of the partition key has type \"%s\", but supplied value is of type \"%s\"",
3226+
j + 1,
3227+
format_type_be(key->parttypid[j]),
3228+
format_type_be(my_extra->variadic_type))));
3229+
3230+
fmgr_info_copy(&my_extra->partsupfunc[0],
3231+
&key->partsupfunc[0],
31423232
fcinfo->flinfo->fn_mcxt);
3233+
}
31433234

31443235
/* Hold lock until commit */
3145-
heap_close(parent, NoLock);
3236+
relation_close(parent, NoLock);
31463237
}
31473238

3148-
for (i = 0; i < nkeys; i++)
3239+
if (!OidIsValid(my_extra->variadic_type))
31493240
{
3150-
/* keys start from fourth argument of function. */
3151-
int argno = i + 3;
3241+
int nkeys = my_extra->nkeys;
3242+
int i;
3243+
3244+
/*
3245+
* For a non-variadic call, neither the number of arguments nor their
3246+
* types can change across calls, so avoid the expense of rechecking
3247+
* here.
3248+
*/
31523249

3153-
if (!PG_ARGISNULL(argno))
3250+
for (i = 0; i < nkeys; i++)
31543251
{
31553252
Datum hash;
31563253

3254+
/* keys start from fourth argument of function. */
3255+
int argno = i + 3;
3256+
3257+
if (PG_ARGISNULL(argno))
3258+
continue;
3259+
31573260
Assert(OidIsValid(my_extra->partsupfunc[i].fn_oid));
31583261

31593262
hash = FunctionCall2(&my_extra->partsupfunc[i],
@@ -3164,6 +3267,45 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
31643267
rowHash = hash_combine64(rowHash, DatumGetUInt64(hash));
31653268
}
31663269
}
3270+
else
3271+
{
3272+
ArrayType *variadic_array = PG_GETARG_ARRAYTYPE_P(3);
3273+
int i;
3274+
int nelems;
3275+
Datum *datum;
3276+
bool *isnull;
3277+
3278+
deconstruct_array(variadic_array,
3279+
my_extra->variadic_type,
3280+
my_extra->variadic_typlen,
3281+
my_extra->variadic_typbyval,
3282+
my_extra->variadic_typalign,
3283+
&datum, &isnull, &nelems);
3284+
3285+
/* complain if wrong number of column values */
3286+
if (nelems != my_extra->nkeys)
3287+
ereport(ERROR,
3288+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
3289+
errmsg("number of partitioning columns (%d) does not match number of partition keys provided (%d)",
3290+
my_extra->nkeys, nelems)));
3291+
3292+
for (i = 0; i < nelems; i++)
3293+
{
3294+
Datum hash;
3295+
3296+
if (isnull[i])
3297+
continue;
3298+
3299+
Assert(OidIsValid(my_extra->partsupfunc[0].fn_oid));
3300+
3301+
hash = FunctionCall2(&my_extra->partsupfunc[0],
3302+
datum[i],
3303+
seed);
3304+
3305+
/* Form a single 64-bit hash value */
3306+
rowHash = hash_combine64(rowHash, DatumGetUInt64(hash));
3307+
}
3308+
}
31673309

31683310
PG_RETURN_BOOL(rowHash % modulus == remainder);
31693311
}

‎src/test/regress/expected/hash_part.out

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
--
2+
-- Hash partitioning.
3+
--
4+
CREATE OR REPLACE FUNCTION hashint4_noop(int4, int8) RETURNS int8 AS
5+
$$SELECT coalesce($1,0)::int8$$ LANGUAGE sql IMMUTABLE;
6+
CREATE OPERATOR CLASS test_int4_ops FOR TYPE int4 USING HASH AS
7+
OPERATOR 1 = , FUNCTION 2 hashint4_noop(int4, int8);
8+
CREATE OR REPLACE FUNCTION hashtext_length(text, int8) RETURNS int8 AS
9+
$$SELECT length(coalesce($1,''))::int8$$ LANGUAGE sql IMMUTABLE;
10+
CREATE OPERATOR CLASS test_text_ops FOR TYPE text USING HASH AS
11+
OPERATOR 1 = , FUNCTION 2 hashtext_length(text, int8);
12+
CREATE TABLE mchash (a int, b text, c jsonb)
13+
PARTITION BY HASH (a test_int4_ops, b test_text_ops);
14+
CREATE TABLE mchash1
15+
PARTITION OF mchash FOR VALUES WITH (MODULUS 4, REMAINDER 0);
16+
-- invalid OID, no such table
17+
SELECT satisfies_hash_partition(0, 4, 0, NULL);
18+
satisfies_hash_partition
19+
--------------------------
20+
21+
(1 row)
22+
23+
-- not partitioned
24+
SELECT satisfies_hash_partition('tenk1'::regclass, 4, 0, NULL);
25+
ERROR: "tenk1" is not a hash partitioned table
26+
-- partition rather than the parent
27+
SELECT satisfies_hash_partition('mchash1'::regclass, 4, 0, NULL);
28+
ERROR: "mchash1" is not a hash partitioned table
29+
-- invalid modulus
30+
SELECT satisfies_hash_partition('mchash'::regclass, 0, 0, NULL);
31+
ERROR: modulus for hash partition must be a positive integer
32+
-- remainder too small
33+
SELECT satisfies_hash_partition('mchash'::regclass, 1, -1, NULL);
34+
ERROR: remainder for hash partition must be a non-negative integer
35+
-- remainder too large
36+
SELECT satisfies_hash_partition('mchash'::regclass, 1, 1, NULL);
37+
ERROR: remainder for hash partition must be less than modulus
38+
-- modulus is null
39+
SELECT satisfies_hash_partition('mchash'::regclass, NULL, 0, NULL);
40+
satisfies_hash_partition
41+
--------------------------
42+
43+
(1 row)
44+
45+
-- remainder is null
46+
SELECT satisfies_hash_partition('mchash'::regclass, 4, NULL, NULL);
47+
satisfies_hash_partition
48+
--------------------------
49+
50+
(1 row)
51+
52+
-- too many arguments
53+
SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, NULL::int, NULL::text, NULL::json);
54+
ERROR: number of partitioning columns (2) does not match number of partition keys provided (3)
55+
-- too few arguments
56+
SELECT satisfies_hash_partition('mchash'::regclass, 3, 1, NULL::int);
57+
ERROR: number of partitioning columns (2) does not match number of partition keys provided (1)
58+
-- wrong argument type
59+
SELECT satisfies_hash_partition('mchash'::regclass, 2, 1, NULL::int, NULL::int);
60+
ERROR: column 2 of the partition key has type "text", but supplied value is of type "integer"
61+
-- ok, should be false
62+
SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, 0, ''::text);
63+
satisfies_hash_partition
64+
--------------------------
65+
f
66+
(1 row)
67+
68+
-- ok, should be true
69+
SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, 1, ''::text);
70+
satisfies_hash_partition
71+
--------------------------
72+
t
73+
(1 row)
74+
75+
-- argument via variadic syntax, should fail because not all partitioning
76+
-- columns are of the correct type
77+
SELECT satisfies_hash_partition('mchash'::regclass, 2, 1,
78+
variadic array[1,2]::int[]);
79+
ERROR: column 2 of the partition key has type "text", but supplied value is of type "integer"
80+
-- multiple partitioning columns of the same type
81+
CREATE TABLE mcinthash (a int, b int, c jsonb)
82+
PARTITION BY HASH (a test_int4_ops, b test_int4_ops);
83+
-- now variadic should work, should be false
84+
SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
85+
variadic array[0, 0]);
86+
satisfies_hash_partition
87+
--------------------------
88+
f
89+
(1 row)
90+
91+
-- should be true
92+
SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
93+
variadic array[1, 0]);
94+
satisfies_hash_partition
95+
--------------------------
96+
t
97+
(1 row)
98+
99+
-- wrong length
100+
SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
101+
variadic array[]::int[]);
102+
ERROR: number of partitioning columns (2) does not match number of partition keys provided (0)
103+
-- wrong type
104+
SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
105+
variadic array[now(), now()]);
106+
ERROR: column 1 of the partition key has type "integer", but supplied value is of type "timestamp with time zone"
107+
-- cleanup
108+
DROP TABLE mchash;
109+
DROP TABLE mcinthash;
110+
DROP OPERATOR CLASS test_text_ops USING hash;
111+
DROP OPERATOR CLASS test_int4_ops USING hash;
112+
DROP FUNCTION hashint4_noop(int4, int8);
113+
DROP FUNCTION hashtext_length(text, int8);

‎src/test/regress/parallel_schedule

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ test: plancache limit plpgsql copy2 temp domain rangefuncs prepare without_oid c
116116
# ----------
117117
# Another group of parallel tests
118118
# ----------
119-
test: identity partition_join reloptions
119+
test: identity partition_join reloptions hash_part
120120

121121
# event triggers cannot run concurrently with any test that runs DDL
122122
test: event_trigger

‎src/test/regress/serial_schedule

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,5 +181,6 @@ test: xml
181181
test: identity
182182
test: partition_join
183183
test: reloptions
184+
test: hash_part
184185
test: event_trigger
185186
test: stats

0 commit comments

Comments
 (0)