Thanks for the review.
On 2019/04/15 5:50, Tom Lane wrote:
> Jesper Pedersen <jesper.peder...@redhat.com> writes:
>> Yeah, that works here - apart from an issue with the test case; fixed in
>> the attached.
>
> Couple issues spotted in an eyeball review of that:
>
> * There is code that supposes that partsupfunc[] is the last
> field of ColumnsHashData, eg
>
> fcinfo->flinfo->fn_extra =
> MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
> offsetof(ColumnsHashData, partsupfunc)
> +
> sizeof(FmgrInfo) * nargs);
>
> I'm a bit surprised that this patch manages to run without crashing,
> because this would certainly not allocate space for partcollid[].
>
> I think we would likely be well advised to do
>
> - FmgrInfo partsupfunc[PARTITION_MAX_KEYS];
> + FmgrInfo partsupfunc[FLEXIBLE_ARRAY_MEMBER];
I went with this:
- FmgrInfo partsupfunc[PARTITION_MAX_KEYS];
Oid partcollid[PARTITION_MAX_KEYS];
+ FmgrInfo partsupfunc[FLEXIBLE_ARRAY_MEMBER];
> to make it more obvious that that has to be the last field. Or else
> drop the cuteness with variable-size allocations of ColumnsHashData.
> FmgrInfo is only 48 bytes, I'm not really sure that it's worth the
> risk of bugs to "optimize" this.
I wonder if workloads on hash partitioned tables that require calling
satisfies_hash_partition repeatedly may not be as common as thought when
writing this code? The only case I see where it's being repeatedly called
is bulk inserts into a hash-partitioned table, that too, only if BR
triggers on partitions necessitate rechecking the partition constraint.
> * I see collation-less calls of the partsupfunc at both partbounds.c:2931
> and partbounds.c:2970, but this patch touches only the first one. How
> can that be right?
Oops, that's wrong.
Attached updated patch.
Thanks,
Amit
diff --git a/src/backend/partitioning/partbounds.c
b/src/backend/partitioning/partbounds.c
index c8770ccfee..460f023134 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -2777,7 +2777,8 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
int16 variadic_typlen;
bool variadic_typbyval;
char variadic_typalign;
- FmgrInfo partsupfunc[PARTITION_MAX_KEYS];
+ Oid partcollid[PARTITION_MAX_KEYS];
+ FmgrInfo partsupfunc[FLEXIBLE_ARRAY_MEMBER];
} ColumnsHashData;
Oid parentId;
int modulus;
@@ -2851,6 +2852,8 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
my_extra->relid = parentId;
my_extra->nkeys = key->partnatts;
+ memcpy(my_extra->partcollid, key->partcollation,
+ key->partnatts * sizeof(Oid));
/* check argument types and save fmgr_infos */
for (j = 0; j < key->partnatts; ++j)
{
@@ -2885,6 +2888,7 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
&my_extra->variadic_typlen,
&my_extra->variadic_typbyval,
&my_extra->variadic_typalign);
+ my_extra->partcollid[0] = key->partcollation[0];
/* check argument types */
for (j = 0; j < key->partnatts; ++j)
@@ -2928,9 +2932,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));
@@ -2967,9 +2972,10 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
Assert(OidIsValid(my_extra->partsupfunc[0].fn_oid));
- hash = FunctionCall2(&my_extra->partsupfunc[0],
- datum[i],
- seed);
+ hash = FunctionCall2Coll(&my_extra->partsupfunc[0],
+
my_extra->partcollid[0],
+
datum[i],
+ 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..8db316be27 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 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;
+ 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;
