Feedback-ID: ia2694551:Fastmail
Date: Wed, 1 May 2024 19:49:35 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>,
	Andrew Bille <andrewbille@gmail.com>,
 Peter Eisentraut <peter@eisentraut.org>,
	Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
Message-ID: <202405011749.txscey5apbn6@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="y4rrcdzr7pstote7"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202404251016.m2jyipj2zaav@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202405011749.txscey5apbn6%40alvherre.pgsql>
Precedence: bulk


--y4rrcdzr7pstote7
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2024-Apr-25, Alvaro Herrera wrote:

> > Also, I've found a weird behaviour with a non-inherited NOT NULL
> > constraint for a partitioned table:
> > CREATE TABLE pt(a int NOT NULL NO INHERIT) PARTITION BY LIST (a);

> Ugh.  Maybe a way to handle this is to disallow NO INHERIT in
> constraints on partitioned tables altogether.  I mean, they are a
> completely useless gimmick, aren't they?

Here are two patches that I intend to push soon (hopefully tomorrow).

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)

--y4rrcdzr7pstote7
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-Disallow-direct-change-of-NO-INHERIT-of-not-null-con.patch"

From 97318ac81cfe82cf52629f141b26a5a497b0913c Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Wed, 1 May 2024 17:32:50 +0200
Subject: [PATCH 1/2] Disallow direct change of NO INHERIT of not-null
 constraints

We support changing NO INHERIT constraint to INHERIT for constraints in
child relations when adding a constraint to some ancestor relation, and
also during pg_upgrade's schema restore; but other than those special
cases, command ALTER TABLE ADD CONSTRAINT should not be allowed to
change an existing constraint from NO INHERIT to INHERIT, as that would
require to process child relations so that they also acquire an
appropriate constraint, which we may not be in a position to do.  (It'd
also be surprising behavior.)

It is conceivable that we want to allow ALTER TABLE SET NOT NULL to make
such a change; but in that case some more code is needed to implement it
correctly, so for now I've made that throw the same error message.

Also, during the prep phase of ALTER TABLE ADD CONSTRAINT, acquire locks
on all descendant tables; otherwise we might operate on child tables on
which no locks are held, particularly in the mode where a primary key
causes not-null constraints to be created on children.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/7d923a66-55f0-3395-cd40-81c142b5448b@gmail.com
---
 doc/src/sgml/ref/alter_table.sgml     |  2 +-
 src/backend/catalog/heap.c            | 20 +++++++++++++++-----
 src/backend/catalog/pg_constraint.c   | 15 +++++++++++----
 src/backend/commands/tablecmds.c      | 17 +++++++++++++++--
 src/include/catalog/pg_constraint.h   |  2 +-
 src/test/regress/expected/inherit.out | 24 +++++++++++++++++++++++-
 src/test/regress/sql/inherit.sql      | 15 +++++++++++++++
 7 files changed, 81 insertions(+), 14 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index ebd8c62038..0bf11f6cb6 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -105,7 +105,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
 <phrase>and <replaceable class="parameter">column_constraint</replaceable> is:</phrase>
 
 [ CONSTRAINT <replaceable class="parameter">constraint_name</replaceable> ]
-{ NOT NULL |
+{ NOT NULL [ NO INHERIT ] |
   NULL |
   CHECK ( <replaceable class="parameter">expression</replaceable> ) [ NO INHERIT ] |
   DEFAULT <replaceable>default_expr</replaceable> |
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index f0278b9c01..136cc42a92 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2549,13 +2549,23 @@ AddRelationNewConstraints(Relation rel,
 
 			/*
 			 * If the column already has an inheritable not-null constraint,
-			 * we need only adjust its inheritance status and we're done.  If
-			 * the constraint is there but marked NO INHERIT, then it is
-			 * updated in the same way, but we also recurse to the children
-			 * (if any) to add the constraint there as well.
+			 * we need only adjust its coninhcount and we're done.  In certain
+			 * cases (see below), if the constraint is there but marked NO
+			 * INHERIT, then we mark it as no longer such and coninhcount
+			 * updated, plus we must also recurse to the children (if any) to
+			 * add the constraint there.
+			 *
+			 * We only allow the inheritability status to change during binary
+			 * upgrade (where it's used to add the not-null constraints for
+			 * children of tables with primary keys), or when we're recursing
+			 * processing a table down an inheritance hierarchy; directly
+			 * allowing a constraint to change from NO INHERIT to INHERIT
+			 * during ALTER TABLE ADD CONSTRAINT would be far too surprising
+			 * behavior.
 			 */
 			existing = AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
-												 cdef->inhcount, cdef->is_no_inherit);
+												 cdef->inhcount, cdef->is_no_inherit,
+												 IsBinaryUpgrade || allow_merge);
 			if (existing == 1)
 				continue;		/* all done! */
 			else if (existing == -1)
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index aaf3537d3f..6b8496e085 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -721,7 +721,7 @@ extractNotNullColumn(HeapTuple constrTup)
  */
 int
 AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
-						  bool is_no_inherit)
+						  bool is_no_inherit, bool allow_noinherit_change)
 {
 	HeapTuple	tup;
 
@@ -744,16 +744,23 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
 		if (is_no_inherit && !conform->connoinherit)
 			ereport(ERROR,
 					errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-					errmsg("cannot change NO INHERIT status of inherited NOT NULL constraint \"%s\" on relation \"%s\"",
+					errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"",
 						   NameStr(conform->conname), get_rel_name(relid)));
 
 		/*
 		 * If the constraint already exists in this relation but it's marked
-		 * NO INHERIT, we can just remove that flag, and instruct caller to
-		 * recurse to add the constraint to children.
+		 * NO INHERIT, we can just remove that flag (provided caller allows
+		 * such a change), and instruct caller to recurse to add the
+		 * constraint to children.
 		 */
 		if (!is_no_inherit && conform->connoinherit)
 		{
+			if (!allow_noinherit_change)
+				ereport(ERROR,
+						errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+						errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",
+							   NameStr(conform->conname), get_rel_name(relid)));
+
 			conform->connoinherit = false;
 			retval = -1;		/* caller must add constraint on child rels */
 		}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 08c87e6029..c2f0a2f5a4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4980,10 +4980,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			break;
 		case AT_AddConstraint:	/* ADD CONSTRAINT */
 			ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
-			/* Recursion occurs during execution phase */
-			/* No command-specific prep needed except saving recurse flag */
 			if (recurse)
+			{
+				/* if recursing, set flag and lock all descendants */
 				cmd->recurse = true;
+				(void) find_all_inheritors(RelationGetRelid(rel), lockmode, NULL);
+			}
 			pass = AT_PASS_ADD_CONSTR;
 			break;
 		case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
@@ -7892,6 +7894,17 @@ ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName,
 		copytup = heap_copytuple(tuple);
 		conForm = (Form_pg_constraint) GETSTRUCT(copytup);
 
+		/*
+		 * Don't let a NO INHERIT constraint be changed into inherit.
+		 */
+		if (conForm->connoinherit && recurse)
+			ereport(ERROR,
+					errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",
+						   NameStr(conForm->conname),
+						   RelationGetRelationName(rel)));
+
+
 		/*
 		 * If we find an appropriate constraint, we're almost done, but just
 		 * need to change some properties on it: if we're recursing, increment
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 5c52d71e09..68bf55fdf7 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -262,7 +262,7 @@ extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
 extern HeapTuple findDomainNotNullConstraint(Oid typid);
 extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
 extern int	AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
-									  bool is_no_inherit);
+									  bool is_no_inherit, bool allow_noinherit_change);
 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 5a5c23fc3b..ab67a9369d 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2126,7 +2126,7 @@ ERROR:  cannot define not-null constraint on column "a2" with NO INHERIT
 DETAIL:  The column 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"
+ERROR:  cannot change NO INHERIT status of 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"
@@ -2277,6 +2277,28 @@ Child tables: inh_nn_child,
               inh_nn_child2
 
 drop table inh_nn_parent, inh_nn_child, inh_nn_child2;
+CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT);
+CREATE TABLE inh_nn_child() INHERITS (inh_nn_parent);
+ALTER TABLE inh_nn_parent ADD CONSTRAINT nna NOT NULL a;
+ERROR:  cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" in relation "inh_nn_parent"
+ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
+ERROR:  cannot change NO INHERIT status of NOT NULL constraint "inh_nn_parent_a_not_null" in relation "inh_nn_parent"
+DROP TABLE inh_nn_parent cascade;
+NOTICE:  drop cascades to table inh_nn_child
+CREATE TABLE inh_nn_lvl1 (a int);
+CREATE TABLE inh_nn_lvl2 () INHERITS (inh_nn_lvl1);
+CREATE TABLE inh_nn_lvl3 (CONSTRAINT foo NOT NULL a NO INHERIT) INHERITS (inh_nn_lvl2);
+CREATE TABLE inh_nn_lvl4 () INHERITS (inh_nn_lvl3);
+CREATE TABLE inh_nn_lvl5 () INHERITS (inh_nn_lvl4);
+INSERT INTO inh_nn_lvl5 VALUES (NULL);
+ALTER TABLE inh_nn_lvl1 ADD PRIMARY KEY (a);
+ERROR:  column "a" of relation "inh_nn_lvl5" contains null values
+DROP TABLE inh_nn_lvl1 CASCADE;
+NOTICE:  drop cascades to 4 other objects
+DETAIL:  drop cascades to table inh_nn_lvl2
+drop cascades to table inh_nn_lvl3
+drop cascades to table inh_nn_lvl4
+drop cascades to table inh_nn_lvl5
 --
 -- test inherit/deinherit
 --
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 277fb74d2c..624134b0f9 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -844,6 +844,21 @@ select conrelid::regclass, conname, contype, conkey,
 \d+ inh_nn*
 drop table inh_nn_parent, inh_nn_child, inh_nn_child2;
 
+CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT);
+CREATE TABLE inh_nn_child() INHERITS (inh_nn_parent);
+ALTER TABLE inh_nn_parent ADD CONSTRAINT nna NOT NULL a;
+ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
+DROP TABLE inh_nn_parent cascade;
+
+CREATE TABLE inh_nn_lvl1 (a int);
+CREATE TABLE inh_nn_lvl2 () INHERITS (inh_nn_lvl1);
+CREATE TABLE inh_nn_lvl3 (CONSTRAINT foo NOT NULL a NO INHERIT) INHERITS (inh_nn_lvl2);
+CREATE TABLE inh_nn_lvl4 () INHERITS (inh_nn_lvl3);
+CREATE TABLE inh_nn_lvl5 () INHERITS (inh_nn_lvl4);
+INSERT INTO inh_nn_lvl5 VALUES (NULL);
+ALTER TABLE inh_nn_lvl1 ADD PRIMARY KEY (a);
+DROP TABLE inh_nn_lvl1 CASCADE;
+
 --
 -- test inherit/deinherit
 --
-- 
2.39.2


--y4rrcdzr7pstote7
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0002-Disallow-NO-INHERIT-not-null-constraints-on-partitio.patch"

From 3aa60ba61d2edd1edaf77ea6d4ae50a1e50f6039 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Wed, 1 May 2024 19:46:52 +0200
Subject: [PATCH 2/2] Disallow NO INHERIT not-null constraints on partitioned
 tables

They are semantically useless and only bring weird cases. Reject them.

Maybe this should be done for all constraints, not just not-null ones.

Per note by Alexander Lakhin.
---
 src/backend/parser/parse_utilcmd.c        | 10 ++++++++++
 src/bin/pg_dump/pg_dump.c                 | 16 ++++++++++++++--
 src/test/regress/expected/constraints.out |  5 +++++
 src/test/regress/sql/constraints.sql      |  4 ++++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index fef084f5d5..9fb6ff86db 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -679,6 +679,10 @@ 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
@@ -969,6 +973,12 @@ transformTableConstraint(CreateStmtContext *cxt, Constraint *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;
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 242ebe807f..1eb58a447e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -9052,7 +9052,15 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
 								 tbinfo->attnames[j]);
 				}
 				else if (PQgetvalue(res, r, i_notnull_is_pk)[0] == 't')
-					use_throwaway_notnull = true;
+				{
+					/*
+					 * We want this flag to be set for columns of a primary key
+					 * in which data is going to be loaded by the dump we
+					 * produce; thus a partitioned table doesn't need it.
+					 */
+					if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
+						use_throwaway_notnull = true;
+				}
 				else if (!PQgetisnull(res, r, i_notnull_name))
 					use_unnamed_notnull = true;
 			}
@@ -9092,7 +9100,11 @@ getTableAttrs(Archive *fout, TableInfo *tblinfo, int numTables)
 					}
 				}
 				else if (PQgetvalue(res, r, i_notnull_is_pk)[0] == 't')
-					use_throwaway_notnull = true;
+				{
+					/* see above */
+					if (tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
+						use_throwaway_notnull = true;
+				}
 			}
 
 			if (use_unnamed_notnull)
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index 2fc0be7925..ec7c9e53d0 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -321,6 +321,11 @@ ALTER TABLE ATACC1 ADD NOT NULL a NO INHERIT;
 Inherits: atacc1
 
 DROP TABLE ATACC1, ATACC2;
+-- no can do
+CREATE TABLE ATACC1 (a int NOT NULL NO INHERIT) PARTITION BY LIST (a);
+ERROR:  not-null constraints on partitioned tables cannot be NO INHERIT
+CREATE TABLE ATACC1 (a int, NOT NULL a NO INHERIT) PARTITION BY LIST (a);
+ERROR:  not-null constraints on partitioned tables cannot be NO INHERIT
 -- overridding a no-inherit constraint with an inheritable one
 CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
 CREATE TABLE ATACC1 (a int);
diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql
index 8f85e72050..e753b8c345 100644
--- a/src/test/regress/sql/constraints.sql
+++ b/src/test/regress/sql/constraints.sql
@@ -212,6 +212,10 @@ ALTER TABLE ATACC1 ADD NOT NULL a NO INHERIT;
 \d+ ATACC2
 DROP TABLE ATACC1, ATACC2;
 
+-- no can do
+CREATE TABLE ATACC1 (a int NOT NULL NO INHERIT) PARTITION BY LIST (a);
+CREATE TABLE ATACC1 (a int, NOT NULL a NO INHERIT) PARTITION BY LIST (a);
+
 -- overridding a no-inherit constraint with an inheritable one
 CREATE TABLE ATACC2 (a int, CONSTRAINT a_is_not_null NOT NULL a NO INHERIT);
 CREATE TABLE ATACC1 (a int);
-- 
2.39.2


--y4rrcdzr7pstote7--