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; - 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