On 2018-Sep-13, Tom Lane wrote:
> Alvaro Herrera <alvhe...@2ndquadrant.com> writes:
> > That's the problem all right. The solution is to drop all
> > index/constraint objects together in one performMultipleDeletions()
> > instead of performDeletion() one by one, as in the attached patch.
>
> Looks reasonable as far as it goes. Given that we no longer require
> any of this:
>
> - * Now we can drop the existing constraints and indexes --- constraints
> - * first, since some of them might depend on the indexes. In fact, we
> - * have to delete FOREIGN KEY constraints before UNIQUE constraints, but
> - * we already ordered the constraint list to ensure that would happen.
>
> can we make any simplifications in earlier steps? At the very least,
> look for comments related to this assumption.
Ah, I had looked, but not hard enough. In this new version I removed
some code in ATExecAlterColumnType that's now irrelevant. I tested this
by changing both lappend calls to lcons in that function; seems to
behave the same. (Also added more constraints to the test case.)
Another thing I found I can change is to move the add_object_address()
calls to the other loops scanning the same lists, so that we don't have
to walk each the two lists twice.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 9a9fe65eeb0bc3074793474b9d85b10c3e260e78 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Thu, 13 Sep 2018 13:26:18 -0300
Subject: [PATCH v2] fix ALTER TYPE
---
src/backend/commands/tablecmds.c | 71 +++++++++++--------------------
src/test/regress/expected/foreign_key.out | 12 ++++++
src/test/regress/sql/foreign_key.sql | 11 +++++
3 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e96512e051..03c0b739b3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9527,33 +9527,12 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation
rel,
{
char *defstring =
pg_get_constraintdef_command(foundObject.objectId);
- /*
- * Put NORMAL dependencies at the front
of the list and
- * AUTO dependencies at the back. This
makes sure that
- * foreign-key constraints depending on
this column will
- * be dropped before unique or
primary-key constraints of
- * the column; which we must have
because the FK
- * constraints depend on the indexes
belonging to the
- * unique constraints.
- */
- if (foundDep->deptype ==
DEPENDENCY_NORMAL)
- {
- tab->changedConstraintOids =
-
lcons_oid(foundObject.objectId,
-
tab->changedConstraintOids);
- tab->changedConstraintDefs =
- lcons(defstring,
-
tab->changedConstraintDefs);
- }
- else
- {
- tab->changedConstraintOids =
-
lappend_oid(tab->changedConstraintOids,
-
foundObject.objectId);
- tab->changedConstraintDefs =
-
lappend(tab->changedConstraintDefs,
-
defstring);
- }
+ tab->changedConstraintOids =
+
lappend_oid(tab->changedConstraintOids,
+
foundObject.objectId);
+ tab->changedConstraintDefs =
+
lappend(tab->changedConstraintDefs,
+ defstring);
}
break;
@@ -9893,10 +9872,18 @@ static void
ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
{
ObjectAddress obj;
+ ObjectAddresses *objects;
ListCell *def_item;
ListCell *oid_item;
/*
+ * Collect all the constraints and indexes to drop so we can process
them
+ * in a single call. That way we don't have to worry about dependencies
+ * among them.
+ */
+ objects = new_object_addresses();
+
+ /*
* Re-parse the index and constraint definitions, and attach them to the
* appropriate work queue entries. We do this before dropping because
in
* the case of a FOREIGN KEY constraint, we might not yet have exclusive
@@ -9937,6 +9924,9 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo
*tab, LOCKMODE lockmode)
conislocal = con->conislocal;
ReleaseSysCache(tup);
+ ObjectAddressSet(obj, ConstraintRelationId,
lfirst_oid(oid_item));
+ add_exact_object_address(&obj, objects);
+
/*
* If the constraint is inherited (only), we don't want to
inject a
* new definition here; it'll get recreated when
ATAddCheckConstraint
@@ -9960,31 +9950,18 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo
*tab, LOCKMODE lockmode)
ATPostAlterTypeParse(oldId, relid, InvalidOid,
(char *)
lfirst(def_item),
wqueue, lockmode,
tab->rewrite);
+
+ ObjectAddressSet(obj, RelationRelationId, lfirst_oid(oid_item));
+ add_exact_object_address(&obj, objects);
}
/*
- * Now we can drop the existing constraints and indexes --- constraints
- * first, since some of them might depend on the indexes. In fact, we
- * have to delete FOREIGN KEY constraints before UNIQUE constraints, but
- * we already ordered the constraint list to ensure that would happen.
It
- * should be okay to use DROP_RESTRICT here, since nothing else should
be
- * depending on these objects.
+ * It should be okay to use DROP_RESTRICT here, since nothing else
should
+ * be depending on these objects.
*/
- foreach(oid_item, tab->changedConstraintOids)
- {
- obj.classId = ConstraintRelationId;
- obj.objectId = lfirst_oid(oid_item);
- obj.objectSubId = 0;
- performDeletion(&obj, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);
- }
+ performMultipleDeletions(objects, DROP_RESTRICT,
PERFORM_DELETION_INTERNAL);
- foreach(oid_item, tab->changedIndexOids)
- {
- obj.classId = RelationRelationId;
- obj.objectId = lfirst_oid(oid_item);
- obj.objectSubId = 0;
- performDeletion(&obj, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);
- }
+ free_object_addresses(objects);
/*
* The objects will get recreated during subsequent passes over the work
diff --git a/src/test/regress/expected/foreign_key.out
b/src/test/regress/expected/foreign_key.out
index b90c4926e2..fc3bbe4deb 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1518,6 +1518,18 @@ DETAIL: Key (a, b)=(2500, 2502) is still referenced
from table "fk_partitioned_
ALTER TABLE fk_partitioned_fk DROP CONSTRAINT fk_partitioned_fk_a_fkey;
-- done.
DROP TABLE fk_notpartitioned_pk, fk_partitioned_fk;
+-- Altering a type referenced by a foreign key needs to drop/recreate the FK.
+-- Ensure that works.
+CREATE TABLE fk_notpartitioned_pk (a INT, PRIMARY KEY(a), CHECK (a > 0));
+CREATE TABLE fk_partitioned_fk (a INT REFERENCES fk_notpartitioned_pk(a)
PRIMARY KEY) PARTITION BY RANGE(a);
+CREATE TABLE fk_partitioned_fk_1 PARTITION OF fk_partitioned_fk FOR VALUES
FROM (MINVALUE) TO (MAXVALUE);
+INSERT INTO fk_notpartitioned_pk VALUES (1);
+INSERT INTO fk_partitioned_fk VALUES (1);
+ALTER TABLE fk_notpartitioned_pk ALTER COLUMN a TYPE bigint;
+DELETE FROM fk_notpartitioned_pk WHERE a = 1;
+ERROR: update or delete on table "fk_notpartitioned_pk" violates foreign key
constraint "fk_partitioned_fk_a_fkey" on table "fk_partitioned_fk"
+DETAIL: Key (a)=(1) is still referenced from table "fk_partitioned_fk".
+DROP TABLE fk_notpartitioned_pk, fk_partitioned_fk;
-- Test some other exotic foreign key features: MATCH SIMPLE, ON UPDATE/DELETE
-- actions
CREATE TABLE fk_notpartitioned_pk (a int, b int, primary key (a, b));
diff --git a/src/test/regress/sql/foreign_key.sql
b/src/test/regress/sql/foreign_key.sql
index f3e7058583..d2cecdf4eb 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -1139,6 +1139,17 @@ ALTER TABLE fk_partitioned_fk DROP CONSTRAINT
fk_partitioned_fk_a_fkey;
-- done.
DROP TABLE fk_notpartitioned_pk, fk_partitioned_fk;
+-- Altering a type referenced by a foreign key needs to drop/recreate the FK.
+-- Ensure that works.
+CREATE TABLE fk_notpartitioned_pk (a INT, PRIMARY KEY(a), CHECK (a > 0));
+CREATE TABLE fk_partitioned_fk (a INT REFERENCES fk_notpartitioned_pk(a)
PRIMARY KEY) PARTITION BY RANGE(a);
+CREATE TABLE fk_partitioned_fk_1 PARTITION OF fk_partitioned_fk FOR VALUES
FROM (MINVALUE) TO (MAXVALUE);
+INSERT INTO fk_notpartitioned_pk VALUES (1);
+INSERT INTO fk_partitioned_fk VALUES (1);
+ALTER TABLE fk_notpartitioned_pk ALTER COLUMN a TYPE bigint;
+DELETE FROM fk_notpartitioned_pk WHERE a = 1;
+DROP TABLE fk_notpartitioned_pk, fk_partitioned_fk;
+
-- Test some other exotic foreign key features: MATCH SIMPLE, ON UPDATE/DELETE
-- actions
CREATE TABLE fk_notpartitioned_pk (a int, b int, primary key (a, b));
--
2.11.0
