Date: Tue, 29 Aug 2023 13:53:34 +0900 (JST)
Message-Id: <20230829.135334.377739201526442411.horikyota.ntt@gmail.com>
To: alvherre@alvh.no-ip.org
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Strange presentaion related to inheritance in \d+
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <20230828113600.kdzqev7e4gcdkdig@alvherre.pgsql>
References: <20230828.161658.1184657435220765047.horikyota.ntt@gmail.com>
	<20230828113600.kdzqev7e4gcdkdig@alvherre.pgsql>
User-Agent: Mew version 6.8 on Emacs 27.2
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Tue_Aug_29_13_53_34_2023_139)--"
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20230829.135334.377739201526442411.horikyota.ntt%40gmail.com>
Precedence: bulk

----Next_Part(Tue_Aug_29_13_53_34_2023_139)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

At Mon, 28 Aug 2023 13:36:00 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 
> On 2023-Aug-28, Kyotaro Horiguchi wrote:
> 
> > But with these tables:
> > 
> > create table p (a int, b int not null default 0);
> > create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);
> > 
> > I get:
> > 
> > > Not-null constraints:
> > >    "c1_b_not_null" NOT NULL "b" *NO INHERIT*
> > 
> > Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
> > "coninhcount <> 0" align with "local" and "inherited". For a clearer
> > picuture, those values for c1 are as follows.
> 
> Hmm, I think the bug here is that we let you create a constraint in c1
> that is NOINHERIT.  If the parent already has one INHERIT constraint
> in that column, then the child must have that one also; it's not
> possible to have both a constraint that inherits and one that doesn't.

Yeah, I had the same question about the coexisting of the two.

> I understand that there are only three possibilities for a NOT NULL
> constraint in a column:
> 
> - There's a NO INHERIT constraint.  A NO INHERIT constraint is always
>   defined locally in that table.  In this case, if there is a parent
>   relation, then it must either not have a NOT NULL constraint in that
>   column, or it may also have a NO INHERIT one.  Therefore, it's
>   correct to print NO INHERIT and nothing else.  We could also print
>   "(local)" but I see no point in doing that.
> 
> - A constraint comes inherited from one or more parent tables and has no
>   local definition.  In this case, the constraint always inherits
>   (otherwise, the parent wouldn't have given it to this table).  So
>   printing "(inherited)" and nothing else is correct.
> 
> - A constraint can have a local definition and also be inherited.  In
>   this case, printing "(local, inherited)" is correct.
> 
> Have I missed other cases?

Seems correct. I don't see another case given that NO INHERIT is
inhibited when a table has an inherited constraint.

> The NO INHERIT bit is part of the syntax, which is why I put it in
> uppercase and not marked it for translation.  The other two are
> informational, so they are translatable.

Given the conditions above, I agree with you.

Attached is the initial version of the patch. It prevents "CREATE
TABLE" from executing if there is an inconsisntent not-null
constraint.  Also I noticed that "ALTER TABLE t ADD NOT NULL c NO
INHERIT" silently ignores the "NO INHERIT" part and fixed it.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

----Next_Part(Tue_Aug_29_13_53_34_2023_139)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="inhibit_inconsistent_not_null_no_inherit.patch"

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index b534da7d80..39fbb0f151 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2533,7 +2533,7 @@ AddRelationNewConstraints(Relation rel,
 			 * update its catalog status and we're done.
 			 */
 			if (AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
-										  cdef->inhcount))
+										  cdef->inhcount, cdef->is_no_inherit))
 				continue;
 
 			/*
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 2a725f6280..bd589c0e7c 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -669,7 +669,8 @@ extractNotNullColumn(HeapTuple constrTup)
  * If no not-null constraint is found for the column, return false.
  */
 bool
-AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count)
+AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
+						  bool is_no_inherit)
 {
 	HeapTuple	tup;
 
@@ -681,6 +682,14 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count)
 
 		pg_constraint = table_open(ConstraintRelationId, RowExclusiveLock);
 		conform = (Form_pg_constraint) GETSTRUCT(tup);
+
+		if (is_no_inherit)
+			ereport(ERROR,
+					errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+					errmsg("cannot change NO INHERIT status of inherited NOT NULL constraint \"%s\" on relation \"%s\"",
+						   NameStr(conform->conname), get_rel_name(relid)));
+
+
 		if (count > 0)
 			conform->coninhcount += count;
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 47c900445c..ae5ef6e115 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -350,8 +350,8 @@ static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
 static void truncate_check_activity(Relation rel);
 static void RangeVarCallbackForTruncate(const RangeVar *relation,
 										Oid relId, Oid oldRelId, void *arg);
-static List *MergeAttributes(List *schema, List *supers, char relpersistence,
-							 bool is_partition, List **supconstr,
+static List *MergeAttributes(List *schema, List *supers, List *nnconstr,
+							 char relpersistence, bool is_partition, List **supconstr,
 							 List **supnotnulls);
 static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
 static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
@@ -873,7 +873,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	 * modified by MergeAttributes.)
 	 */
 	stmt->tableElts =
-		MergeAttributes(stmt->tableElts, inheritOids,
+		MergeAttributes(stmt->tableElts, inheritOids, stmt->nnconstraints,
 						stmt->relation->relpersistence,
 						stmt->partbound != NULL,
 						&old_constraints, &old_notnulls);
@@ -2318,6 +2318,7 @@ storage_name(char c)
  * 'schema' is the column/attribute definition for the table. (It's a list
  *		of ColumnDef's.) It is destructively changed.
  * 'supers' is a list of OIDs of parent relations, already locked by caller.
+ * 'child_nnconstr' is a list of not-null constraints on the child relation.
  * 'relpersistence' is the persistence type of the table.
  * 'is_partition' tells if the table is a partition.
  *
@@ -2377,12 +2378,13 @@ storage_name(char c)
  *----------
  */
 static List *
-MergeAttributes(List *schema, List *supers, char relpersistence,
-				bool is_partition, List **supconstr, List **supnotnulls)
+MergeAttributes(List *schema, List *supers, List *child_nnconstr,
+				char relpersistence, bool is_partition, List **supconstr,
+				List **supnotnulls)
 {
 	List	   *inhSchema = NIL;
 	List	   *constraints = NIL;
-	List	   *nnconstraints = NIL;
+	List	   *super_nnconstr = NIL;
 	bool		have_bogus_defaults = false;
 	int			child_attno;
 	static Node bogus_marker = {0}; /* marks conflicting defaults */
@@ -2718,7 +2720,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 					nn->inhcount = 1;
 					nn->is_no_inherit = false;
 
-					nnconstraints = lappend(nnconstraints, nn);
+					super_nnconstr = lappend(super_nnconstr, nn);
 				}
 
 				/*
@@ -2807,7 +2809,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 					nn->inhcount = 1;
 					nn->is_no_inherit = false;
 
-					nnconstraints = lappend(nnconstraints, nn);
+					super_nnconstr = lappend(super_nnconstr, nn);
 				}
 			}
 
@@ -2967,7 +2969,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 			nn->is_local = false;
 			nn->inhcount = 1;
 
-			nnconstraints = lappend(nnconstraints, nn);
+			super_nnconstr = lappend(super_nnconstr, nn);
 		}
 
 		free_attrmap(newattmap);
@@ -3096,6 +3098,30 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 								 errdetail("%s versus %s", def->compression, newdef->compression)));
 				}
 
+				/*
+				 * Don't allow NO INHERIT when a not-null constraint is
+				 * inherited
+				 */
+				if (def->is_not_null)
+				{
+					ListCell *lcnnconstr;
+
+					foreach(lcnnconstr, child_nnconstr)
+					{
+						Constraint *con = (Constraint *) lfirst(lcnnconstr);
+						char *child_colname = strVal(linitial(con->keys));
+						
+						if (con->is_no_inherit &&
+							strncmp(child_colname, attributeName,
+									NAMEDATALEN) == 0)
+							ereport(ERROR,
+									(errcode(ERRCODE_DATATYPE_MISMATCH),
+									 errmsg("cannot define not-null constraint on column \"%s\" with NO INHERIT",
+											attributeName),
+									 errdetail("The cloumn has an inherited not-null constraint.")));
+					}
+				}
+
 				/*
 				 * Merge of not-null constraints = OR 'em together
 				 */
@@ -3281,7 +3307,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 	}
 
 	*supconstr = constraints;
-	*supnotnulls = nnconstraints;
+	*supnotnulls = super_nnconstr;
 
 	return schema;
 }
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index f6f5796fe0..f366f8cd14 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -248,7 +248,8 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
 extern HeapTuple findNotNullConstraintAttnum(Oid relid, AttrNumber attnum);
 extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
 extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
-extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count);
+extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
+									  bool is_no_inherit);
 extern void AdjustNotNullInheritance(Oid relid, Bitmapset *columns, int count);
 extern List *RelationGetNotNullConstraints(Oid relid, bool cooked);
 
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 6daca12340..7ec00aaebc 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2051,6 +2051,15 @@ Not-null constraints:
 Inherits: pp1,
           cc1
 
+-- cannot create table with inconsistent NO INHERIT contraint: fails
+create table cc3 (a2 int not null no inherit) inherits (cc1);
+NOTICE:  moving and merging column "a2" with inherited definition
+DETAIL:  User-specified column moved to the position of the inherited column.
+ERROR:  cannot define not-null constraint on column "a2" with NO INHERIT
+DETAIL:  The cloumn has an inherited not-null constraint.
+-- change NO INHERIT status of inherited constraint: no dice, it's inherited
+alter table cc2 add not null a2 no inherit;
+ERROR:  cannot change NO INHERIT status of inherited NOT NULL constraint "nn" on relation "cc2"
 -- remove constraint from cc2: no dice, it's inherited
 alter table cc2 alter column a2 drop not null;
 ERROR:  cannot drop inherited constraint "nn" of relation "cc2"
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index d8fae92a53..b58a9286f3 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -736,6 +736,12 @@ alter table pp1 alter column f1 set not null;
 \d+ cc1
 \d+ cc2
 
+-- cannot create table with inconsistent NO INHERIT contraint: fails
+create table cc3 (a2 int not null no inherit) inherits (cc1);
+
+-- change NO INHERIT status of inherited constraint: no dice, it's inherited
+alter table cc2 add not null a2 no inherit;
+
 -- remove constraint from cc2: no dice, it's inherited
 alter table cc2 alter column a2 drop not null;
 

----Next_Part(Tue_Aug_29_13_53_34_2023_139)----