From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Improve new hash partition bound check error messages
Date: Mon, 15 Feb 2021 17:45:51 +0100
Message-ID: <b7a09729-7815-4bf5-c74c-211ac4997688@enterprisedb.com> (raw)
In-Reply-To: <aeb41d57-da42-b46d-bfc1-4933afbc5658@enterprisedb.com>
References: <bb9d60b4-aadb-607a-1a9d-fdc3434dddcd@enterprisedb.com>
<9fc7eb48-0e8f-0836-8def-bc4b242e9c63@iki.fi>
<aeb41d57-da42-b46d-bfc1-4933afbc5658@enterprisedb.com>
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.
Attachments:v2-0001-Improve-new-hash-partition-bound-check-error-mess.patch (text/plain, 6.6K) download
From 537e8f2f27e43b94777b962e408245fb1d4784dd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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.cindex 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.outindex 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.outindex 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
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: peter.eisentraut@enterprisedb.com, hlinnaka@iki.fi
Subject: Re: Improve new hash partition bound check error messages
In-Reply-To: <b7a09729-7815-4bf5-c74c-211ac4997688@enterprisedb.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