Revert structural changes to not-null constraints

There are some problems with the new way to handle these constraints
that were detected at the last minute, and require fixes that appear too
invasive to be doing this late in the cycle.  Revert this (again) for
now, we'll try again with these problems fixed.

The following commits are reverted:

    b0e96f311985  Catalog not-null constraints
    9b581c534186  Disallow changing NO INHERIT status of a not-null constraint
    d0ec2ddbe088  Fix not-null constraint test
    ac22a9545ca9  Move privilege check to the right place
    b0f7dd915bca  Check stack depth in new recursive functions
    3af721794272  Update information_schema definition for not-null constraints
    c3709100be73  Fix propagating attnotnull in multiple inheritance
    d9f686a72ee9  Fix restore of not-null constraints with inheritance
    d72d32f52d26  Don't try to assign smart names to constraints
    0cd711271d42  Better handle indirect constraint drops
    13daa33fa5a6  Disallow NO INHERIT not-null constraints on partitioned tables
    d45597f72fe5  Disallow direct change of NO INHERIT of not-null constraints
    21ac38f498b3  Fix inconsistencies in error messages

Discussion: https://postgr.es/m/202405110940.joxlqcx4dogd@alvherre.pgsql

1 files changed, 59 insertions, 229 deletions

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 0598e897d90..b692d251522 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -84,7 +84,6 @@ typedef struct
 	bool		isalter;		/* true if altering existing table */
 	List	   *columns;		/* ColumnDef items */
 	List	   *ckconstraints;	/* CHECK constraints */
-	List	   *nnconstraints;	/* NOT NULL constraints */
 	List	   *fkconstraints;	/* FOREIGN KEY constraints */
 	List	   *ixconstraints;	/* index-creating constraints */
 	List	   *likeclauses;	/* LIKE clauses that need post-processing */
@@ -244,7 +243,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
 	cxt.isalter = false;
 	cxt.columns = NIL;
 	cxt.ckconstraints = NIL;
-	cxt.nnconstraints = NIL;
 	cxt.fkconstraints = NIL;
 	cxt.ixconstraints = NIL;
 	cxt.likeclauses = NIL;
@@ -351,7 +349,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
 	 */
 	stmt->tableElts = cxt.columns;
 	stmt->constraints = cxt.ckconstraints;
-	stmt->nnconstraints = cxt.nnconstraints;
 
 	result = lappend(cxt.blist, stmt);
 	result = list_concat(result, cxt.alist);
@@ -550,7 +547,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 	bool		saw_default;
 	bool		saw_identity;
 	bool		saw_generated;
-	bool		need_notnull = false;
 	ListCell   *clist;
 
 	cxt->columns = lappend(cxt->columns, column);
@@ -648,8 +644,10 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 		constraint->cooked_expr = NULL;
 		column->constraints = lappend(column->constraints, constraint);
 
-		/* have a not-null constraint added later */
-		need_notnull = true;
+		constraint = makeNode(Constraint);
+		constraint->contype = CONSTR_NOTNULL;
+		constraint->location = -1;
+		column->constraints = lappend(column->constraints, constraint);
 	}
 
 	/* Process column constraints, if any... */
@@ -667,7 +665,7 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 		switch (constraint->contype)
 		{
 			case CONSTR_NULL:
-				if ((saw_nullable && column->is_not_null) || need_notnull)
+				if (saw_nullable && column->is_not_null)
 					ereport(ERROR,
 							(errcode(ERRCODE_SYNTAX_ERROR),
 							 errmsg("conflicting NULL/NOT NULL declarations for column \"%s\" of table \"%s\"",
@@ -679,14 +677,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 				break;
 
 			case CONSTR_NOTNULL:
-				if (cxt->ispartitioned && constraint->is_no_inherit)
-					ereport(ERROR,
-							errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							errmsg("not-null constraints on partitioned tables cannot be NO INHERIT"));
-
-				/*
-				 * Disallow conflicting [NOT] NULL markings
-				 */
 				if (saw_nullable && !column->is_not_null)
 					ereport(ERROR,
 							(errcode(ERRCODE_SYNTAX_ERROR),
@@ -694,25 +684,8 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 									column->colname, cxt->relation->relname),
 							 parser_errposition(cxt->pstate,
 												constraint->location)));
-				/* Ignore redundant NOT NULL markings */
-
-				/*
-				 * If this is the first time we see this column being marked
-				 * not null, add the constraint entry; and get rid of any
-				 * previous markings to mark the column NOT NULL.
-				 */
-				if (!column->is_not_null)
-				{
-					column->is_not_null = true;
-					saw_nullable = true;
-
-					constraint->keys = list_make1(makeString(column->colname));
-					cxt->nnconstraints = lappend(cxt->nnconstraints, constraint);
-
-					/* Don't need this anymore, if we had it */
-					need_notnull = false;
-				}
-
+				column->is_not_null = true;
+				saw_nullable = true;
 				break;
 
 			case CONSTR_DEFAULT:
@@ -762,19 +735,16 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 					column->identity = constraint->generated_when;
 					saw_identity = true;
 
-					/*
-					 * Identity columns are always NOT NULL, but we may have a
-					 * constraint already.
-					 */
-					if (!saw_nullable)
-						need_notnull = true;
-					else if (!column->is_not_null)
+					/* An identity column is implicitly NOT NULL */
+					if (saw_nullable && !column->is_not_null)
 						ereport(ERROR,
 								(errcode(ERRCODE_SYNTAX_ERROR),
 								 errmsg("conflicting NULL/NOT NULL declarations for column \"%s\" of table \"%s\"",
 										column->colname, cxt->relation->relname),
 								 parser_errposition(cxt->pstate,
 													constraint->location)));
+					column->is_not_null = true;
+					saw_nullable = true;
 					break;
 				}
 
@@ -881,29 +851,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 	}
 
 	/*
-	 * If we need a not-null constraint for SERIAL or IDENTITY, and one was
-	 * not explicitly specified, add one now.
-	 */
-	if (need_notnull && !(saw_nullable && column->is_not_null))
-	{
-		Constraint *notnull;
-
-		column->is_not_null = true;
-
-		notnull = makeNode(Constraint);
-		notnull->contype = CONSTR_NOTNULL;
-		notnull->conname = NULL;
-		notnull->deferrable = false;
-		notnull->initdeferred = false;
-		notnull->location = -1;
-		notnull->keys = list_make1(makeString(column->colname));
-		notnull->skip_validation = false;
-		notnull->initially_valid = true;
-
-		cxt->nnconstraints = lappend(cxt->nnconstraints, notnull);
-	}
-
-	/*
 	 * If needed, generate ALTER FOREIGN TABLE ALTER COLUMN statement to add
 	 * per-column foreign data wrapper options to this column after creation.
 	 */
@@ -972,16 +919,6 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
 			cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
 			break;
 
-		case CONSTR_NOTNULL:
-			if (cxt->ispartitioned && constraint->is_no_inherit)
-				ereport(ERROR,
-						errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-						errmsg("not-null constraints on partitioned tables cannot be NO INHERIT"));
-
-
-			cxt->nnconstraints = lappend(cxt->nnconstraints, constraint);
-			break;
-
 		case CONSTR_FOREIGN:
 			if (cxt->isforeign)
 				ereport(ERROR,
@@ -993,6 +930,7 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
 			break;
 
 		case CONSTR_NULL:
+		case CONSTR_NOTNULL:
 		case CONSTR_DEFAULT:
 		case CONSTR_ATTR_DEFERRABLE:
 		case CONSTR_ATTR_NOT_DEFERRABLE:
@@ -1028,7 +966,6 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
 	AclResult	aclresult;
 	char	   *comment;
 	ParseCallbackState pcbstate;
-	bool		process_notnull_constraints = false;
 
 	setup_parser_errposition_callback(&pcbstate, cxt->pstate,
 									  table_like_clause->relation->location);
@@ -1097,18 +1034,14 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
 			continue;
 
 		/*
-		 * Create a new column definition
+		 * Create a new column, which is marked as NOT inherited.
+		 *
+		 * For constraints, ONLY the not-null constraint is inherited by the
+		 * new column definition per SQL99.
 		 */
 		def = makeColumnDef(NameStr(attribute->attname), attribute->atttypid,
 							attribute->atttypmod, attribute->attcollation);
-
-		/*
-		 * For constraints, ONLY the not-null constraint is inherited by the
-		 * new column definition per SQL99; however we cannot do that
-		 * correctly here, so we leave it for expandTableLikeClause to handle.
-		 */
-		if (attribute->attnotnull)
-			process_notnull_constraints = true;
+		def->is_not_null = attribute->attnotnull;
 
 		/*
 		 * Add to column list
@@ -1182,78 +1115,20 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
 	 * we don't yet know what column numbers the copied columns will have in
 	 * the finished table.  If any of those options are specified, add the
 	 * LIKE clause to cxt->likeclauses so that expandTableLikeClause will be
-	 * called after we do know that; in addition, do that if there are any NOT
-	 * NULL constraints, because those must be propagated even if not
-	 * explicitly requested.
-	 *
-	 * In order for this to work, we remember the relation OID so that
+	 * called after we do know that.  Also, remember the relation OID so that
 	 * expandTableLikeClause is certain to open the same table.
 	 */
-	if ((table_like_clause->options &
-		 (CREATE_TABLE_LIKE_DEFAULTS |
-		  CREATE_TABLE_LIKE_GENERATED |
-		  CREATE_TABLE_LIKE_CONSTRAINTS |
-		  CREATE_TABLE_LIKE_INDEXES)) ||
-		process_notnull_constraints)
+	if (table_like_clause->options &
+		(CREATE_TABLE_LIKE_DEFAULTS |
+		 CREATE_TABLE_LIKE_GENERATED |
+		 CREATE_TABLE_LIKE_CONSTRAINTS |
+		 CREATE_TABLE_LIKE_INDEXES))
 	{
 		table_like_clause->relationOid = RelationGetRelid(relation);
 		cxt->likeclauses = lappend(cxt->likeclauses, table_like_clause);
 	}
 
 	/*
-	 * If INCLUDING INDEXES is not given and a primary key exists, we need to
-	 * add not-null constraints to the columns covered by the PK (except those
-	 * that already have one.)  This is required for backwards compatibility.
-	 */
-	if ((table_like_clause->options & CREATE_TABLE_LIKE_INDEXES) == 0)
-	{
-		Bitmapset  *pkcols;
-		int			x = -1;
-		Bitmapset  *donecols = NULL;
-		ListCell   *lc;
-
-		/*
-		 * Obtain a bitmapset of columns on which we'll add not-null
-		 * constraints in expandTableLikeClause, so that we skip this for
-		 * those.
-		 */
-		foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), true))
-		{
-			CookedConstraint *cooked = (CookedConstraint *) lfirst(lc);
-
-			donecols = bms_add_member(donecols, cooked->attnum);
-		}
-
-		pkcols = RelationGetIndexAttrBitmap(relation,
-											INDEX_ATTR_BITMAP_PRIMARY_KEY);
-		while ((x = bms_next_member(pkcols, x)) >= 0)
-		{
-			Constraint *notnull;
-			AttrNumber	attnum = x + FirstLowInvalidHeapAttributeNumber;
-			Form_pg_attribute attForm;
-
-			/* ignore if we already have one for this column */
-			if (bms_is_member(attnum, donecols))
-				continue;
-
-			attForm = TupleDescAttr(tupleDesc, attnum - 1);
-
-			notnull = makeNode(Constraint);
-			notnull->contype = CONSTR_NOTNULL;
-			notnull->conname = NULL;
-			notnull->is_no_inherit = false;
-			notnull->deferrable = false;
-			notnull->initdeferred = false;
-			notnull->location = -1;
-			notnull->keys = list_make1(makeString(pstrdup(NameStr(attForm->attname))));
-			notnull->skip_validation = false;
-			notnull->initially_valid = true;
-
-			cxt->nnconstraints = lappend(cxt->nnconstraints, notnull);
-		}
-	}
-
-	/*
 	 * We may copy extended statistics if requested, since the representation
 	 * of CreateStatsStmt doesn't depend on column numbers.
 	 */
@@ -1319,8 +1194,6 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause)
 	TupleConstr *constr;
 	AttrMap    *attmap;
 	char	   *comment;
-	bool		at_pushed = false;
-	ListCell   *lc;
 
 	/*
 	 * Open the relation referenced by the LIKE clause.  We should still have
@@ -1492,20 +1365,6 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause)
 	}
 
 	/*
-	 * Copy not-null constraints, too (these do not require any option to have
-	 * been given).
-	 */
-	foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), false))
-	{
-		AlterTableCmd *atsubcmd;
-
-		atsubcmd = makeNode(AlterTableCmd);
-		atsubcmd->subtype = AT_AddConstraint;
-		atsubcmd->def = (Node *) lfirst_node(Constraint, lc);
-		atsubcmds = lappend(atsubcmds, atsubcmd);
-	}
-
-	/*
 	 * If we generated any ALTER TABLE actions above, wrap them into a single
 	 * ALTER TABLE command.  Stick it at the front of the result, so it runs
 	 * before any CommentStmts we made above.
@@ -1519,8 +1378,6 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause)
 		atcmd->objtype = OBJECT_TABLE;
 		atcmd->missing_ok = false;
 		result = lcons(atcmd, result);
-
-		at_pushed = true;
 	}
 
 	/*
@@ -1548,39 +1405,6 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause)
 												 attmap,
 												 NULL);
 
-			/*
-			 * The PK columns might not yet non-nullable, so make sure they
-			 * become so.
-			 */
-			if (index_stmt->primary)
-			{
-				foreach(lc, index_stmt->indexParams)
-				{
-					IndexElem  *col = lfirst_node(IndexElem, lc);
-					AlterTableCmd *notnullcmd = makeNode(AlterTableCmd);
-
-					notnullcmd->subtype = AT_SetAttNotNull;
-					notnullcmd->name = pstrdup(col->name);
-					/* Luckily we can still add more AT-subcmds here */
-					atsubcmds = lappend(atsubcmds, notnullcmd);
-				}
-
-				/*
-				 * If we had already put the AlterTableStmt into the output
-				 * list, we don't need to do so again; otherwise do it.
-				 */
-				if (!at_pushed)
-				{
-					AlterTableStmt *atcmd = makeNode(AlterTableStmt);
-
-					atcmd->relation = copyObject(heapRel);
-					atcmd->cmds = atsubcmds;
-					atcmd->objtype = OBJECT_TABLE;
-					atcmd->missing_ok = false;
-					result = lcons(atcmd, result);
-				}
-			}
-
 			/* Copy comment on index, if requested */
 			if (table_like_clause->options & CREATE_TABLE_LIKE_COMMENTS)
 			{
@@ -1661,8 +1485,8 @@ transformOfType(CreateStmtContext *cxt, TypeName *ofTypename)
  * with the index there.
  *
  * Unlike transformIndexConstraint, we don't make any effort to force primary
- * key columns to be not-null.  The larger cloning process this is part of
- * should have cloned their not-null status separately (and DefineIndex will
+ * key columns to be NOT NULL.  The larger cloning process this is part of
+ * should have cloned their NOT NULL status separately (and DefineIndex will
  * complain if that fails to happen).
  */
 IndexStmt *
@@ -2210,12 +2034,10 @@ transformIndexConstraints(CreateStmtContext *cxt)
 	ListCell   *lc;
 
 	/*
-	 * Run through the constraints that need to generate an index, and do so.
-	 *
-	 * For PRIMARY KEY, in addition we set each column's attnotnull flag true.
-	 * We do not create a separate not-null constraint, as that would be
-	 * redundant: the PRIMARY KEY constraint itself fulfills that role.  Other
-	 * constraint types don't need any not-null markings.
+	 * Run through the constraints that need to generate an index. For PRIMARY
+	 * KEY, mark each column as NOT NULL and create an index. For UNIQUE or
+	 * EXCLUDE, create an index as for PRIMARY KEY, but do not insist on NOT
+	 * NULL.
 	 */
 	foreach(lc, cxt->ixconstraints)
 	{
@@ -2289,7 +2111,9 @@ transformIndexConstraints(CreateStmtContext *cxt)
 	}
 
 	/*
-	 * Now append all the IndexStmts to cxt->alist.
+	 * Now append all the IndexStmts to cxt->alist.  If we generated an ALTER
+	 * TABLE SET NOT NULL statement to support a primary key, it's already in
+	 * cxt->alist.
 	 */
 	cxt->alist = list_concat(cxt->alist, finalindexlist);
 }
@@ -2297,10 +2121,12 @@ transformIndexConstraints(CreateStmtContext *cxt)
 /*
  * transformIndexConstraint
  *		Transform one UNIQUE, PRIMARY KEY, or EXCLUDE constraint for
- *		transformIndexConstraints. An IndexStmt is returned.
+ *		transformIndexConstraints.
  *
- * For a PRIMARY KEY constraint, we additionally force the columns to be
- * marked as not-null, without producing a not-null constraint.
+ * We return an IndexStmt.  For a PRIMARY KEY constraint, we additionally
+ * produce not-null constraints, either by marking ColumnDefs in cxt->columns
+ * as is_not_null or by adding an ALTER TABLE SET NOT NULL command to
+ * cxt->alist.
  */
 static IndexStmt *
 transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
@@ -2564,7 +2390,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
 	 * For UNIQUE and PRIMARY KEY, we just have a list of column names.
 	 *
 	 * Make sure referenced keys exist.  If we are making a PRIMARY KEY index,
-	 * also make sure they are not-null.
+	 * also make sure they are NOT NULL.
 	 */
 	else
 	{
@@ -2572,6 +2398,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
 		{
 			char	   *key = strVal(lfirst(lc));
 			bool		found = false;
+			bool		forced_not_null = false;
 			ColumnDef  *column = NULL;
 			ListCell   *columns;
 			IndexElem  *iparam;
@@ -2592,14 +2419,13 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
 				 * column is defined in the new table.  For PRIMARY KEY, we
 				 * can apply the not-null constraint cheaply here ... unless
 				 * the column is marked is_from_type, in which case marking it
-				 * here would be ineffective (see MergeAttributes).  Note that
-				 * this isn't effective in ALTER TABLE either, unless the
-				 * column is being added in the same command.
+				 * here would be ineffective (see MergeAttributes).
 				 */
 				if (constraint->contype == CONSTR_PRIMARY &&
 					!column->is_from_type)
 				{
 					column->is_not_null = true;
+					forced_not_null = true;
 				}
 			}
 			else if (SystemAttributeByName(key) != NULL)
@@ -2607,7 +2433,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
 				/*
 				 * column will be a system column in the new table, so accept
 				 * it. System columns can't ever be null, so no need to worry
-				 * about PRIMARY/NOT NULL constraint.
+				 * about PRIMARY/not-null constraint.
 				 */
 				found = true;
 			}
@@ -2642,6 +2468,14 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
 						if (strcmp(key, inhname) == 0)
 						{
 							found = true;
+
+							/*
+							 * It's tempting to set forced_not_null if the
+							 * parent column is already NOT NULL, but that
+							 * seems unsafe because the column's NOT NULL
+							 * marking might disappear between now and
+							 * execution.  Do the runtime check to be safe.
+							 */
 							break;
 						}
 					}
@@ -2695,11 +2529,15 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
 			iparam->nulls_ordering = SORTBY_NULLS_DEFAULT;
 			index->indexParams = lappend(index->indexParams, iparam);
 
-			if (constraint->contype == CONSTR_PRIMARY)
+			/*
+			 * For a primary-key column, also create an item for ALTER TABLE
+			 * SET NOT NULL if we couldn't ensure it via is_not_null above.
+			 */
+			if (constraint->contype == CONSTR_PRIMARY && !forced_not_null)
 			{
 				AlterTableCmd *notnullcmd = makeNode(AlterTableCmd);
 
-				notnullcmd->subtype = AT_SetAttNotNull;
+				notnullcmd->subtype = AT_SetNotNull;
 				notnullcmd->name = pstrdup(key);
 				notnullcmds = lappend(notnullcmds, notnullcmd);
 			}
@@ -3642,7 +3480,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
 	cxt.isalter = true;
 	cxt.columns = NIL;
 	cxt.ckconstraints = NIL;
-	cxt.nnconstraints = NIL;
 	cxt.fkconstraints = NIL;
 	cxt.ixconstraints = NIL;
 	cxt.likeclauses = NIL;
@@ -3912,8 +3749,8 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
 
 		/*
 		 * We assume here that cxt.alist contains only IndexStmts and possibly
-		 * AT_SetAttNotNull statements generated from primary key constraints.
-		 * We absorb the subcommands of the latter directly.
+		 * ALTER TABLE SET NOT NULL statements generated from primary key
+		 * constraints.  We absorb the subcommands of the latter directly.
 		 */
 		if (IsA(istmt, IndexStmt))
 		{
@@ -3936,7 +3773,7 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
 	}
 	cxt.alist = NIL;
 
-	/* Append any CHECK, NOT NULL or FK constraints to the commands list */
+	/* Append any CHECK or FK constraints to the commands list */
 	foreach(l, cxt.ckconstraints)
 	{
 		newcmd = makeNode(AlterTableCmd);
@@ -3944,13 +3781,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
 		newcmd->def = (Node *) lfirst_node(Constraint, l);
 		newcmds = lappend(newcmds, newcmd);
 	}
-	foreach(l, cxt.nnconstraints)
-	{
-		newcmd = makeNode(AlterTableCmd);
-		newcmd->subtype = AT_AddConstraint;
-		newcmd->def = (Node *) lfirst_node(Constraint, l);
-		newcmds = lappend(newcmds, newcmd);
-	}
 	foreach(l, cxt.fkconstraints)
 	{
 		newcmd = makeNode(AlterTableCmd);