From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Peter Eisentraut <peter@eisentraut.org>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
Date: Mon, 5 Feb 2024 10:50:56 +0100
Message-ID: <202402050950.q367e2d6rtjs@alvherre.pgsql> (raw)
In-Reply-To: <202402050851.2ktovxwc4irp@alvherre.pgsql>
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/Attachments:
[text/x-diff] v1-0001-Fix-failure-to-merge-NOT-NULL-constraints-in-inhe.patch (3.4K, ../202402050950.q367e2d6rtjs@alvherre.pgsql/2-v1-0001-Fix-failure-to-merge-NOT-NULL-constraints-in-inhe.patch)
download | inline diff:
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.cindex 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.outindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, michael@paquier.xyz, exclusion@gmail.com, dean.a.rasheed@gmail.com, peter@eisentraut.org, robertmhaas@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
In-Reply-To: <202402050950.q367e2d6rtjs@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox