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

This patch reverts all the code changes of commit e76de8861, 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 e76de8861 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
index 17085545100c6076be9d4aff7e49f38c408ad494..9c4c3b7bbb799616bc5308d48730fe16f4d4dbd2 100644 (file)
--- 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
index 6c4bd067ce5733eaff800340a53ce557f62d6bcc..55f95ab0538b44019ee43cef5e309ce87975d2d5 100644 (file)
--- 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
index 67d7a95e25f4ebd4e4072a5c81a09bcd332e0714..0cb10c391cf8b5c86d0cae5978d8c796708e1e90 100644 (file)
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1292,12 +1292,19 @@ 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;
@@ -1305,6 +1312,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;