pgsql: Add hash partitioning.
Add hash partitioning.
Hash partitioning is useful when you want to partition a growing data
set evenly. This can be useful to keep table sizes reasonable, which
makes maintenance operations such as VACUUM faster, or to enable
partition-wise join.
At present, we still depend on constraint exclusion for partitioning
pruning, and the shape of the partition constraints for hash
partitioning is such that that doesn't work. Work is underway to fix
that, which should both improve performance and make partitioning
pruning work with hash partitioning.
Amul Sul, reviewed and tested by Dilip Kumar, Ashutosh Bapat, Yugo
Nagata, Rajkumar Raghuwanshi, Jesper Pedersen, and by me. A few
final tweaks also by me.
Discussion: /messages/by-id/CAAJ_b96fhpJAP=ALbETmeLk1Uni_GFZD938zgenhF49qgDTjaQ@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/1aba8e651ac3e37e1d2d875842de1e0ed22a651e
Modified Files
--------------
doc/src/sgml/ddl.sgml | 28 +-
doc/src/sgml/ref/alter_table.sgml | 7 +
doc/src/sgml/ref/create_table.sgml | 85 +++-
src/backend/catalog/partition.c | 682 ++++++++++++++++++++++++---
src/backend/commands/tablecmds.c | 48 +-
src/backend/nodes/copyfuncs.c | 2 +
src/backend/nodes/equalfuncs.c | 2 +
src/backend/nodes/outfuncs.c | 2 +
src/backend/nodes/readfuncs.c | 2 +
src/backend/optimizer/path/joinrels.c | 12 +-
src/backend/parser/gram.y | 76 ++-
src/backend/parser/parse_utilcmd.c | 29 +-
src/backend/utils/adt/ruleutils.c | 15 +-
src/backend/utils/cache/relcache.c | 26 +-
src/bin/psql/tab-complete.c | 2 +-
src/include/catalog/catversion.h | 2 +-
src/include/catalog/partition.h | 3 +
src/include/catalog/pg_proc.h | 4 +
src/include/nodes/parsenodes.h | 8 +-
src/test/regress/expected/alter_table.out | 62 +++
src/test/regress/expected/create_table.out | 78 ++-
src/test/regress/expected/insert.out | 46 ++
src/test/regress/expected/partition_join.out | 81 ++++
src/test/regress/expected/update.out | 29 ++
src/test/regress/sql/alter_table.sql | 64 +++
src/test/regress/sql/create_table.sql | 51 +-
src/test/regress/sql/insert.sql | 33 ++
src/test/regress/sql/partition_join.sql | 32 ++
src/test/regress/sql/update.sql | 28 ++
src/tools/pgindent/typedefs.list | 1 +
30 files changed, 1420 insertions(+), 120 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Robert Haas writes:
Add hash partitioning.
sqlsmith triggers coredumps calling satisfies_hash_partition().
ISTM this function is lacking argument validation. Example:
,----
| PostgreSQL stand-alone backend 11devel
| backend> select satisfies_hash_partition('pg_class'::regclass,null,null,null);
| Program received signal SIGSEGV, Segmentation fault.
| 0x00005555556b3914 in satisfies_hash_partition (fcinfo=0x555555f5f668) at partition.c:3435
| 3435 fmgr_info_copy(&my_extra->partsupfunc[j],
`----
regards,
Andreas
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Sun, Nov 12, 2017 at 3:01 AM, Andreas Seltenreich <seltenreich@gmx.de> wrote:
Robert Haas writes:
Add hash partitioning.
sqlsmith triggers coredumps calling satisfies_hash_partition().
ISTM this function is lacking argument validation. Example:
Thanks for the bug report. Please find attached patch does the fix.
Regards,
Amul
Attachments:
0001-argument-validation.patchapplication/octet-stream; name=0001-argument-validation.patchDownload
From 224b6825dcef26e0dd86483453b6854e6b888181 Mon Sep 17 00:00:00 2001
From: Amul Sul <sulamul@gmail.com>
Date: Mon, 13 Nov 2017 12:30:28 +0530
Subject: [PATCH] argument validation
---
src/backend/catalog/partition.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index cff59ed055..b170d10d5e 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -3407,6 +3407,17 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
ColumnsHashData *my_extra;
uint64 rowHash = 0;
+ /*
+ * Return false if any of the following violated:
+ *
+ * 1. Parent id must be valid relation id,
+ * 2. Modulus must be a positive integer and
+ * 3. Remainder must be a non-negative integer less than the modulus.
+ */
+ if (!OidIsValid(parentId) || modulus <= 0 || remainder < 0 ||
+ modulus <= remainder)
+ PG_RETURN_BOOL(false);
+
/*
* Cache hash function information.
*/
@@ -3418,6 +3429,19 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
PartitionKey key;
int j;
+ /* Open parent relation and fetch partition keyinfo */
+ parent = heap_open(parentId, AccessShareLock);
+
+ /* Return false if given parent is not a partitioned relation */
+ if (parent->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ heap_close(parent, AccessShareLock);
+ PG_RETURN_BOOL(false);
+ }
+
+ key = RelationGetPartitionKey(parent);
+ Assert(key->partnatts == nkeys);
+
fcinfo->flinfo->fn_extra =
MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
offsetof(ColumnsHashData, partsupfunc) +
@@ -3426,11 +3450,6 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
my_extra->nkeys = nkeys;
my_extra->relid = parentId;
- /* Open parent relation and fetch partition keyinfo */
- parent = heap_open(parentId, AccessShareLock);
- key = RelationGetPartitionKey(parent);
-
- Assert(key->partnatts == nkeys);
for (j = 0; j < nkeys; ++j)
fmgr_info_copy(&my_extra->partsupfunc[j],
key->partsupfunc,
--
2.14.1
On Mon, Nov 13, 2017 at 12:41 PM, amul sul <sulamul@gmail.com> wrote:
On Sun, Nov 12, 2017 at 3:01 AM, Andreas Seltenreich <seltenreich@gmx.de> wrote:
Robert Haas writes:
Add hash partitioning.
sqlsmith triggers coredumps calling satisfies_hash_partition().
ISTM this function is lacking argument validation. Example:Thanks for the bug report. Please find attached patch does the fix.
Updated patch attached -- Adjusted code comment to survive against pgindent.
Regards,
Amul
Attachments:
0001-argument-validation-v2.patchapplication/octet-stream; name=0001-argument-validation-v2.patchDownload
From b122296aade635c3e2e8912c20db0260514291e2 Mon Sep 17 00:00:00 2001
From: Amul Sul <sulamul@gmail.com>
Date: Mon, 13 Nov 2017 12:30:28 +0530
Subject: [PATCH] argument validation v2
---
src/backend/catalog/partition.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index cff59ed055..bdcd2d447d 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -3407,6 +3407,18 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
ColumnsHashData *my_extra;
uint64 rowHash = 0;
+ /*----------
+ * Return false if any of the following violated:
+ *
+ * 1. Parent id must be valid relation id,
+ * 2. Modulus must be a positive integer and
+ * 3. Remainder must be a non-negative integer less than the modulus.
+ *----------
+ */
+ if (!OidIsValid(parentId) || modulus <= 0 || remainder < 0 ||
+ modulus <= remainder)
+ PG_RETURN_BOOL(false);
+
/*
* Cache hash function information.
*/
@@ -3416,7 +3428,20 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
{
Relation parent;
PartitionKey key;
- int j;
+ int j;
+
+ /* Open parent relation and fetch partition keyinfo */
+ parent = heap_open(parentId, AccessShareLock);
+
+ /* Return false if given parent is not a partitioned relation */
+ if (parent->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ heap_close(parent, AccessShareLock);
+ PG_RETURN_BOOL(false);
+ }
+
+ key = RelationGetPartitionKey(parent);
+ Assert(key->partnatts == nkeys);
fcinfo->flinfo->fn_extra =
MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
@@ -3426,11 +3451,6 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
my_extra->nkeys = nkeys;
my_extra->relid = parentId;
- /* Open parent relation and fetch partition keyinfo */
- parent = heap_open(parentId, AccessShareLock);
- key = RelationGetPartitionKey(parent);
-
- Assert(key->partnatts == nkeys);
for (j = 0; j < nkeys; ++j)
fmgr_info_copy(&my_extra->partsupfunc[j],
key->partsupfunc,
--
2.14.1
On Mon, Nov 13, 2017 at 3:24 AM, amul sul <sulamul@gmail.com> wrote:
Updated patch attached -- Adjusted code comment to survive against pgindent.
That's not the right fix, or at least it's not complete. You
shouldn't call PG_GETARG_...(n) until you've verified that
PG_ARGISNULL(n) returns false.
Also, I don't think moving the heap_open() earlier helps anything, but
you do need to replace Assert(key->partnatts == nkeys) with an
ereport() -- or just return false, but I think ereport() is probably
better. Otherwise someone calling satisfies_hash_function() with a
wrong number of arguments for the partitioned table can cause an
assertion failure, which is bad.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Nov 14, 2017 at 3:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Nov 13, 2017 at 3:24 AM, amul sul <sulamul@gmail.com> wrote:
Updated patch attached -- Adjusted code comment to survive against pgindent.
That's not the right fix, or at least it's not complete. You
shouldn't call PG_GETARG_...(n) until you've verified that
PG_ARGISNULL(n) returns false.Also, I don't think moving the heap_open() earlier helps anything, but
you do need to replace Assert(key->partnatts == nkeys) with an
ereport() -- or just return false, but I think ereport() is probably
better. Otherwise someone calling satisfies_hash_function() with a
wrong number of arguments for the partitioned table can cause an
assertion failure, which is bad.
Yeah, this patch needs more work. There is no need to do much efforts
on HEAD to crash it:
=# create table aa (a int);
CREATE TABLE
=# select satisfies_hash_partition('aa'::regclass, 0, NULL, 'po');
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
Could you add regression tests calling directly this function?
elog() can also be triggered easily, which should not happen with
user-callable functions:
=# select satisfies_hash_partition(0, 0, NULL, 'po');
ERROR: XX000: could not open relation with OID 0
Thanks,
--
Michael
Thanks, Michael & Robert for your suggestions, and apologize for
delayed response
On Tue, Nov 14, 2017 at 6:02 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Tue, Nov 14, 2017 at 3:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Nov 13, 2017 at 3:24 AM, amul sul <sulamul@gmail.com> wrote:
Updated patch attached -- Adjusted code comment to survive against pgindent.
That's not the right fix, or at least it's not complete. You
shouldn't call PG_GETARG_...(n) until you've verified that
PG_ARGISNULL(n) returns false.Also, I don't think moving the heap_open() earlier helps anything, but
you do need to replace Assert(key->partnatts == nkeys) with an
ereport() -- or just return false, but I think ereport() is probably
better. Otherwise someone calling satisfies_hash_function() with a
wrong number of arguments for the partitioned table can cause an
assertion failure, which is bad.
Understood, fixed in the 001 patch.
Yeah, this patch needs more work. There is no need to do much efforts
on HEAD to crash it:
=# create table aa (a int);
CREATE TABLE
=# select satisfies_hash_partition('aa'::regclass, 0, NULL, 'po');
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
Fixed in the 001 patch.
Could you add regression tests calling directly this function?
Yes sure, but I am not sure that we really need this and also not sure about
which regression file is well suitable for these tests (to be honest, I haven't
browsed regression directory in detail due to lack of time today), so created
new file in 002 patch which is WIP.
Do let me know your thoughts will try to improve 0002 patch, tomorrow.
elog() can also be triggered easily, which should not happen with
user-callable functions:
=# select satisfies_hash_partition(0, 0, NULL, 'po');
ERROR: XX000: could not open relation with OID 0
Fixed in the 001 patch.
IMHO, this function is not meant for a user, so that instead of ereport() cant
we simply return false? TWIW, I have attached 003 patch which replaces all
erepots() by return false.
Regards,
Amul Sul
Attachments:
0001-argument-validation-v3.patchapplication/octet-stream; name=0001-argument-validation-v3.patchDownload
From 02f83ec98ae362e006f2f5ae178a445a8c142289 Mon Sep 17 00:00:00 2001
From: Amul Sul <sulamul@gmail.com>
Date: Thu, 16 Nov 2017 16:51:34 +0530
Subject: [PATCH 1/3] argument validation v3
---
src/backend/catalog/partition.c | 65 +++++++++++++++++++++++++++++++++++------
1 file changed, 56 insertions(+), 9 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index ce29ba2eda..ea93646509 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -3103,15 +3103,41 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
int16 nkeys;
FmgrInfo partsupfunc[PARTITION_MAX_KEYS];
} ColumnsHashData;
- Oid parentId = PG_GETARG_OID(0);
- int modulus = PG_GETARG_INT32(1);
- int remainder = PG_GETARG_INT32(2);
+ Oid parentId;
+ int modulus;
+ int remainder;
short nkeys = PG_NARGS() - 3;
int i;
Datum seed = UInt64GetDatum(HASH_PARTITION_SEED);
ColumnsHashData *my_extra;
uint64 rowHash = 0;
+ /*----------
+ * Error out if any of the following violated:
+ *
+ * 1. Parent id, modulus, and the remainder should not be null.
+ * 2. Parent id must be valid relation id,
+ * 3. Modulus must be a positive integer and
+ * 4. Remainder must be a non-negative integer less than the modulus.
+ *----------
+ */
+ if (!(PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2)))
+ {
+ parentId = PG_GETARG_OID(0);
+ modulus = PG_GETARG_INT32(1);
+ remainder = PG_GETARG_INT32(2);
+
+ if (!OidIsValid(parentId) || modulus <= 0 || remainder < 0 ||
+ modulus <= remainder)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid hash partition bound")));
+ }
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("parent relation id, modulus and the remainder cannot be null")));
+
/*
* Cache hash function information.
*/
@@ -3121,7 +3147,33 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
{
Relation parent;
PartitionKey key;
- int j;
+ int j;
+
+ /* Open parent relation and fetch partition keyinfo */
+ parent = heap_open(parentId, AccessShareLock);
+ key = RelationGetPartitionKey(parent);
+
+ if (parent->rd_rel->relkind != RELKIND_PARTITIONED_TABLE ||
+ key->strategy != PARTITION_STRATEGY_HASH)
+ {
+ heap_close(parent, AccessShareLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not a hash partitioned table",
+ get_rel_name(parentId))));
+ }
+
+ /* complain if too few column values; we ignore extras, however */
+ if (key->partnatts > nkeys)
+ {
+ heap_close(parent, AccessShareLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("value missing for partition column %d",
+ (nkeys + 1))));
+ }
+ else
+ nkeys = key->partnatts;
fcinfo->flinfo->fn_extra =
MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
@@ -3131,11 +3183,6 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
my_extra->nkeys = nkeys;
my_extra->relid = parentId;
- /* Open parent relation and fetch partition keyinfo */
- parent = heap_open(parentId, AccessShareLock);
- key = RelationGetPartitionKey(parent);
-
- Assert(key->partnatts == nkeys);
for (j = 0; j < nkeys; ++j)
fmgr_info_copy(&my_extra->partsupfunc[j],
key->partsupfunc,
--
2.14.1
0002-WIP-regression-tests.patchapplication/octet-stream; name=0002-WIP-regression-tests.patchDownload
From 0d107e7efcbe191ce7f16d9f79cb16344566bbb8 Mon Sep 17 00:00:00 2001
From: Amul Sul <sulamul@gmail.com>
Date: Thu, 16 Nov 2017 19:11:18 +0530
Subject: [PATCH 2/3] WIP-regression-tests
---
src/test/regress/expected/func_sanity.out | 35 +++++++++++++++++++++++++++++++
src/test/regress/parallel_schedule | 3 +++
src/test/regress/sql/func_sanity.sql | 28 +++++++++++++++++++++++++
3 files changed, 66 insertions(+)
create mode 100644 src/test/regress/expected/func_sanity.out
create mode 100644 src/test/regress/sql/func_sanity.sql
diff --git a/src/test/regress/expected/func_sanity.out b/src/test/regress/expected/func_sanity.out
new file mode 100644
index 0000000000..0f85e4a1a1
--- /dev/null
+++ b/src/test/regress/expected/func_sanity.out
@@ -0,0 +1,35 @@
+--
+-- FUNC_SANITY
+-- Sanity checks for common errors in making error
+-- in function argument handling.
+--
+-- **************** satisfies_hash_partition ****************
+CREATE TABLE hash_parted(a int, b int) PARTITION BY HASH(a, b);
+CREATE TABLE list_parted(a int, b int) PARTITION BY LIST(a);
+-- Parent relation id, modulus and the remainder should not be null
+SELECT satisfies_hash_partition(NULL, NULL, NULL, NULL, NULL);
+ERROR: parent relation id, modulus and the remainder cannot be null
+SELECT satisfies_hash_partition('hash_parted'::regclass, NULL, NULL, NULL, NULL);
+ERROR: parent relation id, modulus and the remainder cannot be null
+SELECT satisfies_hash_partition('hash_parted'::regclass, 5, NULL, NULL, NULL);
+ERROR: parent relation id, modulus and the remainder cannot be null
+-- Remainder is greater than modulus
+SELECT satisfies_hash_partition('hash_parted'::regclass, 5, 6, NULL, NULL);
+ERROR: invalid hash partition bound
+-- Invalid modulus
+SELECT satisfies_hash_partition('hash_parted'::regclass, 0, 0, NULL, NULL);
+ERROR: invalid hash partition bound
+-- Invalid parent relation id
+SELECT satisfies_hash_partition(0, 6, 0, NULL, NULL);
+ERROR: invalid hash partition bound
+-- Parent is not a hash partitioned table.
+SELECT satisfies_hash_partition('pg_class'::regclass, 7, 6, NULL, NULL);
+ERROR: "pg_class" is not a hash partitioned table
+SELECT satisfies_hash_partition('list_parted'::regclass, 6, 2, NULL, NULL);
+ERROR: "list_parted" is not a hash partitioned table
+-- Less column values than partition column
+SELECT satisfies_hash_partition('hash_parted'::regclass, 6, 2, NULL);
+ERROR: value missing for partition column 2
+-- Clean up.
+DROP TABLE hash_parted;
+DROP TABLE list_parted;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index aa5e6af621..4786bb0895 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -123,3 +123,6 @@ test: event_trigger
# run stats by itself because its delay may be insufficient under heavy load
test: stats
+
+# sanity check for catalog functions
+test: func_sanity
diff --git a/src/test/regress/sql/func_sanity.sql b/src/test/regress/sql/func_sanity.sql
new file mode 100644
index 0000000000..803dbdd691
--- /dev/null
+++ b/src/test/regress/sql/func_sanity.sql
@@ -0,0 +1,28 @@
+--
+-- FUNC_SANITY
+-- Sanity checks for common errors in making error
+-- in function argument handling.
+--
+
+-- **************** satisfies_hash_partition ****************
+CREATE TABLE hash_parted(a int, b int) PARTITION BY HASH(a, b);
+CREATE TABLE list_parted(a int, b int) PARTITION BY LIST(a);
+-- Parent relation id, modulus and the remainder should not be null
+SELECT satisfies_hash_partition(NULL, NULL, NULL, NULL, NULL);
+SELECT satisfies_hash_partition('hash_parted'::regclass, NULL, NULL, NULL, NULL);
+SELECT satisfies_hash_partition('hash_parted'::regclass, 5, NULL, NULL, NULL);
+-- Remainder is greater than modulus
+SELECT satisfies_hash_partition('hash_parted'::regclass, 5, 6, NULL, NULL);
+-- Invalid modulus
+SELECT satisfies_hash_partition('hash_parted'::regclass, 0, 0, NULL, NULL);
+-- Invalid parent relation id
+SELECT satisfies_hash_partition(0, 6, 0, NULL, NULL);
+-- Parent is not a hash partitioned table.
+SELECT satisfies_hash_partition('pg_class'::regclass, 7, 6, NULL, NULL);
+SELECT satisfies_hash_partition('list_parted'::regclass, 6, 2, NULL, NULL);
+-- Less column values than partition column
+SELECT satisfies_hash_partition('hash_parted'::regclass, 6, 2, NULL);
+
+-- Clean up.
+DROP TABLE hash_parted;
+DROP TABLE list_parted;
--
2.14.1
0003-replace-erreport-by-return-false.patchapplication/octet-stream; name=0003-replace-erreport-by-return-false.patchDownload
From 559f9882995c32c76423d92e9a602146408f7fe0 Mon Sep 17 00:00:00 2001
From: Amul Sul <sulamul@gmail.com>
Date: Thu, 16 Nov 2017 19:44:52 +0530
Subject: [PATCH 3/3] replace erreport by return false
---
src/backend/catalog/partition.c | 48 ++++++++-------------------
src/test/regress/expected/func_sanity.out | 54 +++++++++++++++++++++++++------
2 files changed, 59 insertions(+), 43 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index ea93646509..520c3a05e6 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -3113,7 +3113,7 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
uint64 rowHash = 0;
/*----------
- * Error out if any of the following violated:
+ * Return false if any of the following violated:
*
* 1. Parent id, modulus, and the remainder should not be null.
* 2. Parent id must be valid relation id,
@@ -3121,22 +3121,16 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
* 4. Remainder must be a non-negative integer less than the modulus.
*----------
*/
- if (!(PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2)))
- {
- parentId = PG_GETARG_OID(0);
- modulus = PG_GETARG_INT32(1);
- remainder = PG_GETARG_INT32(2);
-
- if (!OidIsValid(parentId) || modulus <= 0 || remainder < 0 ||
- modulus <= remainder)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("invalid hash partition bound")));
- }
- else
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("parent relation id, modulus and the remainder cannot be null")));
+ if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2))
+ PG_RETURN_BOOL(false);
+
+ parentId = PG_GETARG_OID(0);
+ modulus = PG_GETARG_INT32(1);
+ remainder = PG_GETARG_INT32(2);
+
+ if (!OidIsValid(parentId) || modulus <= 0 || remainder < 0 ||
+ modulus <= remainder)
+ PG_RETURN_BOOL(false);
/*
* Cache hash function information.
@@ -3154,27 +3148,13 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
key = RelationGetPartitionKey(parent);
if (parent->rd_rel->relkind != RELKIND_PARTITIONED_TABLE ||
- key->strategy != PARTITION_STRATEGY_HASH)
+ key->strategy != PARTITION_STRATEGY_HASH ||
+ key->partnatts != nkeys)
{
heap_close(parent, AccessShareLock);
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("\"%s\" is not a hash partitioned table",
- get_rel_name(parentId))));
+ PG_RETURN_BOOL(false);
}
- /* complain if too few column values; we ignore extras, however */
- if (key->partnatts > nkeys)
- {
- heap_close(parent, AccessShareLock);
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("value missing for partition column %d",
- (nkeys + 1))));
- }
- else
- nkeys = key->partnatts;
-
fcinfo->flinfo->fn_extra =
MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
offsetof(ColumnsHashData, partsupfunc) +
diff --git a/src/test/regress/expected/func_sanity.out b/src/test/regress/expected/func_sanity.out
index 0f85e4a1a1..a3cbc33b39 100644
--- a/src/test/regress/expected/func_sanity.out
+++ b/src/test/regress/expected/func_sanity.out
@@ -8,28 +8,64 @@ CREATE TABLE hash_parted(a int, b int) PARTITION BY HASH(a, b);
CREATE TABLE list_parted(a int, b int) PARTITION BY LIST(a);
-- Parent relation id, modulus and the remainder should not be null
SELECT satisfies_hash_partition(NULL, NULL, NULL, NULL, NULL);
-ERROR: parent relation id, modulus and the remainder cannot be null
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
SELECT satisfies_hash_partition('hash_parted'::regclass, NULL, NULL, NULL, NULL);
-ERROR: parent relation id, modulus and the remainder cannot be null
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
SELECT satisfies_hash_partition('hash_parted'::regclass, 5, NULL, NULL, NULL);
-ERROR: parent relation id, modulus and the remainder cannot be null
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
-- Remainder is greater than modulus
SELECT satisfies_hash_partition('hash_parted'::regclass, 5, 6, NULL, NULL);
-ERROR: invalid hash partition bound
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
-- Invalid modulus
SELECT satisfies_hash_partition('hash_parted'::regclass, 0, 0, NULL, NULL);
-ERROR: invalid hash partition bound
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
-- Invalid parent relation id
SELECT satisfies_hash_partition(0, 6, 0, NULL, NULL);
-ERROR: invalid hash partition bound
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
-- Parent is not a hash partitioned table.
SELECT satisfies_hash_partition('pg_class'::regclass, 7, 6, NULL, NULL);
-ERROR: "pg_class" is not a hash partitioned table
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
SELECT satisfies_hash_partition('list_parted'::regclass, 6, 2, NULL, NULL);
-ERROR: "list_parted" is not a hash partitioned table
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
-- Less column values than partition column
SELECT satisfies_hash_partition('hash_parted'::regclass, 6, 2, NULL);
-ERROR: value missing for partition column 2
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
-- Clean up.
DROP TABLE hash_parted;
DROP TABLE list_parted;
--
2.14.1
On Thu, Nov 16, 2017 at 9:37 AM, amul sul <sulamul@gmail.com> wrote:
Fixed in the 001 patch.
IMHO, this function is not meant for a user, so that instead of ereport() cant
we simply return false? TWIW, I have attached 003 patch which replaces all
erepots() by return false.
I don't think just returning false is very helpful behavior, because
the user may not realize that the problem is that the function is
being called incorrectly rather than that the call is correct and the
answer is false.
I took your 0001 patch and made extensive modifications to it. I
replaced your regression tests from 0002 with a new set that I wrote
myself. The result is attached here. This version makes different
decisions about how to handle the various problem cases than you did;
it returns NULL for a NULL input or an OID for which no relation
exists, and throws specific error messages for the other cases,
matching the parser error messages that CREATE TABLE would issue where
possible, but with a different error code. It also checks that the
types match (which your patch did not, and which I'm fairly sure could
crash the server), makes the function work when invoked using the
explicit VARIADIC syntax (which seems fairly useless here but there's
no in-core precedent for variadic function which doesn't support that
case), and fixes the function header comment to describe the behavior
we committed rather than the older behavior you had in earlier patch
versions.
As far as I can tell, this should nail things down pretty tight, but
it would be great if someone can try to find a case where it still
breaks.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
0001-Fix-multiple-problems-with-satisfies_hash_partition.patchapplication/octet-stream; name=0001-Fix-multiple-problems-with-satisfies_hash_partition.patchDownload
From ae39ebe24849c4ebac919840b5456abbcd3c8714 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 17 Nov 2017 11:21:14 -0500
Subject: [PATCH] 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
---
src/backend/catalog/partition.c | 202 +++++++++++++++++++++++++++-----
src/test/regress/expected/hash_part.out | 113 ++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/hash_part.sql | 90 ++++++++++++++
5 files changed, 377 insertions(+), 31 deletions(-)
create mode 100644 src/test/regress/expected/hash_part.out
create mode 100644 src/test/regress/sql/hash_part.sql
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 67d4c2a09b..1a53eea6f2 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -40,6 +40,7 @@
#include "optimizer/planmain.h"
#include "optimizer/prep.h"
#include "optimizer/var.h"
+#include "parser/parse_coerce.h"
#include "rewrite/rewriteManip.h"
#include "storage/lmgr.h"
#include "utils/array.h"
@@ -3085,9 +3086,11 @@ compute_hash_value(PartitionKey key, Datum *values, bool *isnull)
/*
* satisfies_hash_partition
*
- * This is a SQL-callable function for use in hash partition constraints takes
- * an already computed hash values of each partition key attribute, and combine
- * them into a single hash value by calling hash_combine64.
+ * This is an SQL-callable function for use in hash partition constraints.
+ * The first three arguments are the parent table OID, modulus, and remainder.
+ * The remaining arguments are the value of the partitioning columns (or
+ * expressions); these are hashed and the results are combined into a single
+ * hash value by calling hash_combine64.
*
* Returns true if remainder produced when this computed single hash value is
* divided by the given modulus is equal to given remainder, otherwise false.
@@ -3100,60 +3103,160 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
typedef struct ColumnsHashData
{
Oid relid;
- int16 nkeys;
+ int nkeys;
+ Oid variadic_type;
+ int16 variadic_typlen;
+ bool variadic_typbyval;
+ char variadic_typalign;
FmgrInfo partsupfunc[PARTITION_MAX_KEYS];
} ColumnsHashData;
- Oid parentId = PG_GETARG_OID(0);
- int modulus = PG_GETARG_INT32(1);
- int remainder = PG_GETARG_INT32(2);
- short nkeys = PG_NARGS() - 3;
- int i;
+ Oid parentId;
+ int modulus;
+ int remainder;
Datum seed = UInt64GetDatum(HASH_PARTITION_SEED);
ColumnsHashData *my_extra;
uint64 rowHash = 0;
+ /* Return null if the parent OID, modulus, or remainder is NULL. */
+ if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2))
+ PG_RETURN_NULL();
+ parentId = PG_GETARG_OID(0);
+ modulus = PG_GETARG_INT32(1);
+ remainder = PG_GETARG_INT32(2);
+
+ /* Sanity check modulus and remainder. */
+ if (modulus <= 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("modulus for hash partition must be a positive integer")));
+ if (remainder < 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("remainder for hash partition must be a non-negative integer")));
+ if (remainder >= modulus)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("remainder for hash partition must be less than modulus")));
+
/*
* Cache hash function information.
*/
my_extra = (ColumnsHashData *) fcinfo->flinfo->fn_extra;
- if (my_extra == NULL || my_extra->nkeys != nkeys ||
- my_extra->relid != parentId)
+ if (my_extra == NULL || my_extra->relid != parentId)
{
Relation parent;
PartitionKey key;
- int j;
-
- fcinfo->flinfo->fn_extra =
- MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
- offsetof(ColumnsHashData, partsupfunc) +
- sizeof(FmgrInfo) * nkeys);
- my_extra = (ColumnsHashData *) fcinfo->flinfo->fn_extra;
- my_extra->nkeys = nkeys;
- my_extra->relid = parentId;
+ int j;
/* Open parent relation and fetch partition keyinfo */
- parent = heap_open(parentId, AccessShareLock);
+ parent = try_relation_open(parentId, AccessShareLock);
+ if (parent == NULL)
+ PG_RETURN_NULL();
key = RelationGetPartitionKey(parent);
- Assert(key->partnatts == nkeys);
- for (j = 0; j < nkeys; ++j)
- fmgr_info_copy(&my_extra->partsupfunc[j],
- key->partsupfunc,
+ /* Reject parent table that is not hash-partitioned. */
+ if (parent->rd_rel->relkind != RELKIND_PARTITIONED_TABLE ||
+ key->strategy != PARTITION_STRATEGY_HASH)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is not a hash partitioned table",
+ get_rel_name(parentId))));
+
+ if (!get_fn_expr_variadic(fcinfo->flinfo))
+ {
+ int nargs = PG_NARGS() - 3;
+
+ /* complain if wrong number of column values */
+ if (key->partnatts != nargs)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("number of partitioning columns (%d) does not match number of partition keys provided (%d)",
+ key->partnatts, nargs)));
+
+ /* allocate space for our cache */
+ fcinfo->flinfo->fn_extra =
+ MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
+ offsetof(ColumnsHashData, partsupfunc) +
+ sizeof(FmgrInfo) * nargs);
+ my_extra = (ColumnsHashData *) fcinfo->flinfo->fn_extra;
+ my_extra->relid = parentId;
+ my_extra->nkeys = key->partnatts;
+
+ /* check argument types and save fmgr_infos */
+ for (j = 0; j < key->partnatts; ++j)
+ {
+ Oid argtype = get_fn_expr_argtype(fcinfo->flinfo, j + 3);
+
+ if (argtype != key->parttypid[j] && !IsBinaryCoercible(argtype, key->parttypid[j]))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("column %d of the partition key has type \"%s\", but supplied value is of type \"%s\"",
+ j, format_type_be(key->parttypid[j]), format_type_be(argtype))));
+
+ fmgr_info_copy(&my_extra->partsupfunc[j],
+ &key->partsupfunc[j],
+ fcinfo->flinfo->fn_mcxt);
+ }
+
+ }
+ else
+ {
+ ArrayType *variadic_array = PG_GETARG_ARRAYTYPE_P(3);
+
+ /* allocate space for our cache -- just one FmgrInfo in this case */
+ fcinfo->flinfo->fn_extra =
+ MemoryContextAllocZero(fcinfo->flinfo->fn_mcxt,
+ offsetof(ColumnsHashData, partsupfunc) +
+ sizeof(FmgrInfo));
+ my_extra = (ColumnsHashData *) fcinfo->flinfo->fn_extra;
+ my_extra->relid = parentId;
+ my_extra->nkeys = key->partnatts;
+ my_extra->variadic_type = ARR_ELEMTYPE(variadic_array);
+ get_typlenbyvalalign(my_extra->variadic_type,
+ &my_extra->variadic_typlen,
+ &my_extra->variadic_typbyval,
+ &my_extra->variadic_typalign);
+
+ /* 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),
+ errmsg("column %d of the partition key has type \"%s\", but supplied value is of type \"%s\"",
+ j,
+ format_type_be(key->parttypid[j]),
+ format_type_be(my_extra->variadic_type))));
+
+ fmgr_info_copy(&my_extra->partsupfunc[0],
+ &key->partsupfunc[0],
fcinfo->flinfo->fn_mcxt);
+ }
/* Hold lock until commit */
- heap_close(parent, NoLock);
+ relation_close(parent, NoLock);
}
- for (i = 0; i < nkeys; i++)
+ if (!OidIsValid(my_extra->variadic_type))
{
- /* keys start from fourth argument of function. */
- int argno = i + 3;
+ int nkeys = my_extra->nkeys;
+ int i;
+
+ /*
+ * For a non-variadic call, neither the number of arguments nor their
+ * types can change across calls, so avoid the expense of rechecking
+ * here.
+ */
- if (!PG_ARGISNULL(argno))
+ for (i = 0; i < nkeys; i++)
{
Datum hash;
+ /* keys start from fourth argument of function. */
+ int argno = i + 3;
+
+ if (PG_ARGISNULL(argno))
+ continue;
+
Assert(OidIsValid(my_extra->partsupfunc[i].fn_oid));
hash = FunctionCall2(&my_extra->partsupfunc[i],
@@ -3164,6 +3267,45 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
rowHash = hash_combine64(rowHash, DatumGetUInt64(hash));
}
}
+ else
+ {
+ ArrayType *variadic_array = PG_GETARG_ARRAYTYPE_P(3);
+ int i;
+ int nelems;
+ Datum *datum;
+ bool *isnull;
+
+ deconstruct_array(variadic_array,
+ my_extra->variadic_type,
+ my_extra->variadic_typlen,
+ my_extra->variadic_typbyval,
+ my_extra->variadic_typalign,
+ &datum, &isnull, &nelems);
+
+ /* complain if wrong number of column values */
+ if (nelems != my_extra->nkeys)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("number of partitioning columns (%d) does not match number of partition keys provided (%d)",
+ my_extra->nkeys, nelems)));
+
+ for (i = 0; i < nelems; i++)
+ {
+ Datum hash;
+
+ if (isnull[i])
+ continue;
+
+ Assert(OidIsValid(my_extra->partsupfunc[0].fn_oid));
+
+ hash = FunctionCall2(&my_extra->partsupfunc[0],
+ datum[i],
+ seed);
+
+ /* Form a single 64-bit hash value */
+ rowHash = hash_combine64(rowHash, DatumGetUInt64(hash));
+ }
+ }
PG_RETURN_BOOL(rowHash % modulus == remainder);
}
diff --git a/src/test/regress/expected/hash_part.out b/src/test/regress/expected/hash_part.out
new file mode 100644
index 0000000000..fc2919bcd3
--- /dev/null
+++ b/src/test/regress/expected/hash_part.out
@@ -0,0 +1,113 @@
+--
+-- Hash partitioning.
+--
+CREATE OR REPLACE FUNCTION hashint4_noop(int4, int8) RETURNS int8 AS
+$$SELECT coalesce($1,0)::int8$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR CLASS test_int4_ops FOR TYPE int4 USING HASH AS
+OPERATOR 1 = , FUNCTION 2 hashint4_noop(int4, int8);
+CREATE OR REPLACE FUNCTION hashtext_length(text, int8) RETURNS int8 AS
+$$SELECT length(coalesce($1,''))::int8$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR CLASS test_text_ops FOR TYPE text USING HASH AS
+OPERATOR 1 = , FUNCTION 2 hashtext_length(text, int8);
+CREATE TABLE mchash (a int, b text, c jsonb)
+ PARTITION BY HASH (a test_int4_ops, b test_text_ops);
+CREATE TABLE mchash1
+ PARTITION OF mchash FOR VALUES WITH (MODULUS 4, REMAINDER 0);
+-- invalid OID, no such table
+SELECT satisfies_hash_partition(0, 4, 0, NULL);
+ satisfies_hash_partition
+--------------------------
+
+(1 row)
+
+-- not partitioned
+SELECT satisfies_hash_partition('tenk1'::regclass, 4, 0, NULL);
+ERROR: "tenk1" is not a hash partitioned table
+-- partition rather than the parent
+SELECT satisfies_hash_partition('mchash1'::regclass, 4, 0, NULL);
+ERROR: "mchash1" is not a hash partitioned table
+-- invalid modulus
+SELECT satisfies_hash_partition('mchash'::regclass, 0, 0, NULL);
+ERROR: modulus for hash partition must be a positive integer
+-- remainder too small
+SELECT satisfies_hash_partition('mchash'::regclass, 1, -1, NULL);
+ERROR: remainder for hash partition must be a non-negative integer
+-- remainder too large
+SELECT satisfies_hash_partition('mchash'::regclass, 1, 1, NULL);
+ERROR: remainder for hash partition must be less than modulus
+-- modulus is null
+SELECT satisfies_hash_partition('mchash'::regclass, NULL, 0, NULL);
+ satisfies_hash_partition
+--------------------------
+
+(1 row)
+
+-- remainder is null
+SELECT satisfies_hash_partition('mchash'::regclass, 4, NULL, NULL);
+ satisfies_hash_partition
+--------------------------
+
+(1 row)
+
+-- too many arguments
+SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, NULL::int, NULL::text, NULL::json);
+ERROR: number of partitioning columns (2) does not match number of partition keys provided (3)
+-- too few arguments
+SELECT satisfies_hash_partition('mchash'::regclass, 3, 1, NULL::int);
+ERROR: number of partitioning columns (2) does not match number of partition keys provided (1)
+-- wrong argument type
+SELECT satisfies_hash_partition('mchash'::regclass, 2, 1, NULL::int, NULL::int);
+ERROR: column 1 of the partition key has type "text", but supplied value is of type "integer"
+-- ok, should be false
+SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, 0, ''::text);
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
+-- ok, should be true
+SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, 1, ''::text);
+ satisfies_hash_partition
+--------------------------
+ t
+(1 row)
+
+-- argument via variadic syntax, should fail because not all partitioning
+-- columns are of the correct type
+SELECT satisfies_hash_partition('mchash'::regclass, 2, 1,
+ variadic array[1,2]::int[]);
+ERROR: column 1 of the partition key has type "text", but supplied value is of type "integer"
+-- multiple partitioning columns of the same type
+CREATE TABLE mcinthash (a int, b int, c jsonb)
+ PARTITION BY HASH (a test_int4_ops, b test_int4_ops);
+-- now variadic should work, should be false
+SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
+ variadic array[0, 0]);
+ satisfies_hash_partition
+--------------------------
+ f
+(1 row)
+
+-- should be true
+SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
+ variadic array[1, 0]);
+ satisfies_hash_partition
+--------------------------
+ t
+(1 row)
+
+-- wrong length
+SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
+ variadic array[]::int[]);
+ERROR: number of partitioning columns (2) does not match number of partition keys provided (0)
+-- wrong type
+SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
+ variadic array[now(), now()]);
+ERROR: column 0 of the partition key has type "integer", but supplied value is of type "timestamp with time zone"
+-- cleanup
+DROP TABLE mchash;
+DROP TABLE mcinthash;
+DROP OPERATOR CLASS test_text_ops USING hash;
+DROP OPERATOR CLASS test_int4_ops USING hash;
+DROP FUNCTION hashint4_noop(int4, int8);
+DROP FUNCTION hashtext_length(text, int8);
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index aa5e6af621..1a3ac4c1f9 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -116,7 +116,7 @@ test: plancache limit plpgsql copy2 temp domain rangefuncs prepare without_oid c
# ----------
# Another group of parallel tests
# ----------
-test: identity partition_join reloptions
+test: identity partition_join reloptions hash_part
# event triggers cannot run concurrently with any test that runs DDL
test: event_trigger
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 3866314a92..a205e5d05c 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -181,5 +181,6 @@ test: xml
test: identity
test: partition_join
test: reloptions
+test: hash_part
test: event_trigger
test: stats
diff --git a/src/test/regress/sql/hash_part.sql b/src/test/regress/sql/hash_part.sql
new file mode 100644
index 0000000000..94c5eaab0c
--- /dev/null
+++ b/src/test/regress/sql/hash_part.sql
@@ -0,0 +1,90 @@
+--
+-- Hash partitioning.
+--
+
+CREATE OR REPLACE FUNCTION hashint4_noop(int4, int8) RETURNS int8 AS
+$$SELECT coalesce($1,0)::int8$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR CLASS test_int4_ops FOR TYPE int4 USING HASH AS
+OPERATOR 1 = , FUNCTION 2 hashint4_noop(int4, int8);
+
+CREATE OR REPLACE FUNCTION hashtext_length(text, int8) RETURNS int8 AS
+$$SELECT length(coalesce($1,''))::int8$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR CLASS test_text_ops FOR TYPE text USING HASH AS
+OPERATOR 1 = , FUNCTION 2 hashtext_length(text, int8);
+
+CREATE TABLE mchash (a int, b text, c jsonb)
+ PARTITION BY HASH (a test_int4_ops, b test_text_ops);
+CREATE TABLE mchash1
+ PARTITION OF mchash FOR VALUES WITH (MODULUS 4, REMAINDER 0);
+
+-- invalid OID, no such table
+SELECT satisfies_hash_partition(0, 4, 0, NULL);
+
+-- not partitioned
+SELECT satisfies_hash_partition('tenk1'::regclass, 4, 0, NULL);
+
+-- partition rather than the parent
+SELECT satisfies_hash_partition('mchash1'::regclass, 4, 0, NULL);
+
+-- invalid modulus
+SELECT satisfies_hash_partition('mchash'::regclass, 0, 0, NULL);
+
+-- remainder too small
+SELECT satisfies_hash_partition('mchash'::regclass, 1, -1, NULL);
+
+-- remainder too large
+SELECT satisfies_hash_partition('mchash'::regclass, 1, 1, NULL);
+
+-- modulus is null
+SELECT satisfies_hash_partition('mchash'::regclass, NULL, 0, NULL);
+
+-- remainder is null
+SELECT satisfies_hash_partition('mchash'::regclass, 4, NULL, NULL);
+
+-- too many arguments
+SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, NULL::int, NULL::text, NULL::json);
+
+-- too few arguments
+SELECT satisfies_hash_partition('mchash'::regclass, 3, 1, NULL::int);
+
+-- wrong argument type
+SELECT satisfies_hash_partition('mchash'::regclass, 2, 1, NULL::int, NULL::int);
+
+-- ok, should be false
+SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, 0, ''::text);
+
+-- ok, should be true
+SELECT satisfies_hash_partition('mchash'::regclass, 4, 0, 1, ''::text);
+
+-- argument via variadic syntax, should fail because not all partitioning
+-- columns are of the correct type
+SELECT satisfies_hash_partition('mchash'::regclass, 2, 1,
+ variadic array[1,2]::int[]);
+
+-- multiple partitioning columns of the same type
+CREATE TABLE mcinthash (a int, b int, c jsonb)
+ PARTITION BY HASH (a test_int4_ops, b test_int4_ops);
+
+-- now variadic should work, should be false
+SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
+ variadic array[0, 0]);
+
+-- should be true
+SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
+ variadic array[1, 0]);
+
+-- wrong length
+SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
+ variadic array[]::int[]);
+
+-- wrong type
+SELECT satisfies_hash_partition('mcinthash'::regclass, 4, 0,
+ variadic array[now(), now()]);
+
+-- cleanup
+DROP TABLE mchash;
+DROP TABLE mcinthash;
+DROP OPERATOR CLASS test_text_ops USING hash;
+DROP OPERATOR CLASS test_int4_ops USING hash;
+DROP FUNCTION hashint4_noop(int4, int8);
+DROP FUNCTION hashtext_length(text, int8);
--
2.13.5 (Apple Git-94)
On Sat, Nov 18, 2017 at 1:19 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Nov 16, 2017 at 9:37 AM, amul sul <sulamul@gmail.com> wrote:
Fixed in the 001 patch.
IMHO, this function is not meant for a user, so that instead of ereport() cant
we simply return false? TWIW, I have attached 003 patch which replaces all
erepots() by return false.I don't think just returning false is very helpful behavior, because
the user may not realize that the problem is that the function is
being called incorrectly rather than that the call is correct and the
answer is false.I took your 0001 patch and made extensive modifications to it. I
replaced your regression tests from 0002 with a new set that I wrote
myself. The result is attached here. This version makes different
decisions about how to handle the various problem cases than you did;
it returns NULL for a NULL input or an OID for which no relation
exists, and throws specific error messages for the other cases,
matching the parser error messages that CREATE TABLE would issue where
possible, but with a different error code. It also checks that the
types match (which your patch did not, and which I'm fairly sure could
crash the server), makes the function work when invoked using the
explicit VARIADIC syntax (which seems fairly useless here but there's
no in-core precedent for variadic function which doesn't support that
case), and fixes the function header comment to describe the behavior
we committed rather than the older behavior you had in earlier patch
versions.As far as I can tell, this should nail things down pretty tight, but
it would be great if someone can try to find a case where it still
breaks.
Thanks for fixing this function. Patch looks good to me, except column number
in the following errors message should to be 2.
354 +SELECT satisfies_hash_partition('mchash'::regclass, 2, 1,
NULL::int, NULL::int);
355 +ERROR: column 1 of the partition key has type "text", but
supplied value is of type "integer"
Same at the line # 374 & 401 in the patch.
Regards,
Amul
On Mon, Nov 20, 2017 at 7:46 AM, amul sul <sulamul@gmail.com> wrote:
Thanks for fixing this function. Patch looks good to me, except column number
in the following errors message should to be 2.354 +SELECT satisfies_hash_partition('mchash'::regclass, 2, 1,
NULL::int, NULL::int);
355 +ERROR: column 1 of the partition key has type "text", but
supplied value is of type "integer"Same at the line # 374 & 401 in the patch.
Oops. Looks like the indexing should be 1-based rather than 0-based.
Committed with that change.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company