Feedback-ID: ia2694551:Fastmail
Date: Mon, 5 Feb 2024 10:50:56 +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: <202402050950.q367e2d6rtjs@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="5dq2hkzuszcqyiio"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202402050851.2ktovxwc4irp@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202402050950.q367e2d6rtjs%40alvherre.pgsql>
Precedence: bulk


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

On 2024-Feb-05, Alvaro Herrera wrote:

> Hmm, let me have a look, I can probably get this one fixed today before
> embarking on a larger fix elsewhere in the same feature.

You know what -- this missing CCI has a much more visible impact, which
is that the attnotnull marker that a primary key imposes on a partition
is propagated early.  So this regression test no longer fails:

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);

Here, in the existing code the ALTER TABLE ATTACH fails with the error
message that
  ERROR:  primary key column "a" is not marked NOT NULL
but with the patch, this no longer occurs.

I'm not sure that this behavior change is desirable ... I have vague
memories of people complaining that this sort of error was not very
welcome ... but on the other hand it seems now pretty clear that if it
*is* desirable, then its implementation is no good, because a single
added CCI breaks it.

I'm leaning towards accepting the behavior change, but I'd like to
investigate a little bit more first, but what do others think?

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

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

From 486f6bfa8faa6162257c5f12b52180b5a3d89704 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 v1] 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


--5dq2hkzuszcqyiio--