Re: ALTER TABLE with ADD COLUMN and ADD PRIMARY KEY USING INDEX throws spurious "column contains null values"

"Zhang, Jie" <zhangjie2@cn.fujitsu.com> writes:
> Here's a patch to fix this bug.

I took a look at this patch, but I really dislike it: it adds a mighty
ad-hoc parameter to a whole bunch of functions that shouldn't really
have anything to do with the problem.  Moreover, I have little confidence
that it's really solving the problem and not introducing any new problems
(such as failure to apply the not-null check when we need to).

I think the real problem is exactly that we've got index_check_primary_key
doing its own mini ALTER TABLE in ignorance of what might be happening
in the outer ALTER TABLE.  That's just ripe for order-of-operations
problems, seeing that ALTER TABLE has a lot of very careful decisions
about which steps have to happen before which other ones.  Moreover,
as this old comment notes, it's a horridly inefficient approach if
the table is large:

    /*
     * XXX: possible future improvement: when being called from ALTER TABLE,
     * it would be more efficient to merge this with the outer ALTER TABLE, so
     * as to avoid two scans.  But that seems to complicate DefineIndex's API
     * unduly.
     */

So I thought a bit about how to fix that, and realized that we could
easily adjust the parser to emit AT_SetNotNull subcommands as part of the
outer ALTER TABLE that has the ADD PRIMARY KEY subcommand.  Then,
index_check_primary_key doesn't need to do anything at all about adding
NOT NULL, although it seems like a good safety feature for it to check
that somebody else already added that.

So, attached is a WIP patch that fixes it that way.  Some notes
for review:

* I initially thought that index_check_primary_key could be simplified
to the point where it *only* throws an error for missing NOT NULL.
This soon proved to be wrong, because the comments for the function
are lies, or at least obsolete: there are multiple scenarios in which
a CREATE TABLE with a PRIMARY KEY option does need this function to
perform ALTER TABLE SET NOT NULL.  Fortunately, that's not so expensive
in that case, since the table must be empty.  So as coded, it throws
an error if is_alter_table, and otherwise does what it did before.

* We need to fix the order of operations in ALTER TABLE phase 2 so that
any AT_SetNotNull subcommands happen after the AT_PASS_ADD_COL pass
(else the column might not be there yet) and before AT_PASS_ADD_INDEX
(else index_check_primary_key will complain).  I did this by putting
AT_SetNotNull into the AT_PASS_COL_ATTRS pass and moving that to after
AT_PASS_ADD_COL; it had been before AT_PASS_ADD_COL, but that seems at
best random and at worst broken.  (AT_SetIdentity is the only existing
subcommand using AT_PASS_COL_ATTRS, and it sure seems like it'd make more
sense to run it after AT_PASS_ADD_COL, so that it can work on a column
being added in the same ALTER.  Am I missing something?)

* Some existing regression tests for "ALTER TABLE ONLY partitioned_table
ADD PRIMARY KEY" failed.  That apparently is supposed to work if all
partitions already have a suitable unique index and NOT NULL constraint,
but it was failing because ATPrepSetNotNull wasn't expecting to be used
this way.  I thought that function was a pretty terrible kluge anyway,
so what I did was to refactor things so that in this scenario we just
apply checks to see if all the partitions already have suitable NOT NULL.
Note that this represents looser semantics than what was there before,
because previously you couldn't say "ALTER TABLE ONLY partitioned_table
SET NOT NULL" if there were any partitions; now you can, if the partitions
all have suitable NOT NULL already.  We probably ought to change the error
message to reflect that, but I didn't yet.

* A couple of existing test cases change error messages, like so:

-ERROR:  column "test1" named in key does not exist
+ERROR:  column "test1" of relation "atacc1" does not exist

This is because the added AT_SetNotNull subcommand runs before
AT_AddIndex, so it's the one that notices that there's not really
any such column, and historically it's spelled its error message
differently.  This change seems all to the good to me, so I didn't
try to avoid it.

* I haven't yet added any test case(s) reflecting the bug fix nor
the looser semantics for adding NOT NULL to partitioned tables.
It does pass check-world as presented.

* I'm not sure whether we want to try to back-patch this, or how
far it should go.  The misbehavior has been there a long time
(at least back to 8.4, I didn't check further); so the lack of
previous reports means people seldom try to do it.  That may
indicate that it's not worth taking any risks of new bugs to
squash this one.  (Also, I suspect that it might take a lot of
work to port this to before v10, because there are comments
suggesting that this code worked a good bit differently before.)
I do think we should shove this into v12 though.

Comments?

            regards, tom lane

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index e9399be..0dbc410 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -185,12 +185,28 @@ relationHasPrimaryKey(Relation rel)
  *
  * We check for a pre-existing primary key, and that all columns of the index
  * are simple column references (not expressions), and that all those
- * columns are marked NOT NULL.  If they aren't (which can only happen during
- * ALTER TABLE ADD CONSTRAINT, since the parser forces such columns to be
- * created NOT NULL during CREATE TABLE), do an ALTER SET NOT NULL to mark
- * them so --- or fail if they are not in fact nonnull.
+ * columns are marked NOT NULL.
  *
- * As of PG v10, the SET NOT NULL is applied to child tables as well, so
+ * If any column is not so marked, then:
+ *
+ * 1. If we're in an ALTER TABLE, fail; the ALTER TABLE machinery should have
+ * already taken care of making it NOT NULL.  (Hence, we don't really expect
+ * to hit that error, but it seems wise to check anyway.)
+ *
+ * 2. Otherwise, perform our own ALTER TABLE to set such column(s) NOT NULL.
+ * We should not hit this case either, in simple CREATE TABLE cases, because
+ * the parser would've marked the columns NOT NULL to start with.  However,
+ * it is possible to hit the case in unusual scenarios, for example when the
+ * column is inherited from another table or from a composite type.
+ *
+ * The reason we don't try to do another ALTER TABLE when inside ALTER TABLE
+ * is that we don't know what else the outer ALTER TABLE might be doing, and
+ * we're quite likely to end up doing things in the wrong order.  Even if
+ * it worked reliably, it would be very inefficient, because we would be
+ * performing our own scan of the table in addition to whatever the outer
+ * ALTER TABLE might need to do.
+ *
+ * As of PG v10, SET NOT NULL is applied to child tables as well, so
  * that the behavior is like a manual SET NOT NULL.
  *
  * Caller had better have at least ShareLock on the table, else the not-null
@@ -206,8 +222,8 @@ index_check_primary_key(Relation heapRel,
     int            i;

     /*
-     * If ALTER TABLE and CREATE TABLE .. PARTITION OF, check that there isn't
-     * already a PRIMARY KEY.  In CREATE TABLE for an ordinary relations, we
+     * If ALTER TABLE or CREATE TABLE .. PARTITION OF, check that there isn't
+     * already a PRIMARY KEY.  In CREATE TABLE for an ordinary relation, we
      * have faith that the parser rejected multiple pkey clauses; and CREATE
      * INDEX doesn't have a way to say PRIMARY KEY, so it's no problem either.
      */
@@ -222,7 +238,7 @@ index_check_primary_key(Relation heapRel,

     /*
      * Check that all of the attributes in a primary key are marked as not
-     * null, otherwise attempt to ALTER TABLE .. SET NOT NULL
+     * null, otherwise consider using ALTER TABLE .. SET NOT NULL.
      */
     cmds = NIL;
     for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
@@ -250,25 +266,32 @@ index_check_primary_key(Relation heapRel,

         if (!attform->attnotnull)
         {
-            /* Add a subcommand to make this one NOT NULL */
-            AlterTableCmd *cmd = makeNode(AlterTableCmd);
+            if (is_alter_table)
+            {
+                /* Complain, per comments above */
+                ereport(ERROR,
+                        (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                         errmsg("primary key column \"%s\" is not marked NOT NULL",
+                                NameStr(attform->attname))));
+            }
+            else
+            {
+                /* Prepare a subcommand to make this one NOT NULL */
+                AlterTableCmd *cmd = makeNode(AlterTableCmd);

-            cmd->subtype = AT_SetNotNull;
-            cmd->name = pstrdup(NameStr(attform->attname));
-            cmds = lappend(cmds, cmd);
+                cmd->subtype = AT_SetNotNull;
+                cmd->name = pstrdup(NameStr(attform->attname));
+                cmds = lappend(cmds, cmd);
+            }
         }

         ReleaseSysCache(atttuple);
     }

-    /*
-     * XXX: possible future improvement: when being called from ALTER TABLE,
-     * it would be more efficient to merge this with the outer ALTER TABLE, so
-     * as to avoid two scans.  But that seems to complicate DefineIndex's API
-     * unduly.
-     */
+    /* Perform the internal ALTER TABLE if needed */
     if (cmds)
     {
+        Assert(!is_alter_table);
         EventTriggerAlterTableStart((Node *) stmt);
         AlterTableInternal(RelationGetRelid(heapRel), cmds, true);
         EventTriggerAlterTableEnd();
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d48a947..6209d6e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -142,9 +142,9 @@ static List *on_commits = NIL;
 #define AT_PASS_ALTER_TYPE        1    /* ALTER COLUMN TYPE */
 #define AT_PASS_OLD_INDEX        2    /* re-add existing indexes */
 #define AT_PASS_OLD_CONSTR        3    /* re-add existing constraints */
-#define AT_PASS_COL_ATTRS        4    /* set other column attributes */
 /* We could support a RENAME COLUMN pass here, but not currently used */
-#define AT_PASS_ADD_COL            5    /* ADD COLUMN */
+#define AT_PASS_ADD_COL            4    /* ADD COLUMN */
+#define AT_PASS_COL_ATTRS        5    /* set other column attributes */
 #define AT_PASS_ADD_INDEX        6    /* ADD indexes */
 #define AT_PASS_ADD_CONSTR        7    /* ADD constraints, defaults */
 #define AT_PASS_MISC            8    /* other stuff */
@@ -370,9 +370,13 @@ static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
 static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid);
 static void ATPrepDropNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode);
-static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
+static void ATPrepSetNotNull(List **wqueue, Relation rel,
+                 AlterTableCmd *cmd, bool recurse, bool recursing,
+                 LOCKMODE lockmode);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
                  const char *colName, LOCKMODE lockmode);
+static void ATExecCheckNotNull(AlteredTableInfo *tab, Relation rel,
+                   const char *colName, LOCKMODE lockmode);
 static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static bool ConstraintImpliedByRelConstraint(Relation scanrel,
                                      List *partConstraint, List *existedConstraints);
@@ -3765,6 +3769,15 @@ AlterTableGetLockLevel(List *cmds)
                 cmd_lockmode = AccessExclusiveLock;
                 break;

+            case AT_CheckNotNull:
+
+                /*
+                 * This only examines the table's schema; but lock must be
+                 * strong enough to prevent concurrent DROP NOT NULL.
+                 */
+                cmd_lockmode = AccessShareLock;
+                break;
+
             default:            /* oops */
                 elog(ERROR, "unrecognized alter table type: %d",
                      (int) cmd->subtype);
@@ -3889,15 +3902,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
             ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
             ATPrepDropNotNull(rel, recurse, recursing);
             ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
-            /* No command-specific prep needed */
             pass = AT_PASS_DROP;
             break;
         case AT_SetNotNull:        /* ALTER COLUMN SET NOT NULL */
             ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
-            ATPrepSetNotNull(rel, recurse, recursing);
+            /* Need command-specific recursion decision */
+            ATPrepSetNotNull(wqueue, rel, cmd, recurse, recursing, lockmode);
+            pass = AT_PASS_COL_ATTRS;
+            break;
+        case AT_CheckNotNull:    /* check column is already marked NOT NULL */
+            ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
             ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
             /* No command-specific prep needed */
-            pass = AT_PASS_ADD_CONSTR;
+            pass = AT_PASS_COL_ATTRS;
             break;
         case AT_SetStatistics:    /* ALTER COLUMN SET STATISTICS */
             ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
@@ -4214,6 +4231,9 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
         case AT_SetNotNull:        /* ALTER COLUMN SET NOT NULL */
             address = ATExecSetNotNull(tab, rel, cmd->name, lockmode);
             break;
+        case AT_CheckNotNull:    /* check column is already marked NOT NULL */
+            ATExecCheckNotNull(tab, rel, cmd->name, lockmode);
+            break;
         case AT_SetStatistics:    /* ALTER COLUMN SET STATISTICS */
             address = ATExecSetStatistics(rel, cmd->name, cmd->num, cmd->def, lockmode);
             break;
@@ -5990,6 +6010,7 @@ ATPrepDropNotNull(Relation rel, bool recurse, bool recursing)
                      errhint("Do not specify the ONLY keyword.")));
     }
 }
+
 static ObjectAddress
 ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
 {
@@ -6116,23 +6137,33 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
  */

 static void
-ATPrepSetNotNull(Relation rel, bool recurse, bool recursing)
+ATPrepSetNotNull(List **wqueue, Relation rel,
+                 AlterTableCmd *cmd, bool recurse, bool recursing,
+                 LOCKMODE lockmode)
 {
     /*
-     * If the parent is a partitioned table, like check constraints, NOT NULL
-     * constraints must be added to the child tables.  Complain if requested
-     * otherwise and partitions exist.
+     * If we're already recursing, there's nothing to do; the topmost
+     * invocation of ATSimpleRecursion already visited all children.
      */
-    if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+    if (recursing)
+        return;
+
+    /*
+     * If we have ALTER TABLE ONLY ... SET NOT NULL on a partitioned table,
+     * apply ALTER TABLE ... CHECK NOT NULL to every child.  Otherwise, use
+     * normal recursion logic.
+     */
+    if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+        !recurse)
     {
-        PartitionDesc partdesc = RelationGetPartitionDesc(rel);
+        AlterTableCmd *newcmd = makeNode(AlterTableCmd);

-        if (partdesc && partdesc->nparts > 0 && !recurse && !recursing)
-            ereport(ERROR,
-                    (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
-                     errmsg("cannot add constraint to only the partitioned table when partitions exist"),
-                     errhint("Do not specify the ONLY keyword.")));
+        newcmd->subtype = AT_CheckNotNull;
+        newcmd->name = pstrdup(cmd->name);
+        ATSimpleRecursion(wqueue, rel, newcmd, true, lockmode);
     }
+    else
+        ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
 }

 /*
@@ -6208,6 +6239,46 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 }

 /*
+ * ALTER TABLE ALTER COLUMN CHECK NOT NULL
+ *
+ * This doesn't exist in the grammar, but we generate AT_CheckNotNull
+ * commands against the partitions of a partitioned table if the user
+ * writes ALTER TABLE ONLY ... SET NOT NULL on the partitioned table,
+ * or tries to create a primary key on it (which internally creates
+ * AT_SetNotNull on the partitioned table).   Such a command doesn't
+ * allow us to actually modify any partition, but we want to let it
+ * go through if the partitions are already properly marked.
+ */
+static void
+ATExecCheckNotNull(AlteredTableInfo *tab, Relation rel,
+                   const char *colName, LOCKMODE lockmode)
+{
+    HeapTuple    tuple;
+
+    tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
+
+    if (!HeapTupleIsValid(tuple))
+        ereport(ERROR,
+                (errcode(ERRCODE_UNDEFINED_COLUMN),
+                 errmsg("column \"%s\" of relation \"%s\" does not exist",
+                        colName, RelationGetRelationName(rel))));
+
+    if (!((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)
+    {
+        /*
+         * This error message may seem pretty inapropos, but it corresponds to
+         * what we did in a previous implementation of this check.
+         */
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                 errmsg("cannot add constraint to only the partitioned table when partitions exist"),
+                 errhint("Do not specify the ONLY keyword.")));
+    }
+
+    ReleaseSysCache(tuple);
+}
+
+/*
  * NotNullImpliedByRelConstraints
  *        Does rel's existing constraints imply NOT NULL for the given attribute?
  */
@@ -11269,6 +11340,16 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
                                              NIL,
                                              con->conname);
                 }
+                else if (cmd->subtype == AT_SetNotNull)
+                {
+                    /*
+                     * The parser will create AT_SetNotNull subcommands for
+                     * columns of PRIMARY KEY indexes/constraints, but we need
+                     * not do anything with them here, because the columns'
+                     * NOT NULL marks will already have been propagated into
+                     * the new table definition.
+                     */
+                }
                 else
                     elog(ERROR, "unexpected statement subtype: %d",
                          (int) cmd->subtype);
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 674f4b9..94bcd63 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3236,13 +3236,36 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
      * Push any index-creation commands into the ALTER, so that they can be
      * scheduled nicely by tablecmds.c.  Note that tablecmds.c assumes that
      * the IndexStmt attached to an AT_AddIndex or AT_AddIndexConstraint
-     * subcommand has already been through transformIndexStmt.
+     * subcommand has already been through transformIndexStmt, and that any
+     * NOT NULL constraints required for PRIMARY KEY indexes are handled by
+     * earlier AT_SetNotNull commands.
      */
     foreach(l, cxt.alist)
     {
         IndexStmt  *idxstmt = lfirst_node(IndexStmt, l);

         idxstmt = transformIndexStmt(relid, idxstmt, queryString);
+
+        /*
+         * If it's PRIMARY KEY, also insert SET NOT NULL commands for each
+         * column.
+         */
+        if (idxstmt->primary)
+        {
+            ListCell   *l2;
+
+            foreach(l2, idxstmt->indexParams)
+            {
+                IndexElem  *ielem = lfirst(l2);
+
+                Assert(ielem->name);    /* should not have any index exprs */
+                newcmd = makeNode(AlterTableCmd);
+                newcmd->subtype = AT_SetNotNull;
+                newcmd->name = pstrdup(ielem->name);
+                newcmds = lappend(newcmds, newcmd);
+            }
+        }
+
         newcmd = makeNode(AlterTableCmd);
         newcmd->subtype = OidIsValid(idxstmt->indexOid) ? AT_AddIndexConstraint : AT_AddIndex;
         newcmd->def = (Node *) idxstmt;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 94c0b7a..462237d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1764,6 +1764,7 @@ typedef enum AlterTableType
     AT_ColumnDefault,            /* alter column default */
     AT_DropNotNull,                /* alter column drop not null */
     AT_SetNotNull,                /* alter column set not null */
+    AT_CheckNotNull,            /* check column is already marked not null */
     AT_SetStatistics,            /* alter column set statistics */
     AT_SetOptions,                /* alter column set ( options ) */
     AT_ResetOptions,            /* alter column reset ( options ) */
diff --git a/src/test/modules/test_ddl_deparse/expected/alter_table.out
b/src/test/modules/test_ddl_deparse/expected/alter_table.out
index 7da847d..141060f 100644
--- a/src/test/modules/test_ddl_deparse/expected/alter_table.out
+++ b/src/test/modules/test_ddl_deparse/expected/alter_table.out
@@ -23,8 +23,7 @@ NOTICE:  DDL test: type simple, tag CREATE TABLE
 CREATE TABLE part1 PARTITION OF part FOR VALUES FROM (1) to (100);
 NOTICE:  DDL test: type simple, tag CREATE TABLE
 ALTER TABLE part ADD PRIMARY KEY (a);
-NOTICE:  DDL test: type alter table, tag CREATE INDEX
+NOTICE:  DDL test: type alter table, tag ALTER TABLE
 NOTICE:    subcommand: SET NOT NULL
 NOTICE:    subcommand: SET NOT NULL
-NOTICE:  DDL test: type alter table, tag ALTER TABLE
 NOTICE:    subcommand: ADD INDEX
diff --git a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
index 2fe0c24..7f77f19 100644
--- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
+++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
@@ -117,6 +117,9 @@ get_altertable_subcmdtypes(PG_FUNCTION_ARGS)
             case AT_SetNotNull:
                 strtype = "SET NOT NULL";
                 break;
+            case AT_CheckNotNull:
+                strtype = "CHECK NOT NULL";
+                break;
             case AT_SetStatistics:
                 strtype = "SET STATS";
                 break;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 2a26aa3..aa80038 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -978,7 +978,7 @@ drop table atacc1;
 create table atacc1 ( test int );
 -- add a primary key constraint (fails)
 alter table atacc1 add constraint atacc_test1 primary key (test1);
-ERROR:  column "test1" named in key does not exist
+ERROR:  column "test1" of relation "atacc1" does not exist
 drop table atacc1;
 -- adding a new column as primary key to a non-empty table.
 -- should fail unless the column has a non-null default value.
@@ -1404,9 +1404,9 @@ ERROR:  column "a" does not exist
 alter table atacc1 rename "........pg.dropped.1........" to x;
 ERROR:  column "........pg.dropped.1........" does not exist
 alter table atacc1 add primary key(a);
-ERROR:  column "a" named in key does not exist
+ERROR:  column "a" of relation "atacc1" does not exist
 alter table atacc1 add primary key("........pg.dropped.1........");
-ERROR:  column "........pg.dropped.1........" named in key does not exist
+ERROR:  column "........pg.dropped.1........" of relation "atacc1" does not exist
 alter table atacc1 add unique(a);
 ERROR:  column "a" named in key does not exist
 alter table atacc1 add unique("........pg.dropped.1........");