From: Jesper Pedersen <jesper.pedersen@redhat.com>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
To: sulamul@gmail.com
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Subject: Re: COLLATE: Hash partition vs UPDATE
Date: Tue, 9 Apr 2019 08:43:57 -0400
Message-ID: <826e4d52-234a-4d35-73b2-7c6b0a92220e@redhat.com> (raw)
In-Reply-To: <b462ff0c-a846-dce6-b0a7-ab1397e73b98@lab.ntt.co.jp>
References: <a620f85a-42ab-e0f3-3337-b04b97e2e2f5@redhat.com>
<b462ff0c-a846-dce6-b0a7-ab1397e73b98@lab.ntt.co.jp>
Hi Amit,
On 4/8/19 11:18 PM, Amit Langote wrote:
> 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.
>
Yeah, that works here - apart from an issue with the test case; fixed in
the attached.
Best regards,
Jesper
Attachments:satisfies_hash_partition-collate-2.patch (text/x-patch, 4.4K) download
From 1902dcf1fd31c95fd95e1231630b3c446857cd59 Mon Sep 17 00:00:00 2001
From: jesperpedersen <jesper.pedersen@redhat.com>
Date: Tue, 9 Apr 2019 07:35:28 -0400
Subject: [PATCH] 0001
---
src/backend/partitioning/partbounds.c | 12 +++++++++---
src/test/regress/expected/hash_part.out | 14 ++++++++++++++
src/test/regress/sql/hash_part.sql | 10 ++++++++++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.cindex 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.outindex 731d26fc3d..adb4410b4b 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 at least+-- 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.sqlindex f457ac344c..8cd9e8a8a2 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 at least+-- 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;--
2.17.2
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jesper.pedersen@redhat.com, Langote_Amit_f8@lab.ntt.co.jp, sulamul@gmail.com, pgsql-hackers@lists.postgresql.org, peter.eisentraut@2ndquadrant.com
Subject: Re: COLLATE: Hash partition vs UPDATE
In-Reply-To: <826e4d52-234a-4d35-73b2-7c6b0a92220e@redhat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox