Hi Jesper,
On 2019/04/09 1:33, Jesper Pedersen wrote:
> Hi,
>
> The following case
>
> -- test.sql --
> CREATE TABLE test (a text PRIMARY KEY, b text) PARTITION BY HASH (a);
> CREATE TABLE test_p0 PARTITION OF test FOR VALUES WITH (MODULUS 2,
> REMAINDER 0);
> CREATE TABLE test_p1 PARTITION OF test FOR VALUES WITH (MODULUS 2,
> REMAINDER 1);
> -- CREATE INDEX idx_test_b ON test USING HASH (b);
>
> INSERT INTO test VALUES ('aaaa', 'aaaa');
>
> -- Regression
> UPDATE test SET b = 'bbbb' WHERE a = 'aaaa';
> -- test.sql --
>
> fails on master, which includes [1], with
>
>
> psql:test.sql:9: ERROR: could not determine which collation to use for
> string hashing
> HINT: Use the COLLATE clause to set the collation explicitly.
>
>
> It passes on 11.x.
Thanks for the report.
This seems to broken since the following commit (I see you already cc'd
Peter):
commit 5e1963fb764e9cc092e0f7b58b28985c311431d9
Author: Peter Eisentraut <[email protected]>
Date: Fri Mar 22 12:09:32 2019 +0100
Collations with nondeterministic comparison
As of this commit, hashing functions hashtext() and hashtextextended()
require a valid collation to be passed in. ISTM,
satisfies_hash_partition() that's called by hash partition constraint
checking should have been changed to use FunctionCall2Coll() interface to
account for the requirements of the above commit. I see that it did that
for compute_partition_hash_value(), which is used by hash partition tuple
routing. That also seems to be covered by regression tests, but there are
no tests that cover satisfies_hash_partition().
Attached patch is an attempt to fix this. I've also added Amul Sul who
can maybe comment on the satisfies_hash_partition() changes.
BTW, it seems we don't need to back-patch this to PG 11 which introduced
hash partitioning, because text hashing functions don't need collation
there, right?
Thanks,
Amit
diff --git a/src/backend/partitioning/partbounds.c
b/src/backend/partitioning/partbounds.c
index c8770ccfee..331dab3954 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -2778,6 +2778,7 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
bool variadic_typbyval;
char variadic_typalign;
FmgrInfo partsupfunc[PARTITION_MAX_KEYS];
+ Oid partcollid[PARTITION_MAX_KEYS];
} ColumnsHashData;
Oid parentId;
int modulus;
@@ -2865,6 +2866,7 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
fmgr_info_copy(&my_extra->partsupfunc[j],
&key->partsupfunc[j],
fcinfo->flinfo->fn_mcxt);
+ my_extra->partcollid[j] = key->partcollation[j];
}
}
@@ -2888,6 +2890,7 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
/* check argument types */
for (j = 0; j < key->partnatts; ++j)
+ {
if (key->parttypid[j] !=
my_extra->variadic_type)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -2895,6 +2898,8 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
j + 1,
format_type_be(key->parttypid[j]),
format_type_be(my_extra->variadic_type))));
+ my_extra->partcollid[j] = key->partcollation[j];
+ }
fmgr_info_copy(&my_extra->partsupfunc[0],
&key->partsupfunc[0],
@@ -2928,9 +2933,10 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
Assert(OidIsValid(my_extra->partsupfunc[i].fn_oid));
- hash = FunctionCall2(&my_extra->partsupfunc[i],
-
PG_GETARG_DATUM(argno),
- seed);
+ hash = FunctionCall2Coll(&my_extra->partsupfunc[i],
+
my_extra->partcollid[i],
+
PG_GETARG_DATUM(argno),
+ seed);
/* Form a single 64-bit hash value */
rowHash = hash_combine64(rowHash, DatumGetUInt64(hash));
diff --git a/src/test/regress/expected/hash_part.out
b/src/test/regress/expected/hash_part.out
index 731d26fc3d..b536b6955c 100644
--- a/src/test/regress/expected/hash_part.out
+++ b/src/test/regress/expected/hash_part.out
@@ -99,6 +99,20 @@ ERROR: number of partitioning columns (2) does not match
number of partition ke
SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
variadic
array[now(), now()]);
ERROR: column 1 of the partition key has type "integer", but supplied value
is of type "timestamp with time zone"
+-- check satisfies_hash_partition passes correct collation
+create table text_hashp (a text) partition by hash (a);
+create table text_hashp0 partition of text_hashp for values with (modulus 2,
remainder 0);
+create table text_hashp1 partition of text_hashp for values with (modulus 2,
remainder 1);
+-- The result here should always be true, because 'xxx' must belong at least
+-- one of the partitions
+select satisfies_hash_partition('text_hashp'::regclass, 2, 0, 'xxx'::text) OR
+ satisfies_hash_partition('text_hashp'::regclass, 2, 1, 'xxx'::text)
AS satisfies;
+ satisfies
+-----------
+ t
+(1 row)
+
-- cleanup
DROP TABLE mchash;
DROP TABLE mcinthash;
+DROP TABLE text_hashp;
diff --git a/src/test/regress/sql/hash_part.sql
b/src/test/regress/sql/hash_part.sql
index f457ac344c..30601b913e 100644
--- a/src/test/regress/sql/hash_part.sql
+++ b/src/test/regress/sql/hash_part.sql
@@ -75,6 +75,16 @@ SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
variadic
array[now(), now()]);
+-- check satisfies_hash_partition passes correct collation
+create table text_hashp (a text) partition by hash (a);
+create table text_hashp0 partition of text_hashp for values with (modulus 2,
remainder 0);
+create table text_hashp1 partition of text_hashp for values with (modulus 2,
remainder 1);
+-- The result here should always be true, because 'xxx' must belong to
+-- one of the two defined partitions
+select satisfies_hash_partition('text_hashp'::regclass, 2, 0, 'xxx'::text) OR
+ satisfies_hash_partition('text_hashp'::regclass, 2, 1, 'xxx'::text)
AS satisfies;
+
-- cleanup
DROP TABLE mchash;
DROP TABLE mcinthash;
+DROP TABLE text_hashp;
