Re: BUG #15865: ALTER TABLE statements causing "relation already exists" errors when some indexes exist

I wrote:
>> Yeah, obviously I fat-fingered something there.  Looking ...

After further review it seems like I was led into this error by a siren
singing something about how we could skip collecting the index definition
string for an index we were going to ignore later.  (Cue standard lecture
about premature optimization...)  That absolutely *does not* work, because
we might not find out till we're considering some later ALTER TYPE
subcommand that the index depends on a relevant constraint.  And we have
to capture the index definition before we alter the type of any column it
depends on, or pg_get_indexdef_string will get very confused.  That little
dependency wasn't documented anywhere.  I also found a pre-existing
comment that contradicted the new reality but I'd missed removing in
e76de8861.

Here's a patch against HEAD --- since I'm feeling more mortal than usual
right now, I'll put this out for review rather than just pushing it.
It might be easier to review the code changes by just ignoring e76de8861
and diffing against tablecmds.c from before that, as I've done in the
second attachment.

BTW, has anyone got an explanation for the order in which psql is
listing the indexes of "anothertab" in this test case?

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cb2c5e1..83dc6df 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10508,9 +10508,6 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
     SysScanDesc scan;
     HeapTuple    depTup;
     ObjectAddress address;
-    ListCell   *lc;
-    ListCell   *prev;
-    ListCell   *next;

     /*
      * Clear all the missing values if we're rewriting the table, since this
@@ -10603,11 +10600,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 = table_open(DependRelationId, RowExclusiveLock);

@@ -10650,19 +10642,21 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
                         relKind == RELKIND_PARTITIONED_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.
+                         * This de-duplication check is critical for two
+                         * independent reasons: we mustn't try to create 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.
                          */
                         Assert(foundObject.objectSubId == 0);
-                        tab->changedIndexOids =
-                            list_append_unique_oid(tab->changedIndexOids,
-                                                   foundObject.objectId);
+                        if (!list_member_oid(tab->changedIndexOids, foundObject.objectId))
+                        {
+                            tab->changedIndexOids = lappend_oid(tab->changedIndexOids,
+                                                                foundObject.objectId);
+                            tab->changedIndexDefs = lappend(tab->changedIndexDefs,
+                                                            pg_get_indexdef_string(foundObject.objectId));
+                        }
                     }
                     else if (relKind == RELKIND_SEQUENCE)
                     {
@@ -10697,6 +10691,12 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
                 }

             case OCLASS_CONSTRAINT:
+
+                /*
+                 * As with indexes, de-duplication is critical, and it's
+                 * needed because the same constraint could depend on multiple
+                 * target columns.
+                 */
                 Assert(foundObject.objectSubId == 0);
                 if (!list_member_oid(tab->changedConstraintOids,
                                      foundObject.objectId))
@@ -10829,41 +10829,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
      * want to remove, possibly a collation dependency, and dependencies on
@@ -11242,8 +11207,25 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
             def_item, tab->changedIndexDefs)
     {
         Oid            oldId = lfirst_oid(oid_item);
+        Oid            conoid;
         Oid            relid;

+        /*
+         * We must check each index to see if it belongs to a constraint that
+         * we already processed above.  Typically this check does not fire
+         * because constraint indexes normally have only dependencies on their
+         * constraint, and thus would not have been found by the dependency
+         * scan in ATExecAlterColumnType.  But it's possible for such an index
+         * to also have direct dependencies on the table, for example with a
+         * partial exclusion constraint.  In that case we'd have list entries
+         * for both the index and the constraint, and we must disregard the
+         * one for the index.
+         */
+        conoid = get_index_constraint(oldId);
+        if (OidIsValid(conoid) &&
+            list_member_oid(tab->changedConstraintOids, conoid))
+            continue;
+
         relid = IndexGetRelation(oldId, false);
         ATPostAlterTypeParse(oldId, relid, InvalidOid,
                              (char *) lfirst(def_item),
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index c845a16..01ba41c 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1906,6 +1906,10 @@ 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 unique(f3,f4);
+create index on anothertab(f2,f3);
+create unique index on anothertab(f4);
 \d anothertab
              Table "public.anothertab"
  Column |  Type   | Collation | Nullable | Default
@@ -1917,6 +1921,9 @@ alter table anothertab
 Indexes:
     "anothertab_pkey" PRIMARY KEY, btree (f1)
     "anothertab_f2_key" UNIQUE CONSTRAINT, btree (f2)
+    "anothertab_f3_f4_key" UNIQUE CONSTRAINT, btree (f3, f4)
+    "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)

@@ -1936,6 +1943,9 @@ alter table anothertab
 Indexes:
     "anothertab_pkey" PRIMARY KEY, btree (f1)
     "anothertab_f2_key" UNIQUE CONSTRAINT, btree (f2)
+    "anothertab_f3_f4_key" UNIQUE CONSTRAINT, btree (f3, f4)
+    "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)

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 8c8b627..95fa1ce 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1323,6 +1323,10 @@ 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 unique(f3,f4);
+create index on anothertab(f2,f3);
+create unique index on anothertab(f4);

 \d anothertab
 alter table anothertab alter column f1 type bigint;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 98519ef..83dc6df 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10600,11 +10600,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 = table_open(DependRelationId, RowExclusiveLock);

@@ -10646,6 +10641,14 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
                     if (relKind == RELKIND_INDEX ||
                         relKind == RELKIND_PARTITIONED_INDEX)
                     {
+                        /*
+                         * This de-duplication check is critical for two
+                         * independent reasons: we mustn't try to create 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.
+                         */
                         Assert(foundObject.objectSubId == 0);
                         if (!list_member_oid(tab->changedIndexOids, foundObject.objectId))
                         {
@@ -10688,6 +10691,12 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
                 }

             case OCLASS_CONSTRAINT:
+
+                /*
+                 * As with indexes, de-duplication is critical, and it's
+                 * needed because the same constraint could depend on multiple
+                 * target columns.
+                 */
                 Assert(foundObject.objectSubId == 0);
                 if (!list_member_oid(tab->changedConstraintOids,
                                      foundObject.objectId))
@@ -11198,8 +11207,25 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
             def_item, tab->changedIndexDefs)
     {
         Oid            oldId = lfirst_oid(oid_item);
+        Oid            conoid;
         Oid            relid;

+        /*
+         * We must check each index to see if it belongs to a constraint that
+         * we already processed above.  Typically this check does not fire
+         * because constraint indexes normally have only dependencies on their
+         * constraint, and thus would not have been found by the dependency
+         * scan in ATExecAlterColumnType.  But it's possible for such an index
+         * to also have direct dependencies on the table, for example with a
+         * partial exclusion constraint.  In that case we'd have list entries
+         * for both the index and the constraint, and we must disregard the
+         * one for the index.
+         */
+        conoid = get_index_constraint(oldId);
+        if (OidIsValid(conoid) &&
+            list_member_oid(tab->changedConstraintOids, conoid))
+            continue;
+
         relid = IndexGetRelation(oldId, false);
         ATPostAlterTypeParse(oldId, relid, InvalidOid,
                              (char *) lfirst(def_item),