Re: Improve new hash partition bound check error messages
On 15.02.21 17:45, Peter Eisentraut wrote: 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. committed
Re: Improve new hash partition bound check error messages
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
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;
- boolvalid_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),
+
errms
Re: Improve new hash partition bound check error messages
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.
Re: Improve new hash partition bound check error messages
On 02/02/2021 12:35, Peter Eisentraut wrote: 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. Yeah, that error message is hard to understand. This is an improvement, but it still took me a while to understand it. Let's look at the example in the regression test: -- check partition bound syntax for the hash partition CREATE TABLE hash_parted ( a int ) PARTITION BY HASH (a); CREATE TABLE hpart_1 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 10, REMAINDER 0); CREATE TABLE hpart_2 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 50, REMAINDER 1); CREATE TABLE hpart_3 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 200, REMAINDER 2); With this patch, you get 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: The existing modulus 10 is not a factor of the new modulus 25. 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. 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" 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: 150 is not a factor of 200, the modulus of existing partition "hpart_3" Calling the existing partition by name seems good. And this phrasing puts the focus on the new modulus in both variants; presumably the existing partition is OK and the problem is in the new definition. - Heikki
Re: Improve new hash partition bound check error messages
On Tue, Feb 2, 2021 at 7:36 PM Peter Eisentraut wrote: > 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. That is definitely an improvement, thanks. -- Amit Langote EDB: http://www.enterprisedb.com
Improve new hash partition bound check error messages
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
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;
- boolvalid_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]);
-
