On 2021-02-03 15:52, Peter Eisentraut wrote:
On 2021-02-02 13:26, Heikki Linnakangas wrote:
How about this?
CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS
25, REMAINDER 3);
ERROR: every hash partition modulus must be a factor of the next larger
modulus
DETAIL: 25 is not divisible by 10, the modulus of existing partition
"hpart_1"
I don't know if we can easily get the name of the existing partition.
I'll have to check that.
I'm worried that this phrasing requires the user to understand that
"divisible by" is related to "factor of", which is of course correct but
introduces yet more complexity into this.
I'll play around with this a bit more.
Here is a new patch that implements the suggestions.
From 537e8f2f27e43b94777b962e408245fb1d4784dd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 15 Feb 2021 17:10:11 +0100
Subject: [PATCH v2] Improve new hash partition bound check error messages
For the error message "every hash partition modulus must be a factor
of the next larger modulus", add a detail message that shows the
particular numbers involved. Also comment the code more.
Discussion:
https://www.postgresql.org/message-id/flat/bb9d60b4-aadb-607a-1a9d-fdc3434dddcd%40enterprisedb.com
---
src/backend/partitioning/partbounds.c | 62 +++++++++++++++-------
src/test/regress/expected/alter_table.out | 1 +
src/test/regress/expected/create_table.out | 2 +
3 files changed, 47 insertions(+), 18 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c
b/src/backend/partitioning/partbounds.c
index 0c3f212ff2..02cd58ce5f 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -2832,14 +2832,9 @@ check_new_partition_bound(char *relname, Relation parent,
if (partdesc->nparts > 0)
{
- Datum **datums = boundinfo->datums;
- int ndatums =
boundinfo->ndatums;
int
greatest_modulus;
int remainder;
int offset;
- bool valid_modulus = true;
- int prev_modulus,
/* Previous largest modulus */
- next_modulus;
/* Next largest modulus */
/*
* Check rule that every modulus must
be a factor of the
@@ -2849,7 +2844,9 @@ check_new_partition_bound(char *relname, Relation parent,
* modulus 15, but you cannot add both
a partition with
* modulus 10 and a partition with
modulus 15, because 10
* is not a factor of 15.
- *
+ */
+
+ /*
* Get the greatest (modulus,
remainder) pair contained in
* boundinfo->datums that is less than
or equal to the
* (spec->modulus, spec->remainder)
pair.
@@ -2859,26 +2856,55 @@ check_new_partition_bound(char *relname, Relation
parent,
spec->remainder);
if (offset < 0)
{
- next_modulus =
DatumGetInt32(datums[0][0]);
- valid_modulus = (next_modulus %
spec->modulus) == 0;
+ int
next_modulus;
+
+ /*
+ * All existing moduli are
greater or equal, so the
+ * new one must be a factor of
the smallest one, which
+ * is first in the boundinfo.
+ */
+ next_modulus =
DatumGetInt32(boundinfo->datums[0][0]);
+ if (next_modulus %
spec->modulus != 0)
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+
errmsg("every hash partition modulus must be a factor of the next larger
modulus"),
+
errdetail("The new modulus %d is not a factor of %d, the modulus of existing
partition \"%s\".",
+
spec->modulus, next_modulus,
+
get_rel_name(partdesc->oids[boundinfo->indexes[0]]))));
}
else
{
- prev_modulus =
DatumGetInt32(datums[offset][0]);
- valid_modulus = (spec->modulus
% prev_modulus) == 0;
+ int
prev_modulus;
+
+ /* We found the largest modulus
less than or equal to ours. */
+ prev_modulus =
DatumGetInt32(boundinfo->datums[offset][0]);
+
+ if (spec->modulus %
prev_modulus != 0)
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+
errmsg("every hash partition modulus must be a factor of the next larger
modulus"),
+
errdetail("The new modulus %d is not divisible by %d, the modulus of existing
partition \"%s\".",
+
spec->modulus,
+
prev_modulus,
+
get_rel_name(partdesc->oids[boundinfo->indexes[offset]]))));
- if (valid_modulus && (offset +
1) < ndatums)
+ if (offset + 1 <
boundinfo->ndatums)
{
- next_modulus =
DatumGetInt32(datums[offset + 1][0]);
- valid_modulus =
(next_modulus % spec->modulus) == 0;
+ int
next_modulus;
+
+ /* Look at the next
higher modulus */
+ next_modulus =
DatumGetInt32(boundinfo->datums[offset + 1][0]);
+
+ if (next_modulus %
spec->modulus != 0)
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+
errmsg("every hash partition modulus must be a factor of the next larger
modulus"),
+
errdetail("The new modulus %d is not factor of %d, the modulus of existing
partition \"%s\".",
+
spec->modulus, next_modulus,
+
get_rel_name(partdesc->oids[boundinfo->indexes[offset +
1]]))));
}
}
- if (!valid_modulus)
- ereport(ERROR,
-
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("every
hash partition modulus must be a factor of the next larger modulus")));
-
greatest_modulus = boundinfo->nindexes;
remainder = spec->remainder;
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index 0ce6ee4622..bb3f873f22 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4109,6 +4109,7 @@ ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR
VALUES WITH (MODULUS 8, R
ERROR: remainder for hash partition must be less than modulus
ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR VALUES WITH (MODULUS 3,
REMAINDER 2);
ERROR: every hash partition modulus must be a factor of the next larger
modulus
+DETAIL: The new modulus 3 is not a factor of 4, the modulus of existing
partition "hpart_1".
DROP TABLE fail_part;
--
-- DETACH PARTITION
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/expected/create_table.out
index ed8c01b8de..a392df2d6a 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -783,9 +783,11 @@ CREATE TABLE hpart_3 PARTITION OF hash_parted FOR VALUES
WITH (MODULUS 200, REMA
-- modulus 25 is factor of modulus of 50 but 10 is not factor of 25.
CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS 25,
REMAINDER 3);
ERROR: every hash partition modulus must be a factor of the next larger
modulus
+DETAIL: The new modulus 25 is not divisible by 10, the modulus of existing
partition "hpart_1".
-- previous modulus 50 is factor of 150 but this modulus is not factor of next
modulus 200.
CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS 150,
REMAINDER 3);
ERROR: every hash partition modulus must be a factor of the next larger
modulus
+DETAIL: The new modulus 150 is not factor of 200, the modulus of existing
partition "hpart_3".
-- trying to specify range for the hash partitioned table
CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES FROM ('a', 1) TO
('z');
ERROR: invalid bound specification for a hash partition
--
2.30.0
