Fix ALTER TABLE's scheduling rules for AT_AddConstraint subcommands.

Commit 1281a5c90 rearranged the logic in this area rather drastically,
and it broke the case of adding a foreign key constraint in the same
ALTER that adds the pkey or unique constraint it depends on.  While
self-referential fkeys are surely a pretty niche case, this used to
work so we shouldn't break it.

To fix, reorganize the scheduling rules in ATParseTransformCmd so
that a transformed AT_AddConstraint subcommand will be delayed into
a later pass in all cases, not only when it's been spit out as a
side-effect of parsing some other command type.

Also tweak the logic so that we won't run ATParseTransformCmd twice
while doing this.  It seems to work even without that, but it's
surely wasting cycles to do so.

Per bug #16589 from Jeremy Evans.  Back-patch to v13 where the new
code was introduced.

Discussion: https://postgr.es/m/16589-31c8d981ca503896@postgresql.org

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 790c09c522e42becf555cf33eb3a2002d98ccbae..d2b15a3387b0bee7529471f2fb532ce96a63c2fa 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4513,9 +4513,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
                                     lockmode);
            break;
        case AT_AddConstraint:  /* ADD CONSTRAINT */
-           cmd = ATParseTransformCmd(wqueue, tab, rel, cmd, false, lockmode,
-                                     cur_pass, context);
-           /* Might not have gotten AddConstraint back from parse transform */
+           /* Transform the command only during initial examination */
+           if (cur_pass == AT_PASS_ADD_CONSTR)
+               cmd = ATParseTransformCmd(wqueue, tab, rel, cmd,
+                                         false, lockmode,
+                                         cur_pass, context);
+           /* Depending on constraint type, might be no more work to do now */
            if (cmd != NULL)
                address =
                    ATExecAddConstraint(wqueue, tab, rel,
@@ -4523,9 +4526,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
                                        false, false, lockmode);
            break;
        case AT_AddConstraintRecurse:   /* ADD CONSTRAINT with recursion */
-           cmd = ATParseTransformCmd(wqueue, tab, rel, cmd, true, lockmode,
-                                     cur_pass, context);
-           /* Might not have gotten AddConstraint back from parse transform */
+           /* Transform the command only during initial examination */
+           if (cur_pass == AT_PASS_ADD_CONSTR)
+               cmd = ATParseTransformCmd(wqueue, tab, rel, cmd,
+                                         true, lockmode,
+                                         cur_pass, context);
+           /* Depending on constraint type, might be no more work to do now */
            if (cmd != NULL)
                address =
                    ATExecAddConstraint(wqueue, tab, rel,
@@ -4787,75 +4793,88 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
    foreach(lc, atstmt->cmds)
    {
        AlterTableCmd *cmd2 = lfirst_node(AlterTableCmd, lc);
+       int         pass;
+
+       /*
+        * This switch need only cover the subcommand types that can be added
+        * by parse_utilcmd.c; otherwise, we'll use the default strategy of
+        * executing the subcommand immediately, as a substitute for the
+        * original subcommand.  (Note, however, that this does cause
+        * AT_AddConstraint subcommands to be rescheduled into later passes,
+        * which is important for index and foreign key constraints.)
+        *
+        * We assume we needn't do any phase-1 checks for added subcommands.
+        */
+       switch (cmd2->subtype)
+       {
+           case AT_SetNotNull:
+               /* Need command-specific recursion decision */
+               ATPrepSetNotNull(wqueue, rel, cmd2,
+                                recurse, false,
+                                lockmode, context);
+               pass = AT_PASS_COL_ATTRS;
+               break;
+           case AT_AddIndex:
+               /* This command never recurses */
+               /* No command-specific prep needed */
+               pass = AT_PASS_ADD_INDEX;
+               break;
+           case AT_AddIndexConstraint:
+               /* This command never recurses */
+               /* No command-specific prep needed */
+               pass = AT_PASS_ADD_INDEXCONSTR;
+               break;
+           case AT_AddConstraint:
+               /* Recursion occurs during execution phase */
+               if (recurse)
+                   cmd2->subtype = AT_AddConstraintRecurse;
+               switch (castNode(Constraint, cmd2->def)->contype)
+               {
+                   case CONSTR_PRIMARY:
+                   case CONSTR_UNIQUE:
+                   case CONSTR_EXCLUSION:
+                       pass = AT_PASS_ADD_INDEXCONSTR;
+                       break;
+                   default:
+                       pass = AT_PASS_ADD_OTHERCONSTR;
+                       break;
+               }
+               break;
+           case AT_AlterColumnGenericOptions:
+               /* This command never recurses */
+               /* No command-specific prep needed */
+               pass = AT_PASS_MISC;
+               break;
+           default:
+               pass = cur_pass;
+               break;
+       }
 
-       if (newcmd == NULL &&
-           (cmd->subtype == cmd2->subtype ||
-            (cmd->subtype == AT_AddConstraintRecurse &&
-             cmd2->subtype == AT_AddConstraint)))
+       if (pass < cur_pass)
+       {
+           /* Cannot schedule into a pass we already finished */
+           elog(ERROR, "ALTER TABLE scheduling failure: too late for pass %d",
+                pass);
+       }
+       else if (pass > cur_pass)
        {
-           /* Found the transformed version of our subcommand */
-           cmd2->subtype = cmd->subtype;   /* copy recursion flag */
-           newcmd = cmd2;
+           /* OK, queue it up for later */
+           tab->subcmds[pass] = lappend(tab->subcmds[pass], cmd2);
        }
        else
        {
-           int         pass;
-
            /*
-            * Schedule added subcommand appropriately.  We assume we needn't
-            * do any phase-1 checks for it.  This switch only has to cover
-            * the subcommand types that can be added by parse_utilcmd.c.
+            * We should see at most one subcommand for the current pass,
+            * which is the transformed version of the original subcommand.
             */
-           switch (cmd2->subtype)
+           if (newcmd == NULL && cmd->subtype == cmd2->subtype)
            {
-               case AT_SetNotNull:
-                   /* Need command-specific recursion decision */
-                   ATPrepSetNotNull(wqueue, rel, cmd2,
-                                    recurse, false,
-                                    lockmode, context);
-                   pass = AT_PASS_COL_ATTRS;
-                   break;
-               case AT_AddIndex:
-                   /* This command never recurses */
-                   /* No command-specific prep needed */
-                   pass = AT_PASS_ADD_INDEX;
-                   break;
-               case AT_AddIndexConstraint:
-                   /* This command never recurses */
-                   /* No command-specific prep needed */
-                   pass = AT_PASS_ADD_INDEXCONSTR;
-                   break;
-               case AT_AddConstraint:
-                   /* Recursion occurs during execution phase */
-                   if (recurse)
-                       cmd2->subtype = AT_AddConstraintRecurse;
-                   switch (castNode(Constraint, cmd2->def)->contype)
-                   {
-                       case CONSTR_PRIMARY:
-                       case CONSTR_UNIQUE:
-                       case CONSTR_EXCLUSION:
-                           pass = AT_PASS_ADD_INDEXCONSTR;
-                           break;
-                       default:
-                           pass = AT_PASS_ADD_OTHERCONSTR;
-                           break;
-                   }
-                   break;
-               case AT_AlterColumnGenericOptions:
-                   /* This command never recurses */
-                   /* No command-specific prep needed */
-                   pass = AT_PASS_MISC;
-                   break;
-               default:
-                   elog(ERROR, "unexpected AlterTableType: %d",
-                        (int) cmd2->subtype);
-                   pass = AT_PASS_UNSET;
-                   break;
+               /* Found the transformed version of our subcommand */
+               newcmd = cmd2;
            }
-           /* Must be for a later pass than we're currently doing */
-           if (pass <= cur_pass)
-               elog(ERROR, "ALTER TABLE scheduling failure");
-           tab->subcmds[pass] = lappend(tab->subcmds[pass], cmd2);
+           else
+               elog(ERROR, "ALTER TABLE scheduling failure: bogus item for pass %d",
+                    pass);
        }
    }
 

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 6f90eae2f8ce99265a8a164d26f279bd3ab26d30..f56615393ec32780bb5c2b458a097b98fd246ecf 100644 (file)
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3678,6 +3678,42 @@ ALTER TABLE ataddindex
 Indexes:
     "ataddindex_expr_excl" EXCLUDE USING btree ((f1 ~~ 'a'::text) WITH =)
 
+DROP TABLE ataddindex;
+CREATE TABLE ataddindex(id int, ref_id int);
+ALTER TABLE ataddindex
+  ADD PRIMARY KEY (id),
+  ADD FOREIGN KEY (ref_id) REFERENCES ataddindex;
+\d ataddindex
+             Table "public.ataddindex"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ id     | integer |           | not null | 
+ ref_id | integer |           |          | 
+Indexes:
+    "ataddindex_pkey" PRIMARY KEY, btree (id)
+Foreign-key constraints:
+    "ataddindex_ref_id_fkey" FOREIGN KEY (ref_id) REFERENCES ataddindex(id)
+Referenced by:
+    TABLE "ataddindex" CONSTRAINT "ataddindex_ref_id_fkey" FOREIGN KEY (ref_id) REFERENCES ataddindex(id)
+
+DROP TABLE ataddindex;
+CREATE TABLE ataddindex(id int, ref_id int);
+ALTER TABLE ataddindex
+  ADD UNIQUE (id),
+  ADD FOREIGN KEY (ref_id) REFERENCES ataddindex (id);
+\d ataddindex
+             Table "public.ataddindex"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ id     | integer |           |          | 
+ ref_id | integer |           |          | 
+Indexes:
+    "ataddindex_id_key" UNIQUE CONSTRAINT, btree (id)
+Foreign-key constraints:
+    "ataddindex_ref_id_fkey" FOREIGN KEY (ref_id) REFERENCES ataddindex(id)
+Referenced by:
+    TABLE "ataddindex" CONSTRAINT "ataddindex_ref_id_fkey" FOREIGN KEY (ref_id) REFERENCES ataddindex(id)
+
 DROP TABLE ataddindex;
 -- unsupported constraint types for partitioned tables
 CREATE TABLE partitioned (

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index ce6401d80d288dd4c4d93b97b1d93627864378e1..4cc55d852513e698ecb51586efad2c0e44f72e83 100644 (file)
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2252,6 +2252,20 @@ ALTER TABLE ataddindex
 \d ataddindex
 DROP TABLE ataddindex;
 
+CREATE TABLE ataddindex(id int, ref_id int);
+ALTER TABLE ataddindex
+  ADD PRIMARY KEY (id),
+  ADD FOREIGN KEY (ref_id) REFERENCES ataddindex;
+\d ataddindex
+DROP TABLE ataddindex;
+
+CREATE TABLE ataddindex(id int, ref_id int);
+ALTER TABLE ataddindex
+  ADD UNIQUE (id),
+  ADD FOREIGN KEY (ref_id) REFERENCES ataddindex (id);
+\d ataddindex
+DROP TABLE ataddindex;
+
 -- unsupported constraint types for partitioned tables
 CREATE TABLE partitioned (
    a int,