postgres.git / summary / log / commit / refs
commit 66aaa7a71841f0dda4c5e2918d4a7fe371c4b701
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri Jul 12 10:54:01 2024 +0000
Fix ALTER TABLE DETACH for inconsistent indexes
When a partitioned table has an index that doesn't support a constraint,
but a partition has an equivalent index that does, then a DETACH
operation would misbehave: a crash in assertion-enabled systems (because
we fail to find the constraint in the parent that we expect to), or a
broken coninhcount value (-1) in production systems (because we blindly
believe that we've successfully detached the parent).
While we should reject an ATTACH of a partition with such an index, we
have failed to do so in existing releases, so adding an error in stable
releases might break the (unlikely) existing applications that rely on
this behavior. At this point I don't even want to reject them in
master, because it'd break pg_upgrade if such databases exist, and there
would be no easy way to fix existing databases without expensive index
rebuilds.
(Later on we could add ALTER TABLE ... ADD CONSTRAINT USING INDEX to
partitioned tables, which would allow the user to fix such patterns. At
that point we could add more restrictions to prevent the problem from
its root.)
Also, add a test case that leaves one table in this condition, so that
we can verify that pg_upgrade continues to work if we later decide to
change the policy on the master branch.
Backpatch to all supported branches.
Co-authored-by: Tender Wang <tndrwang@gmail.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/18500-62948b6fe5522f56@postgresql.org
src/backend/commands/tablecmds.c | 17 +++++++++---
src/test/regress/expected/sanity_check.out | 2 ++
src/test/regress/input/constraints.source | 40 ++++++++++++++++++++++++++++
src/test/regress/output/constraints.source | 42 ++++++++++++++++++++++++++++++
4 files changed, 97 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6448c596a85..8fd6e6bd408 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18209,22 +18209,31 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
foreach(cell, indexes)
{
Oid idxid = lfirst_oid(cell);
+ Oid parentidx;
Relation idx;
Oid constrOid;
+ Oid parentConstrOid;
if (!has_superclass(idxid))
continue;
- Assert((IndexGetRelation(get_partition_parent(idxid, false), false) ==
- RelationGetRelid(rel)));
+ parentidx = get_partition_parent(idxid, false);
+ Assert((IndexGetRelation(parentidx, false) == RelationGetRelid(rel)));
idx = index_open(idxid, AccessExclusiveLock);
IndexSetParentIndex(idx, InvalidOid);
- /* If there's a constraint associated with the index, detach it too */
+ /*
+ * If there's a constraint associated with the index, detach it too.
+ * Careful: it is possible for a constraint index in a partition to be
+ * the child of a non-constraint index, so verify whether the parent
+ * index does actually have a constraint.
+ */
constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel),
idxid);
- if (OidIsValid(constrOid))
+ parentConstrOid = get_relation_idx_constraint_oid(RelationGetRelid(rel),
+ parentidx);
+ if (OidIsValid(parentConstrOid) && OidIsValid(constrOid))
ConstraintSetParentConstraint(constrOid, InvalidOid, InvalidOid);
index_close(idx, NoLock);
diff --git a/src/test/regress/expected/sanity_check.out b/src/test/regress/expected/sanity_check.out
index 99e7bb4886b..51b129c96cf 100644
--- a/src/test/regress/expected/sanity_check.out
+++ b/src/test/regress/expected/sanity_check.out
@@ -173,6 +173,8 @@ quad_poly_tbl|t
radix_text_tbl|t
ramp|f
real_city|f
+regress_constr_partition1|t
+regress_constr_partitioned|t
reservations|f
road|t
shighway|t
diff --git a/src/test/regress/input/constraints.source b/src/test/regress/input/constraints.source
index 8fe9754904c..6e216e665a8 100644
--- a/src/test/regress/input/constraints.source
+++ b/src/test/regress/input/constraints.source
@@ -429,6 +429,46 @@ ALTER TABLE parted_fk_naming ATTACH PARTITION parted_fk_naming_1 FOR VALUES IN (
SELECT conname FROM pg_constraint WHERE conrelid = 'parted_fk_naming_1'::regclass AND contype = 'f';
DROP TABLE parted_fk_naming;
+--
+-- Test various ways to create primary keys on partitions, linked to unique
+-- indexes (without constraints) on the partitioned table. Ideally these should
+-- fail, but we don't dare change released behavior, so instead cope with it at
+-- DETACH time.
+CREATE TEMP TABLE t (a integer, b integer) PARTITION BY HASH (a, b);
+CREATE TEMP TABLE tp (a integer, b integer, PRIMARY KEY (a, b), UNIQUE (b, a));
+ALTER TABLE t ATTACH PARTITION tp FOR VALUES WITH (MODULUS 1, REMAINDER 0);
+CREATE UNIQUE INDEX t_a_idx ON t (a, b);
+CREATE UNIQUE INDEX t_b_idx ON t (b, a);
+ALTER INDEX t_a_idx ATTACH PARTITION tp_pkey;
+ALTER INDEX t_b_idx ATTACH PARTITION tp_b_a_key;
+ALTER TABLE t DETACH PARTITION tp;
+SELECT conname, conparentid, conislocal, coninhcount
+ FROM pg_constraint WHERE conname IN ('tp_pkey', 'tp_b_a_key');
+DROP TABLE t, tp;
+
+CREATE TEMP TABLE t (a integer) PARTITION BY LIST (a);
+CREATE TEMP TABLE tp (a integer PRIMARY KEY);
+CREATE UNIQUE INDEX t_a_idx ON t (a);
+ALTER TABLE t ATTACH PARTITION tp FOR VALUES IN (1);
+ALTER TABLE t DETACH PARTITION tp;
+DROP TABLE t, tp;
+
+CREATE TEMP TABLE t (a integer) PARTITION BY LIST (a);
+CREATE TEMP TABLE tp (a integer PRIMARY KEY);
+CREATE UNIQUE INDEX t_a_idx ON ONLY t (a);
+ALTER TABLE t ATTACH PARTITION tp FOR VALUES IN (1);
+ALTER TABLE t DETACH PARTITION tp;
+DROP TABLE t, tp;
+
+CREATE TABLE regress_constr_partitioned (a integer) PARTITION BY LIST (a);
+CREATE TABLE regress_constr_partition1 PARTITION OF regress_constr_partitioned FOR VALUES IN (1);
+ALTER TABLE regress_constr_partition1 ADD PRIMARY KEY (a);
+CREATE UNIQUE INDEX ON regress_constr_partitioned (a);
+BEGIN;
+ALTER TABLE regress_constr_partitioned DETACH PARTITION regress_constr_partition1;
+ROLLBACK;
+-- Leave this one in funny state for pg_upgrade testing
+
-- test a HOT update that invalidates the conflicting tuple.
-- the trigger should still fire and catch the violation
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index 28e3f2c717b..4e0bddb6ea0 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -598,6 +598,48 @@ SELECT conname FROM pg_constraint WHERE conrelid = 'parted_fk_naming_1'::regclas
(1 row)
DROP TABLE parted_fk_naming;
+--
+-- Test various ways to create primary keys on partitions, linked to unique
+-- indexes (without constraints) on the partitioned table. Ideally these should
+-- fail, but we don't dare change released behavior, so instead cope with it at
+-- DETACH time.
+CREATE TEMP TABLE t (a integer, b integer) PARTITION BY HASH (a, b);
+CREATE TEMP TABLE tp (a integer, b integer, PRIMARY KEY (a, b), UNIQUE (b, a));
+ALTER TABLE t ATTACH PARTITION tp FOR VALUES WITH (MODULUS 1, REMAINDER 0);
+CREATE UNIQUE INDEX t_a_idx ON t (a, b);
+CREATE UNIQUE INDEX t_b_idx ON t (b, a);
+ALTER INDEX t_a_idx ATTACH PARTITION tp_pkey;
+ALTER INDEX t_b_idx ATTACH PARTITION tp_b_a_key;
+ALTER TABLE t DETACH PARTITION tp;
+SELECT conname, conparentid, conislocal, coninhcount
+ FROM pg_constraint WHERE conname IN ('tp_pkey', 'tp_b_a_key');
+ conname | conparentid | conislocal | coninhcount
+------------+-------------+------------+-------------
+ tp_pkey | 0 | t | 0
+ tp_b_a_key | 0 | t | 0
+(2 rows)
+
+DROP TABLE t, tp;
+CREATE TEMP TABLE t (a integer) PARTITION BY LIST (a);
+CREATE TEMP TABLE tp (a integer PRIMARY KEY);
+CREATE UNIQUE INDEX t_a_idx ON t (a);
+ALTER TABLE t ATTACH PARTITION tp FOR VALUES IN (1);
+ALTER TABLE t DETACH PARTITION tp;
+DROP TABLE t, tp;
+CREATE TEMP TABLE t (a integer) PARTITION BY LIST (a);
+CREATE TEMP TABLE tp (a integer PRIMARY KEY);
+CREATE UNIQUE INDEX t_a_idx ON ONLY t (a);
+ALTER TABLE t ATTACH PARTITION tp FOR VALUES IN (1);
+ALTER TABLE t DETACH PARTITION tp;
+DROP TABLE t, tp;
+CREATE TABLE regress_constr_partitioned (a integer) PARTITION BY LIST (a);
+CREATE TABLE regress_constr_partition1 PARTITION OF regress_constr_partitioned FOR VALUES IN (1);
+ALTER TABLE regress_constr_partition1 ADD PRIMARY KEY (a);
+CREATE UNIQUE INDEX ON regress_constr_partitioned (a);
+BEGIN;
+ALTER TABLE regress_constr_partitioned DETACH PARTITION regress_constr_partition1;
+ROLLBACK;
+-- Leave this one in funny state for pg_upgrade testing
-- test a HOT update that invalidates the conflicting tuple.
-- the trigger should still fire and catch the violation
BEGIN;
[parent: 27831127d641]