I had a bit of trouble parsing the error message "every hash partition
modulus must be a factor of the next larger modulus", so I went into the
code, added some comments and added errdetail messages for each case. I
think it's a bit clearer now.
From e7f392e2f8950236a22f007cc3aed36729da22e1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 2 Feb 2021 11:21:21 +0100
Subject: [PATCH] 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.
---
src/backend/partitioning/partbounds.c | 58 +++++++++++++++-------
src/test/regress/expected/alter_table.out | 1 +
src/test/regress/expected/create_table.out | 2 +
3 files changed, 43 insertions(+), 18 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c
b/src/backend/partitioning/partbounds.c
index 0c3f212ff2..7425e34040 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,51 @@ 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 the existing modulus %d.",
+
spec->modulus, next_modulus)));
}
else
{
- prev_modulus =
DatumGetInt32(datums[offset][0]);
- valid_modulus = (spec->modulus
% prev_modulus) == 0;
+ int
prev_modulus;
- if (valid_modulus && (offset +
1) < ndatums)
+ /* 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 existing modulus %d is not a factor of the new modulus %d.",
+
prev_modulus, spec->modulus)));
+
+ 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 the existing modulus %d.",
+
spec->modulus, next_modulus)));
}
}
- 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..62bf27fc50 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 the existing modulus 4.
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..95d8ed4c45 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 existing modulus 10 is not a factor of the new modulus 25.
-- 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 the existing modulus 200.
-- 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
