Apply ALTER ... SET NOT NULL recursively in ALTER ... ADD PRIMARY KEY.

If you do ALTER COLUMN SET NOT NULL against an inheritance parent table,
it will recurse to mark all the child columns as NOT NULL as well.  This
is necessary for consistency: if the column is labeled NOT NULL then
reading it should never produce nulls.

However, that didn't happen in the case where ALTER ... ADD PRIMARY KEY
marks a target column NOT NULL that wasn't before.  That was questionable
from the beginning, and now Tushar Ahuja points out that it can lead to
dump/restore failures in some cases.  So let's make that case recurse too.

Although this is meant to fix a bug, it's enough of a behavioral change
that I'm pretty hesitant to back-patch, especially in view of the lack
of similar field complaints.  It doesn't seem to be too late to put it
into v10 though.

Michael Paquier, editorialized on slightly by me

Discussion: https://postgr.es/m/b8794d6a-38f0-9d7c-ad4b-e85adf860fc9@enterprisedb.com

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index d25b39bb54eb964f983d65bff36d0c84b2be1069..25c5bead9f0a99db4528902b46c8ff611e2378e2 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -185,6 +185,9 @@ relationHasPrimaryKey(Relation rel)
  * 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.
  *
+ * As of PG v10, the 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
  * checking isn't trustworthy.
  */
@@ -253,17 +256,13 @@ index_check_primary_key(Relation heapRel,
        }
 
        /*
-        * XXX: Shouldn't the ALTER TABLE .. SET NOT NULL cascade to child tables?
-        * Currently, since the PRIMARY KEY itself doesn't cascade, we don't
-        * cascade the notnull constraint(s) either; but this is pretty debatable.
-        *
         * 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.
         */
        if (cmds)
-               AlterTableInternal(RelationGetRelid(heapRel), cmds, false);
+               AlterTableInternal(RelationGetRelid(heapRel), cmds, true);
 }
 
 /*

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 13d6a4b74785da653173890003cfed86d2b1d105..e9fd1aacce6013de574026fe0faf9437cee0f2a6 100644 (file)
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -328,7 +328,7 @@ Number of child tables: 1 (Use \d+ to list them.)
       Table "public.constraint_rename_test2"
  Column |  Type   | Collation | Nullable | Default 
 --------+---------+-----------+----------+---------
- a      | integer |           |          | 
+ a      | integer |           | not null | 
  b      | integer |           |          | 
  c      | integer |           |          | 
  d      | integer |           |          |