public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alvaro Herrera <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: Dean Rasheed <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: [email protected]
Subject: Re: cataloguing NOT NULL constraints
Date: Mon, 5 Feb 2024 19:11:18 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
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/
Attachments:
[text/x-diff] v2-0001-Fix-failure-to-merge-NOT-NULL-constraints-in-inhe.patch (3.5K, ../[email protected]/2-v2-0001-Fix-failure-to-merge-NOT-NULL-constraints-in-inhe.patch)
download | inline diff:
From 6a9feb7800675983198fbb3928c3f34360ac5a49 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email protected]>
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 <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
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
[text/x-diff] v2-0002-ATTACH-PARTITION-Don-t-let-a-UNIQUE-constraint-ma.patch (2.4K, ../[email protected]/3-v2-0002-ATTACH-PARTITION-Don-t-let-a-UNIQUE-constraint-ma.patch)
download | inline diff:
From e871a4a991762ec5312239aa1a1e0ff918d6ce90 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email protected]>
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
view thread (2+ messages)
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: cataloguing NOT NULL constraints
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox