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

Keith Fiske <keith.fiske@crunchydata.com> writes:
> On Thu, Jun 20, 2019 at 9:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

> Can't really provide a thorough code review, but I did apply the patch to
> the base 11.4 code (not HEAD from github) and the compound ALTER table
> statement that was failing before now works without error. Thank you for
> the quick fix!

Thanks for testing!  However, I had a nagging feeling that I was still
missing something, and this morning I realized what.  The proposed
patch basically changes ATExecAlterColumnType's assumptions from
"no constraint index will have any direct dependencies on table columns"
to "if a constraint index has a direct dependency on a table column,
so will its constraint".  This is easily shown to not be the case:

regression=# create table foo (f1 int, f2 int);
CREATE TABLE
regression=# alter table foo add exclude using btree (f1 with =) where (f2 > 0);
ALTER TABLE
regression=# select pg_describe_object(classid,objid,objsubid) as obj,
pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype from pg_depend where objid >= 'foo'::regclass or
refobjid>= 'foo'::regclass; 
                 obj                 |                 ref                 | deptype
-------------------------------------+-------------------------------------+---------
 type foo                            | table foo                           | i
 type foo[]                          | type foo                            | i
 table foo                           | schema public                       | n
 constraint foo_f1_excl on table foo | column f1 of table foo              | a
 index foo_f1_excl                   | constraint foo_f1_excl on table foo | i
 index foo_f1_excl                   | column f2 of table foo              | a
(6 rows)

Notice that the index has a dependency on column f2 but the constraint
doesn't.  So if we change (just) f2, ATExecAlterColumnType never notices
the constraint at all, and kaboom:

regression=# alter table foo alter column f2 type bigint;
ERROR:  cannot drop index foo_f1_excl because constraint foo_f1_excl on table foo requires it
HINT:  You can drop constraint foo_f1_excl on table foo instead.

This is the same with or without yesterday's patch, and while I didn't
trouble to verify it, I'm quite sure pre-e76de8861 would fail the same.

I'm not exactly convinced that this dependency structure is a Good Thing,
but in any case we don't get to rethink it in released branches.  So
we need to make ATExecAlterColumnType cope, and the way to do that seems
to be to do the get_index_constraint check in that function not later on.

In principle this might lead to a few more duplicative
get_index_constraint calls than before, because if a constraint index has
multiple column dependencies we'll have to repeat get_index_constraint for
each one.  But I hardly think that case is worth stressing about the
performance of, given it never worked at all before this month.

As before, I attach a patch against HEAD, plus one that assumes e76de8861
has been reverted first, which is likely easier to review.

Unlike yesterday, I'm feeling pretty good about this patch now, but it
still wouldn't hurt for somebody else to go over it.

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cb2c5e1..8205e08 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,58 @@ 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 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.
                          */
                         Assert(foundObject.objectSubId == 0);
-                        tab->changedIndexOids =
-                            list_append_unique_oid(tab->changedIndexOids,
-                                                   foundObject.objectId);
+                        if (!list_member_oid(tab->changedIndexOids, foundObject.objectId))
+                        {
+                            /*
+                             * Before adding it as an index-to-rebuild,
+                             * though, we'd better see if it belongs to a
+                             * constraint, and if so rebuild the constraint
+                             * instead.  Typically this check does not fire
+                             * because constraint indexes normally have only
+                             * dependencies on their constraint.  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
+                             * have to handle this, or bad things ensue.
+                             */
+                            Oid            conoid;
+
+                            conoid = get_index_constraint(foundObject.objectId);
+                            if (OidIsValid(conoid))
+                            {
+                                /* Should match OCLASS_CONSTRAINT code below */
+                                if (!list_member_oid(tab->changedConstraintOids,
+                                                     conoid))
+                                {
+                                    char       *defstring = pg_get_constraintdef_command(conoid);
+
+                                    tab->changedConstraintOids =
+                                        lappend_oid(tab->changedConstraintOids,
+                                                    conoid);
+                                    tab->changedConstraintDefs =
+                                        lappend(tab->changedConstraintDefs,
+                                                defstring);
+                                }
+                            }
+                            else
+                            {
+                                /* OK to handle as plain index */
+                                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 +10728,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 +10866,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
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index c845a16..5207554 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1901,11 +1901,18 @@ 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);
+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   | Collation | Nullable | Default
@@ -1914,17 +1921,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  | Collation | Nullable | Default
@@ -1933,11 +1946,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
index 8c8b627..601253c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1318,11 +1318,18 @@ 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);
+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;
@@ -1330,6 +1337,7 @@ 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;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 98519ef..8205e08 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,13 +10641,58 @@ 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 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.
+                         */
                         Assert(foundObject.objectSubId == 0);
                         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));
+                            /*
+                             * Before adding it as an index-to-rebuild,
+                             * though, we'd better see if it belongs to a
+                             * constraint, and if so rebuild the constraint
+                             * instead.  Typically this check does not fire
+                             * because constraint indexes normally have only
+                             * dependencies on their constraint.  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
+                             * have to handle this, or bad things ensue.
+                             */
+                            Oid            conoid;
+
+                            conoid = get_index_constraint(foundObject.objectId);
+                            if (OidIsValid(conoid))
+                            {
+                                /* Should match OCLASS_CONSTRAINT code below */
+                                if (!list_member_oid(tab->changedConstraintOids,
+                                                     conoid))
+                                {
+                                    char       *defstring = pg_get_constraintdef_command(conoid);
+
+                                    tab->changedConstraintOids =
+                                        lappend_oid(tab->changedConstraintOids,
+                                                    conoid);
+                                    tab->changedConstraintDefs =
+                                        lappend(tab->changedConstraintDefs,
+                                                defstring);
+                                }
+                            }
+                            else
+                            {
+                                /* OK to handle as plain index */
+                                tab->changedIndexOids =
+                                    lappend_oid(tab->changedIndexOids,
+                                                foundObject.objectId);
+                                tab->changedIndexDefs =
+                                    lappend(tab->changedIndexDefs,
+                                            pg_get_indexdef_string(foundObject.objectId));
+                            }
                         }
                     }
                     else if (relKind == RELKIND_SEQUENCE)
@@ -10688,6 +10728,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))
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index cedc7c6..5207554 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1900,6 +1900,64 @@ 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, 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   | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ f1     | integer |           | not null |
+ 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  | Collation | Nullable | Default
+--------+--------+-----------+----------+---------
+ f1     | bigint |           | not null |
+ 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);
 insert into another values(1, 'one');
 insert into another values(2, 'two');
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index e8e094a..601253c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1317,6 +1317,31 @@ 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, 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;
+
 create table another (f1 int, f2 text);

 insert into another values(1, 'one');