Further fix ALTER COLUMN TYPE's handling of indexes and index constraints.

tglsfdc · tglsfdc · commit ddfb1b2eeaec · 2019-06-24T16:43:05.000-04:00
This patch reverts all the code changes of commit e76de88, which turns out to have been seriously misguided. We can't wait till later to compute the definition string for an index; we must capture that before applying the data type change for any column it depends on, else ruleutils.c will deliverr wrong/misleading results. (This fine point was documented nowhere, of course.) I'd also managed to forget that ATExecAlterColumnType executes once per ALTER COLUMN TYPE clause, not once per statement; which resulted in the code being basically completely broken for any case in which multiple ALTER COLUMN TYPE clauses are applied to a table having non-constraint indexes that must be rebuilt. Through very bad luck, none of the existing test cases nor the ones added by e76de88 caught that, but of course it was soon found in the field. The previous patch also had an implicit assumption that if a constraint's index had a dependency on a table column, so would the constraint --- but that isn't actually true, so it didn't fix such cases. Instead of trying to delete unneeded index dependencies later, do the is-there-a-constraint lookup immediately on seeing an index dependency, and switch to remembering the constraint if so. In the unusual case of multiple column dependencies for a constraint index, this will result in duplicate constraint lookups, but that's not that horrible compared to all the other work that happens here. Besides, such cases did not work at all before, so it's hard to argue that they're performance-critical for anyone. Per bug #15865 from Keith Fiske. As before, back-patch to all supported branches. Discussion: https://postgr.es/m/15865-17940eacc8f8b081@postgresql.org
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
@@ -372,6 +372,9 @@ static void ATPrepAlterColumnType(List **wqueue,
 static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
 static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 					  AlterTableCmd *cmd, LOCKMODE lockmode);
+static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab,
+											DependencyType deptype);
+static void RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab);
 static void ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
 								List *options, LOCKMODE lockmode);
 static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode);
@@ -7787,9 +7790,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 	ScanKeyData key[3];
 	SysScanDesc scan;
 	HeapTuple	depTup;
-	ListCell   *lc;
-	ListCell   *prev;
-	ListCell   *next;
 
 	attrelation = heap_open(AttributeRelationId, RowExclusiveLock);
 
@@ -7858,11 +7858,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 	 * performed all the individual ALTER TYPE operations.  We have to save
 	 * the info before executing ALTER TYPE, though, else the deparser will
 	 * get confused.
-	 *
-	 * There could be multiple entries for the same object, so we must check
-	 * to ensure we process each one only once.  Note: we assume that an index
-	 * that implements a constraint will not show a direct dependency on the
-	 * column.
 	 */
 	depRel = heap_open(DependRelationId, RowExclusiveLock);
 
@@ -7903,20 +7898,8 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 
 					if (relKind == RELKIND_INDEX)
 					{
-						/*
-						 * Indexes that are directly dependent on the table
-						 * might be regular indexes or constraint indexes.
-						 * Constraint indexes typically have only indirect
-						 * dependencies; but there are exceptions, notably
-						 * partial exclusion constraints.  Hence we must check
-						 * whether the index depends on any constraint that's
-						 * due to be rebuilt, which we'll do below after we've
-						 * found all such constraints.
-						 */
 						Assert(foundObject.objectSubId == 0);
-						tab->changedIndexOids =
-							list_append_unique_oid(tab->changedIndexOids,
-												   foundObject.objectId);
+						RememberIndexForRebuilding(foundObject.objectId, tab);
 					}
 					else if (relKind == RELKIND_SEQUENCE)
 					{
@@ -7937,39 +7920,8 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 
 			case OCLASS_CONSTRAINT:
 				Assert(foundObject.objectSubId == 0);
-				if (!list_member_oid(tab->changedConstraintOids,
-									 foundObject.objectId))
-				{
-					char	   *defstring = pg_get_constraintdef_string(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);
-					}
-				}
+				RememberConstraintForRebuilding(foundObject.objectId, tab,
+												foundDep->deptype);
 				break;
 
 			case OCLASS_REWRITE:
@@ -8052,41 +8004,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 
 	systable_endscan(scan);
 
-	/*
-	 * Check the collected index OIDs to see which ones belong to the
-	 * constraint(s) of the table, and drop those from the list of indexes
-	 * that we need to process; rebuilding the constraints will handle them.
-	 */
-	prev = NULL;
-	for (lc = list_head(tab->changedIndexOids); lc; lc = next)
-	{
-		Oid			indexoid = lfirst_oid(lc);
-		Oid			conoid;
-
-		next = lnext(lc);
-
-		conoid = get_index_constraint(indexoid);
-		if (OidIsValid(conoid) &&
-			list_member_oid(tab->changedConstraintOids, conoid))
-			tab->changedIndexOids = list_delete_cell(tab->changedIndexOids,
-													 lc, prev);
-		else
-			prev = lc;
-	}
-
-	/*
-	 * Now collect the definitions of the indexes that must be rebuilt.  (We
-	 * could merge this into the previous loop, but it'd be more complicated
-	 * for little gain.)
-	 */
-	foreach(lc, tab->changedIndexOids)
-	{
-		Oid			indexoid = lfirst_oid(lc);
-
-		tab->changedIndexDefs = lappend(tab->changedIndexDefs,
-										pg_get_indexdef_string(indexoid));
-	}
-
 	/*
 	 * Now scan for dependencies of this column on other things.  The only
 	 * thing we should find is the dependency on the column datatype, which we
@@ -8188,6 +8105,95 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
 	heap_freetuple(heapTup);
 }
 
+/*
+ * Subroutine for ATExecAlterColumnType: remember that a constraint needs
+ * to be rebuilt (which we might already know).
+ */
+static void
+RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab,
+								DependencyType deptype)
+{
+	/*
+	 * This de-duplication check is critical for two independent reasons: we
+	 * mustn't try to recreate the same constraint twice, and if a constraint
+	 * depends on more than one column whose type is to be altered, we must
+	 * capture its definition string before applying any of the column type
+	 * changes.  ruleutils.c will get confused if we ask again later.
+	 */
+	if (!list_member_oid(tab->changedConstraintOids, conoid))
+	{
+		/* OK, capture the constraint's existing definition string */
+		char	   *defstring = pg_get_constraintdef_string(conoid);
+
+		/*
+		 * 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 (deptype == DEPENDENCY_NORMAL)
+		{
+			tab->changedConstraintOids = lcons_oid(conoid,
+												   tab->changedConstraintOids);
+			tab->changedConstraintDefs = lcons(defstring,
+											   tab->changedConstraintDefs);
+		}
+		else
+		{
+			tab->changedConstraintOids = lappend_oid(tab->changedConstraintOids,
+													 conoid);
+			tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
+												 defstring);
+		}
+	}
+}
+
+/*
+ * Subroutine for ATExecAlterColumnType: remember that an index needs
+ * to be rebuilt (which we might already know).
+ */
+static void
+RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
+{
+	/*
+	 * This de-duplication check is critical for two independent reasons: we
+	 * mustn't try to recreate the same index twice, and if an index depends
+	 * on more than one column whose type is to be altered, we must capture
+	 * its definition string before applying any of the column type changes.
+	 * ruleutils.c will get confused if we ask again later.
+	 */
+	if (!list_member_oid(tab->changedIndexOids, indoid))
+	{
+		/*
+		 * Before adding it as an index-to-rebuild, we'd better see if it
+		 * belongs to a constraint, and if so rebuild the constraint instead.
+		 * Typically this check fails, because constraint indexes normally
+		 * have only dependencies on their constraint.  But it's possible for
+		 * such an index to also have direct dependencies on table columns,
+		 * for example with a partial exclusion constraint.
+		 */
+		Oid			conoid = get_index_constraint(indoid);
+
+		if (OidIsValid(conoid))
+		{
+			/* index dependencies on columns should generally be AUTO */
+			RememberConstraintForRebuilding(conoid, tab, DEPENDENCY_AUTO);
+		}
+		else
+		{
+			/* OK, capture the index's existing definition string */
+			char	   *defstring = pg_get_indexdef_string(indoid);
+
+			tab->changedIndexOids = lappend_oid(tab->changedIndexOids,
+												indoid);
+			tab->changedIndexDefs = lappend(tab->changedIndexDefs,
+											defstring);
+		}
+	}
+}
+
 static void
 ATExecAlterColumnGenericOptions(Relation rel,
 								const char *colName,
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
@@ -1899,12 +1899,19 @@ select * from anothertab;
 (3 rows)
 
 drop table anothertab;
--- Test alter table column type with constraint indexes (cf. bug #15835)
-create table anothertab(f1 int primary key, f2 int unique, f3 int, f4 int);
+-- Test index handling in alter table column type (cf. bugs #15835, #15865)
+create table anothertab(f1 int primary key, f2 int unique,
+                        f3 int, f4 int, f5 int);
 alter table anothertab
   add exclude using btree (f3 with =);
 alter table anothertab
   add exclude using btree (f4 with =) where (f4 is not null);
+alter table anothertab
+  add exclude using btree (f4 with =) where (f5 > 0);
+alter table anothertab
+  add unique(f1,f4);
+create index on anothertab(f2,f3);
+create unique index on anothertab(f4);
 \d anothertab
   Table "public.anothertab"
  Column |  Type   | Modifiers 
@@ -1913,17 +1920,23 @@ alter table anothertab
  f2     | integer | 
  f3     | integer | 
  f4     | integer | 
+ f5     | integer | 
 Indexes:
     "anothertab_pkey" PRIMARY KEY, btree (f1)
+    "anothertab_f1_f4_key" UNIQUE CONSTRAINT, btree (f1, f4)
     "anothertab_f2_key" UNIQUE CONSTRAINT, btree (f2)
+    "anothertab_f4_idx" UNIQUE, btree (f4)
+    "anothertab_f2_f3_idx" btree (f2, f3)
     "anothertab_f3_excl" EXCLUDE USING btree (f3 WITH =)
     "anothertab_f4_excl" EXCLUDE USING btree (f4 WITH =) WHERE (f4 IS NOT NULL)
+    "anothertab_f4_excl1" EXCLUDE USING btree (f4 WITH =) WHERE (f5 > 0)
 
 alter table anothertab alter column f1 type bigint;
 alter table anothertab
   alter column f2 type bigint,
   alter column f3 type bigint,
   alter column f4 type bigint;
+alter table anothertab alter column f5 type bigint;
 \d anothertab
   Table "public.anothertab"
  Column |  Type  | Modifiers 
@@ -1932,11 +1945,16 @@ alter table anothertab
  f2     | bigint | 
  f3     | bigint | 
  f4     | bigint | 
+ f5     | bigint | 
 Indexes:
     "anothertab_pkey" PRIMARY KEY, btree (f1)
+    "anothertab_f1_f4_key" UNIQUE CONSTRAINT, btree (f1, f4)
     "anothertab_f2_key" UNIQUE CONSTRAINT, btree (f2)
+    "anothertab_f4_idx" UNIQUE, btree (f4)
+    "anothertab_f2_f3_idx" btree (f2, f3)
     "anothertab_f3_excl" EXCLUDE USING btree (f3 WITH =)
     "anothertab_f4_excl" EXCLUDE USING btree (f4 WITH =) WHERE (f4 IS NOT NULL)
+    "anothertab_f4_excl1" EXCLUDE USING btree (f4 WITH =) WHERE (f5 > 0)
 
 drop table anothertab;
 create table another (f1 int, f2 text);
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
@@ -1292,19 +1292,27 @@ select * from anothertab;
 
 drop table anothertab;
 
--- Test alter table column type with constraint indexes (cf. bug #15835)
-create table anothertab(f1 int primary key, f2 int unique, f3 int, f4 int);
+-- Test index handling in alter table column type (cf. bugs #15835, #15865)
+create table anothertab(f1 int primary key, f2 int unique,
+                        f3 int, f4 int, f5 int);
 alter table anothertab
   add exclude using btree (f3 with =);
 alter table anothertab
   add exclude using btree (f4 with =) where (f4 is not null);
+alter table anothertab
+  add exclude using btree (f4 with =) where (f5 > 0);
+alter table anothertab
+  add unique(f1,f4);
+create index on anothertab(f2,f3);
+create unique index on anothertab(f4);
 
 \d anothertab
 alter table anothertab alter column f1 type bigint;
 alter table anothertab
   alter column f2 type bigint,
   alter column f3 type bigint,
   alter column f4 type bigint;
+alter table anothertab alter column f5 type bigint;
 \d anothertab
 
 drop table anothertab;
