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 and existing partition involved.  Also comment the
code more.

Reviewed-by: Amit Langote <amitlangote09@gmail.com>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/flat/bb9d60b4-aadb-607a-1a9d-fdc3434dddcd%40enterprisedb.com

diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index a60c379725d1d50390548bbbf01073662c6ab2cc..398a83f74cdf96f1ed031fbd53a93ed700ab3b36 100644 (file)
--- 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 0ce6ee4622d4bfc49e732b81a8953995ee5896af..bb3f873f22a83eaad47300518a8df3679b28b3c2 100644 (file)
--- 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 ed8c01b8decc99e4012e6cc6eaa7fb0305cc8d33..a392df2d6a5068f2d365b078c6257ee9a05dc501 100644 (file)
--- 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