Fix bogus logic for reporting which hash partition conflicts.

Commit efbfb6424 added logic for reporting exactly which existing
partition conflicts when complaining that a new hash partition's
modulus isn't compatible with the existing ones.  However, it
misunderstood the partitioning data structure, and would select
the wrong partition in some cases, or crash outright due to fetching
a bogus table OID in other cases.

Per bug #17076 from Alexander Lakhin.  Fix by Amit Langote;
some further work on the code comments by me.

Discussion: https://postgr.es/m/17076-89a16ae835d329b9@postgresql.org

diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7925fcce3b355e68fec3188db4366c62ba1fa921..7096d3bf4504ab36b1c2e1e92307d54f52c592e0 100644 (file)
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -2838,12 +2838,15 @@ check_new_partition_bound(char *relname, Relation parent,
 
                    /*
                     * Check rule that every modulus must be a factor of the
-                    * next larger modulus.  For example, if you have a bunch
+                    * next larger modulus.  (For example, if you have a bunch
                     * of partitions that all have modulus 5, you can add a
                     * new partition with modulus 10 or a new partition with
                     * 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.
+                    * is not a factor of 15.)  We need only check the next
+                    * smaller and next larger existing moduli, relying on
+                    * previous enforcement of this rule to be sure that the
+                    * rest are in line.
                     */
 
                    /*
@@ -2870,15 +2873,16 @@ check_new_partition_bound(char *relname, Relation parent,
                                     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]]))));
+                                              get_rel_name(partdesc->oids[0]))));
                    }
                    else
                    {
                        int         prev_modulus;
 
                        /*
-                        * We found the largest modulus less than or equal to
-                        * ours.
+                        * We found the largest (modulus, remainder) pair less
+                        * than or equal to the new one.  That modulus must be
+                        * a divisor of, or equal to, the new modulus.
                         */
                        prev_modulus = DatumGetInt32(boundinfo->datums[offset][0]);
 
@@ -2889,13 +2893,19 @@ check_new_partition_bound(char *relname, Relation parent,
                                     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]]))));
+                                              get_rel_name(partdesc->oids[offset]))));
 
                        if (offset + 1 < boundinfo->ndatums)
                        {
                            int         next_modulus;
 
-                           /* Look at the next higher modulus */
+                           /*
+                            * Look at the next higher (modulus, remainder)
+                            * pair.  That could have the same modulus and a
+                            * larger remainder than the new pair, in which
+                            * case we're good.  If it has a larger modulus,
+                            * the new modulus must divide that one.
+                            */
                            next_modulus = DatumGetInt32(boundinfo->datums[offset + 1][0]);
 
                            if (next_modulus % spec->modulus != 0)
@@ -2904,7 +2914,7 @@ check_new_partition_bound(char *relname, Relation parent,
                                         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[offset + 1]]))));
+                                                  get_rel_name(partdesc->oids[offset + 1]))));
                        }
                    }
 

diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index ad89dd05c142e7efd1df778ba60a3ada2a6bb7a3..96bf426d98ee21e380d346cf3941d44152a37fd1 100644 (file)
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -780,14 +780,20 @@ CREATE TABLE hash_parted (
 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);
+CREATE TABLE hpart_4 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 10, REMAINDER 3);
 -- modulus 25 is factor of modulus of 50 but 10 is not a 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".
+DETAIL:  The new modulus 25 is not divisible by 10, the modulus of existing partition "hpart_4".
 -- previous modulus 50 is factor of 150 but this modulus is not a 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 a factor of 200, the modulus of existing partition "hpart_3".
+-- overlapping remainders
+CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS 100, REMAINDER 3);
+ERROR:  partition "fail_part" would overlap partition "hpart_4"
+LINE 1: ...BLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODU...
+                                                             ^
 -- 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
@@ -1100,7 +1106,7 @@ Number of partitions: 3 (Use \d+ to list them.)
 --------+---------+-----------+----------+---------
  a      | integer |           |          | 
 Partition key: HASH (a)
-Number of partitions: 3 (Use \d+ to list them.)
+Number of partitions: 4 (Use \d+ to list them.)
 
 -- check that we get the expected partition constraints
 CREATE TABLE range_parted4 (a int, b int, c int) PARTITION BY RANGE (abs(a), abs(b), c);

diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 54cbf6c0595f05ac62aa880f1d3eb58d5bfed3c4..cc41f58ba22a73e82c220c3666a4fdb5834cf59a 100644 (file)
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -631,10 +631,13 @@ CREATE TABLE hash_parted (
 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);
+CREATE TABLE hpart_4 PARTITION OF hash_parted FOR VALUES WITH (MODULUS 10, REMAINDER 3);
 -- modulus 25 is factor of modulus of 50 but 10 is not a factor of 25.
 CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS 25, REMAINDER 3);
 -- previous modulus 50 is factor of 150 but this modulus is not a factor of next modulus 200.
 CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS 150, REMAINDER 3);
+-- overlapping remainders
+CREATE TABLE fail_part PARTITION OF hash_parted FOR VALUES WITH (MODULUS 100, REMAINDER 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');
 -- trying to specify list value for the hash partitioned table