Revert "Catalog NOT NULL constraints" and fallout

This reverts commit e056c557aef4 and minor later fixes thereof.

There's a few problems in this new feature -- most notably regarding
pg_upgrade behavior, but others as well.  This new feature is not in any
way critical on its own, so instead of scrambling to fix it we revert it
and try again in early 17 with these issues in mind.

Discussion: https://postgr.es/m/3801207.1681057430@sss.pgh.pa.us

1 files changed, 55 insertions, 151 deletions

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index ea0fe6f8116..b0f6fe4fa61 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -83,7 +83,6 @@ typedef struct
 	List	   *ckconstraints;	/* CHECK constraints */
 	List	   *fkconstraints;	/* FOREIGN KEY constraints */
 	List	   *ixconstraints;	/* index-creating constraints */
-	List	   *nnconstraints;	/* NOT NULL constraints */
 	List	   *likeclauses;	/* LIKE clauses that need post-processing */
 	List	   *extstats;		/* cloned extended statistics */
 	List	   *blist;			/* "before list" of things to do before
@@ -245,7 +244,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
 	cxt.ckconstraints = NIL;
 	cxt.fkconstraints = NIL;
 	cxt.ixconstraints = NIL;
-	cxt.nnconstraints = NIL;
 	cxt.likeclauses = NIL;
 	cxt.extstats = NIL;
 	cxt.blist = NIL;
@@ -350,7 +348,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);
@@ -540,7 +537,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);
@@ -638,8 +634,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... */
@@ -657,7 +655,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\"",
@@ -669,58 +667,15 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 				break;
 
 			case CONSTR_NOTNULL:
-
-				/*
-				 * For NOT NULL declarations, we need to mark the column as
-				 * not nullable, and set things up to have a CHECK constraint
-				 * created.  Also, duplicate NOT NULL declarations are not
-				 * allowed.
-				 */
-				if (saw_nullable)
-				{
-					if (!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)));
-					else
-						ereport(ERROR,
-								errcode(ERRCODE_SYNTAX_ERROR),
-								errmsg("redundant NOT NULL declarations for column \"%s\" of table \"%s\"",
-									   column->colname, cxt->relation->relname),
-								parser_errposition(cxt->pstate,
-												   constraint->location));
-				}
-
-				/*
-				 * If this is the first time we see this column being marked
-				 * not null, keep track to later add a NOT NULL constraint.
-				 */
-				if (!column->is_not_null)
-				{
-					Constraint *notnull;
-
-					column->is_not_null = true;
-					saw_nullable = true;
-
-					notnull = makeNode(Constraint);
-					notnull->contype = CONSTR_NOTNULL;
-					notnull->conname = constraint->conname;
-					notnull->deferrable = false;
-					notnull->initdeferred = false;
-					notnull->location = -1;
-					notnull->colname = column->colname;
-					notnull->skip_validation = false;
-					notnull->initially_valid = true;
-
-					cxt->nnconstraints = lappend(cxt->nnconstraints, notnull);
-
-					/* Don't need this anymore, if we had it */
-					need_notnull = false;
-				}
-
+				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;
 
 			case CONSTR_DEFAULT:
@@ -770,19 +725,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;
 				}
 
@@ -806,11 +758,6 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 
 			case CONSTR_CHECK:
 				cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
-
-				/*
-				 * XXX If the user says CHECK (IS NOT NULL), should we turn
-				 * that into a regular NOT NULL constraint?
-				 */
 				break;
 
 			case CONSTR_PRIMARY:
@@ -894,29 +841,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->colname = 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.
 	 */
@@ -991,10 +915,6 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *constraint)
 			cxt->ckconstraints = lappend(cxt->ckconstraints, constraint);
 			break;
 
-		case CONSTR_NOTNULL:
-			cxt->nnconstraints = lappend(cxt->nnconstraints, constraint);
-			break;
-
 		case CONSTR_FOREIGN:
 			if (cxt->isforeign)
 				ereport(ERROR,
@@ -1006,6 +926,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:
@@ -1041,7 +962,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);
@@ -1123,8 +1043,6 @@ transformTableLikeClause(CreateStmtContext *cxt, TableLikeClause *table_like_cla
 		def->inhcount = 0;
 		def->is_local = true;
 		def->is_not_null = attribute->attnotnull;
-		if (attribute->attnotnull)
-			process_notnull_constraints = true;
 		def->is_from_type = false;
 		def->storage = 0;
 		def->raw_default = NULL;
@@ -1206,19 +1124,14 @@ 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 &
+	if (table_like_clause->options &
 		(CREATE_TABLE_LIKE_DEFAULTS |
 		 CREATE_TABLE_LIKE_GENERATED |
 		 CREATE_TABLE_LIKE_CONSTRAINTS |
-		 CREATE_TABLE_LIKE_INDEXES)) ||
-		process_notnull_constraints)
+		 CREATE_TABLE_LIKE_INDEXES))
 	{
 		table_like_clause->relationOid = RelationGetRelid(relation);
 		cxt->likeclauses = lappend(cxt->likeclauses, table_like_clause);
@@ -1290,7 +1203,6 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause)
 	TupleConstr *constr;
 	AttrMap    *attmap;
 	char	   *comment;
-	ListCell   *lc;
 
 	/*
 	 * Open the relation referenced by the LIKE clause.  We should still have
@@ -1471,20 +1383,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(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.
@@ -2161,12 +2059,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 CHECK (IS 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)
 	{
@@ -2240,7 +2136,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);
 }
@@ -2248,10 +2146,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 CHECK (IS 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)
@@ -2517,6 +2417,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;
@@ -2537,14 +2438,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)
@@ -2587,6 +2487,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;
 						}
 					}
@@ -2640,11 +2548,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);
 			}
@@ -3416,7 +3328,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;
@@ -3660,8 +3571,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))
 		{
@@ -3689,21 +3600,14 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
 	{
 		newcmd = makeNode(AlterTableCmd);
 		newcmd->subtype = AT_AddConstraint;
-		newcmd->def = (Node *) lfirst_node(Constraint, l);
+		newcmd->def = (Node *) lfirst(l);
 		newcmds = lappend(newcmds, newcmd);
 	}
 	foreach(l, cxt.fkconstraints)
 	{
 		newcmd = makeNode(AlterTableCmd);
 		newcmd->subtype = AT_AddConstraint;
-		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);
+		newcmd->def = (Node *) lfirst(l);
 		newcmds = lappend(newcmds, newcmd);
 	}