Feedback-ID: ia2694551:Fastmail
Date: Mon, 5 Feb 2024 19:11:18 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	Dean Rasheed <dean.a.rasheed@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: <202402051811.kqefo2toa3yg@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nikzhkweprj7v6js"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202402051447.wimb4xmtiiyb@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202402051811.kqefo2toa3yg%40alvherre.pgsql>
Precedence: bulk


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

On 2024-Feb-05, Alvaro Herrera wrote:

> While playing with it I noticed this other behavior change from 16,
> 
> create table pa (a int primary key) partition by list (a);
> create table pe (a int unique);
> alter table pa attach partition pe for values in (1, null);
> 
> In 16, we get the error:
> ERROR:  column "a" in child table must be marked NOT NULL
> which is correct (because the PK requires not-null).  In master we just
> let that through, but that seems to be a separate bug.

Hmm, so my initial reaction was to make the constraint-matching code
ignore the constraint in the partition-to-be if it's not the same type
(this is what patch 0002 here does) ... but what ends up happening is
that we create a separate, identical constraint+index for the primary
key.  I don't like that behavior too much myself, as it seems too
magical and surprising, since it could cause the ALTER TABLE ATTACH
operation of a large partition become costly and slower, since it needs
to create an index instead of merely scanning the whole data.

I'll look again at the idea of raising an error if the not-null
constraint is not already present.  That seems safer (and also, it's
what we've been doing all along).

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

--nikzhkweprj7v6js
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0001-Fix-failure-to-merge-NOT-NULL-constraints-in-inhe.patch"

From 6a9feb7800675983198fbb3928c3f34360ac5a49 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 5 Feb 2024 10:18:43 +0100
Subject: [PATCH v2 1/2] Fix failure to merge NOT NULL constraints in
 inheritance

set_attnotnull() was not careful to CommandCounterIncrement() in cases
of multiple recursion.  Omission in b0e96f311985.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/045dec3f-9b3d-aa44-0c99-85f6992306c7@gmail.com
---
 src/backend/commands/tablecmds.c          | 4 ++++
 src/test/regress/expected/constraints.out | 1 -
 src/test/regress/expected/inherit.out     | 8 ++++++++
 src/test/regress/sql/inherit.sql          | 8 ++++++++
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9f51696740..02724d5f04 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7703,6 +7703,10 @@ set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse,
 		List	   *children;
 		ListCell   *lc;
 
+		/* Make above catalog changes visible */
+		if (retval)
+			CommandCounterIncrement();
+
 		children = find_inheritance_children(RelationGetRelid(rel), lockmode);
 		foreach(lc, children)
 		{
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index 5b068477bf..bef3d6577c 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -1010,7 +1010,6 @@ ERROR:  constraint "cnn_parent_pkey" of relation "cnn_parent" does not exist
 create table cnn2_parted(a int primary key) partition by list (a);
 create table cnn2_part1(a int);
 alter table cnn2_parted attach partition cnn2_part1 for values in (1);
-ERROR:  primary key column "a" is not marked NOT NULL
 drop table cnn2_parted, cnn2_part1;
 create table cnn2_parted(a int not null) partition by list (a);
 create table cnn2_part1(a int primary key);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 130a924228..fe33bc4c2f 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2496,6 +2496,14 @@ drop table inh_p1, inh_p2, inh_p3, inh_p4 cascade;
 NOTICE:  drop cascades to 2 other objects
 DETAIL:  drop cascades to table inh_multiparent
 drop cascades to table inh_multiparent2
+-- recursively add column with constraint while merging existing column
+create table inh_p1 ();
+create table inh_p2 (f1 int) inherits (inh_p1);
+create table inh_p3 () inherits (inh_p1, inh_p2);
+alter table inh_p1 add column f1 int not null;
+NOTICE:  merging definition of column "f1" for child "inh_p2"
+NOTICE:  merging definition of column "f1" for child "inh_p3"
+drop table inh_p1, inh_p2, inh_p3;
 --
 -- Mixed ownership inheritance tree
 --
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 0ef9a61bc1..41cdde1d90 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -961,6 +961,14 @@ select conrelid::regclass, contype, conname,
 
 drop table inh_p1, inh_p2, inh_p3, inh_p4 cascade;
 
+-- recursively add column with constraint while merging existing column
+create table inh_p1 ();
+create table inh_p2 (f1 int) inherits (inh_p1);
+create table inh_p3 () inherits (inh_p1, inh_p2);
+alter table inh_p1 add column f1 int not null;
+
+drop table inh_p1, inh_p2, inh_p3;
+
 --
 -- Mixed ownership inheritance tree
 --
-- 
2.39.2


--nikzhkweprj7v6js
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0002-ATTACH-PARTITION-Don-t-let-a-UNIQUE-constraint-ma.patch"

From e871a4a991762ec5312239aa1a1e0ff918d6ce90 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 5 Feb 2024 19:05:34 +0100
Subject: [PATCH v2 2/2] ATTACH PARTITION: Don't let a UNIQUE constraint match
 a PRIMARY KEY

---
 src/backend/commands/tablecmds.c    |  5 +++++
 src/backend/utils/cache/lsyscache.c | 25 +++++++++++++++++++++++++
 src/include/utils/lsyscache.h       |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 02724d5f04..cd4687fb7f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -19269,6 +19269,11 @@ AttachPartitionEnsureIndexes(List **wqueue, Relation rel, Relation attachrel)
 					/* no dice */
 					if (!OidIsValid(cldConstrOid))
 						continue;
+
+					/* Ensure they're both the same type of constraint */
+					if (get_constraint_type(constraintOid) !=
+						get_constraint_type(cldConstrOid))
+						continue;
 				}
 
 				/* bingo. */
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index f730aa26c4..da737786ad 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -1132,6 +1132,31 @@ get_constraint_index(Oid conoid)
 		return InvalidOid;
 }
 
+/*
+ * get_constraint_type
+ *		Return the pg_constraint.contype value for the given constraint.
+ *
+ * No frills.
+ */
+char
+get_constraint_type(Oid conoid)
+{
+	HeapTuple	tp;
+
+	tp = SearchSysCache1(CONSTROID, ObjectIdGetDatum(conoid));
+	if (HeapTupleIsValid(tp))
+	{
+		Form_pg_constraint contup = (Form_pg_constraint) GETSTRUCT(tp);
+		char	result;
+
+		result = contup->contype;
+		ReleaseSysCache(tp);
+		return result;
+	}
+
+	return '\0';
+}
+
 /*				---------- LANGUAGE CACHE ----------					 */
 
 char *
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index e4a200b00e..fe694f5c4a 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -100,6 +100,8 @@ extern char *get_collation_name(Oid colloid);
 extern bool get_collation_isdeterministic(Oid colloid);
 extern char *get_constraint_name(Oid conoid);
 extern Oid	get_constraint_index(Oid conoid);
+extern char get_constraint_type(Oid conoid);
+
 extern char *get_language_name(Oid langoid, bool missing_ok);
 extern Oid	get_opclass_family(Oid opclass);
 extern Oid	get_opclass_input_type(Oid opclass);
-- 
2.39.2


--nikzhkweprj7v6js--