Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8391779

Browse filesBrowse files
alvherretenderwg
alvherre
and
tenderwg
committed
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
1 parent 734c057 commit 8391779
Copy full SHA for 8391779

File tree

Expand file treeCollapse file tree

3 files changed

+95
-4
lines changed
Filter options
Expand file treeCollapse file tree

3 files changed

+95
-4
lines changed

‎src/backend/commands/tablecmds.c

Copy file name to clipboardExpand all lines: src/backend/commands/tablecmds.c
+13-4Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19281,22 +19281,31 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
1928119281
foreach(cell, indexes)
1928219282
{
1928319283
Oid idxid = lfirst_oid(cell);
19284+
Oid parentidx;
1928419285
Relation idx;
1928519286
Oid constrOid;
19287+
Oid parentConstrOid;
1928619288

1928719289
if (!has_superclass(idxid))
1928819290
continue;
1928919291

19290-
Assert((IndexGetRelation(get_partition_parent(idxid, false), false) ==
19291-
RelationGetRelid(rel)));
19292+
parentidx = get_partition_parent(idxid, false);
19293+
Assert((IndexGetRelation(parentidx, false) == RelationGetRelid(rel)));
1929219294

1929319295
idx = index_open(idxid, AccessExclusiveLock);
1929419296
IndexSetParentIndex(idx, InvalidOid);
1929519297

19296-
/* If there's a constraint associated with the index, detach it too */
19298+
/*
19299+
* If there's a constraint associated with the index, detach it too.
19300+
* Careful: it is possible for a constraint index in a partition to be
19301+
* the child of a non-constraint index, so verify whether the parent
19302+
* index does actually have a constraint.
19303+
*/
1929719304
constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel),
1929819305
idxid);
19299-
if (OidIsValid(constrOid))
19306+
parentConstrOid = get_relation_idx_constraint_oid(RelationGetRelid(rel),
19307+
parentidx);
19308+
if (OidIsValid(parentConstrOid) && OidIsValid(constrOid))
1930019309
ConstraintSetParentConstraint(constrOid, InvalidOid, InvalidOid);
1930119310

1930219311
index_close(idx, NoLock);

‎src/test/regress/expected/constraints.out

Copy file name to clipboardExpand all lines: src/test/regress/expected/constraints.out
+42Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -626,6 +626,48 @@ SELECT conname FROM pg_constraint WHERE conrelid = 'parted_fk_naming_1'::regclas
626626
(1 row)
627627

628628
DROP TABLE parted_fk_naming;
629+
--
630+
-- Test various ways to create primary keys on partitions, linked to unique
631+
-- indexes (without constraints) on the partitioned table. Ideally these should
632+
-- fail, but we don't dare change released behavior, so instead cope with it at
633+
-- DETACH time.
634+
CREATE TEMP TABLE t (a integer, b integer) PARTITION BY HASH (a, b);
635+
CREATE TEMP TABLE tp (a integer, b integer, PRIMARY KEY (a, b), UNIQUE (b, a));
636+
ALTER TABLE t ATTACH PARTITION tp FOR VALUES WITH (MODULUS 1, REMAINDER 0);
637+
CREATE UNIQUE INDEX t_a_idx ON t (a, b);
638+
CREATE UNIQUE INDEX t_b_idx ON t (b, a);
639+
ALTER INDEX t_a_idx ATTACH PARTITION tp_pkey;
640+
ALTER INDEX t_b_idx ATTACH PARTITION tp_b_a_key;
641+
SELECT conname, conparentid, conislocal, coninhcount
642+
FROM pg_constraint WHERE conname IN ('tp_pkey', 'tp_b_a_key');
643+
conname | conparentid | conislocal | coninhcount
644+
------------+-------------+------------+-------------
645+
tp_pkey | 0 | t | 0
646+
tp_b_a_key | 0 | t | 0
647+
(2 rows)
648+
649+
ALTER TABLE t DETACH PARTITION tp;
650+
DROP TABLE t, tp;
651+
CREATE TEMP TABLE t (a integer) PARTITION BY LIST (a);
652+
CREATE TEMP TABLE tp (a integer PRIMARY KEY);
653+
CREATE UNIQUE INDEX t_a_idx ON t (a);
654+
ALTER TABLE t ATTACH PARTITION tp FOR VALUES IN (1);
655+
ALTER TABLE t DETACH PARTITION tp;
656+
DROP TABLE t, tp;
657+
CREATE TEMP TABLE t (a integer) PARTITION BY LIST (a);
658+
CREATE TEMP TABLE tp (a integer PRIMARY KEY);
659+
CREATE UNIQUE INDEX t_a_idx ON ONLY t (a);
660+
ALTER TABLE t ATTACH PARTITION tp FOR VALUES IN (1);
661+
ALTER TABLE t DETACH PARTITION tp;
662+
DROP TABLE t, tp;
663+
CREATE TABLE regress_constr_partitioned (a integer) PARTITION BY LIST (a);
664+
CREATE TABLE regress_constr_partition1 PARTITION OF regress_constr_partitioned FOR VALUES IN (1);
665+
ALTER TABLE regress_constr_partition1 ADD PRIMARY KEY (a);
666+
CREATE UNIQUE INDEX ON regress_constr_partitioned (a);
667+
BEGIN;
668+
ALTER TABLE regress_constr_partitioned DETACH PARTITION regress_constr_partition1;
669+
ROLLBACK;
670+
-- Leave this one in funny state for pg_upgrade testing
629671
-- test a HOT update that invalidates the conflicting tuple.
630672
-- the trigger should still fire and catch the violation
631673
BEGIN;

‎src/test/regress/sql/constraints.sql

Copy file name to clipboardExpand all lines: src/test/regress/sql/constraints.sql
+40Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,46 @@ ALTER TABLE parted_fk_naming ATTACH PARTITION parted_fk_naming_1 FOR VALUES IN (
449449
SELECT conname FROM pg_constraint WHERE conrelid = 'parted_fk_naming_1'::regclass AND contype = 'f';
450450
DROP TABLE parted_fk_naming;
451451

452+
--
453+
-- Test various ways to create primary keys on partitions, linked to unique
454+
-- indexes (without constraints) on the partitioned table. Ideally these should
455+
-- fail, but we don't dare change released behavior, so instead cope with it at
456+
-- DETACH time.
457+
CREATE TEMP TABLE t (a integer, b integer) PARTITION BY HASH (a, b);
458+
CREATE TEMP TABLE tp (a integer, b integer, PRIMARY KEY (a, b), UNIQUE (b, a));
459+
ALTER TABLE t ATTACH PARTITION tp FOR VALUES WITH (MODULUS 1, REMAINDER 0);
460+
CREATE UNIQUE INDEX t_a_idx ON t (a, b);
461+
CREATE UNIQUE INDEX t_b_idx ON t (b, a);
462+
ALTER INDEX t_a_idx ATTACH PARTITION tp_pkey;
463+
ALTER INDEX t_b_idx ATTACH PARTITION tp_b_a_key;
464+
SELECT conname, conparentid, conislocal, coninhcount
465+
FROM pg_constraint WHERE conname IN ('tp_pkey', 'tp_b_a_key');
466+
ALTER TABLE t DETACH PARTITION tp;
467+
DROP TABLE t, tp;
468+
469+
CREATE TEMP TABLE t (a integer) PARTITION BY LIST (a);
470+
CREATE TEMP TABLE tp (a integer PRIMARY KEY);
471+
CREATE UNIQUE INDEX t_a_idx ON t (a);
472+
ALTER TABLE t ATTACH PARTITION tp FOR VALUES IN (1);
473+
ALTER TABLE t DETACH PARTITION tp;
474+
DROP TABLE t, tp;
475+
476+
CREATE TEMP TABLE t (a integer) PARTITION BY LIST (a);
477+
CREATE TEMP TABLE tp (a integer PRIMARY KEY);
478+
CREATE UNIQUE INDEX t_a_idx ON ONLY t (a);
479+
ALTER TABLE t ATTACH PARTITION tp FOR VALUES IN (1);
480+
ALTER TABLE t DETACH PARTITION tp;
481+
DROP TABLE t, tp;
482+
483+
CREATE TABLE regress_constr_partitioned (a integer) PARTITION BY LIST (a);
484+
CREATE TABLE regress_constr_partition1 PARTITION OF regress_constr_partitioned FOR VALUES IN (1);
485+
ALTER TABLE regress_constr_partition1 ADD PRIMARY KEY (a);
486+
CREATE UNIQUE INDEX ON regress_constr_partitioned (a);
487+
BEGIN;
488+
ALTER TABLE regress_constr_partitioned DETACH PARTITION regress_constr_partition1;
489+
ROLLBACK;
490+
-- Leave this one in funny state for pg_upgrade testing
491+
452492
-- test a HOT update that invalidates the conflicting tuple.
453493
-- the trigger should still fire and catch the violation
454494

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.