Strange presentaion related to inheritance in \d+
While translating a message, I found a questionable behavior in \d+,
introduced by a recent commit b0e96f3119. In short, the current code
hides the constraint's origin when "NO INHERIT" is used.
For these tables:
create table p (a int, b int not null default 0);
create table c1 (a int, b int not null default 1) inherits (p);
The output from "\d+ c1" contains the lines:
Not-null constraints:
"c1_b_not_null" NOT NULL "b" *(local, inherited)*
But with these tables:
create table p (a int, b int not null default 0);
create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);
I get:
Not-null constraints:
"c1_b_not_null" NOT NULL "b" *NO INHERIT*
Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
"coninhcount <> 0" align with "local" and "inherited". For a clearer
picuture, those values for c1 are as follows.
=# SELECT co.conname, at.attname, co.connoinherit, co.conislocal, co.coninhcount FROM pg_catalog.pg_constraint co JOIN pg_catalog.pg_attribute at ON (at.attnum = co.conkey[1]) WHERE co.contype = 'n' AND co.conrelid = 'c1'::pg_catalog.regclass AND at.attrelid = 'c1'::pg_catalog.regclass ORDER BY at.attnum;
conname | attname | connoinherit | conislocal | coninhcount
---------------+---------+--------------+------------+-------------
c1_b_not_null | b | t | t | 1
It feels off to me, but couldn't find any discussion about it. Is it
the intended behavior? I believe it's more appropriate to show the
origins even when specifed as NO INHERIT.
======
If not so, the following change might be possible, which is quite simple.
Not-null constraints:
"c1_b_not_null" NOT NULL "b" NO INHERIT(local, inherited)
However, it looks somewhat strange as the information in parentheses
is not secondary to "NO INHERIT". Thus, perhaps a clearer or more
proper representation would be:
"c1_b_not_null" NOT NULL "b" (local, inherited, not inheritable)
That being said, I don't come up with a simple way to do this for now..
(Note that we need to translate the puctuations and the words.)
There's no need to account for all combinations. "Local" and
"inherited" don't be false at the same time and the combination (local
& !inherited) is not displayed. Given these factors, we're left with 6
possible combinations, which I don't think aren't worth the hassle:
(local, inherited, not inheritable)
(inherited, not inheritable) # I didn't figure out how to cause this.
(not inheritable)
(local, inherited)
(inherited)
"" (empty string, means local)
A potential solution that comes to mind is presenting the attributes
in a space sparated list after a colon as attached. (Honestly, I'm not
fond of the format and the final term, though.)
"c1_b_not_null" NOT NULL "b": local inherited uninheritable
In 0001, I did wonder about hiding "local" when it's not inherited,
but this behavior rfollows existing code.
In 0002, I'm not completely satisfied with the location, but standard
regression test suite seems more suitable for this check than the TAP
test suite used for testing psql.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Fix-not-null-constraint-representation-in-d.patchtext/x-patch; charset=us-asciiDownload
From 6c8511499d5dbfc769c38b32292d415fa8982707 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Mon, 28 Aug 2023 14:38:58 +0900
Subject: [PATCH 1/2] Fix not-null constraint representation in \d+
The recent commit b0e96f3119 added the description about not-null
constraints in the output of \d+ command. It hided constraints' origin
when it is marked as NO INHERIT. Show their origin irrespective of
the NO INHERIT state.
---
src/bin/psql/describe.c | 22 +++++++---
src/test/regress/expected/constraints.out | 12 ++---
src/test/regress/expected/create_table.out | 6 +--
.../regress/expected/create_table_like.out | 6 +--
src/test/regress/expected/foreign_data.out | 44 +++++++++----------
src/test/regress/expected/generated.out | 2 +-
src/test/regress/expected/inherit.out | 30 ++++++-------
7 files changed, 66 insertions(+), 56 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index bac94a338c..3bf1c0cb97 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3079,16 +3079,26 @@ describeOneTableDetails(const char *schemaname,
/* Might be an empty set - that's ok */
for (i = 0; i < tuples; i++)
{
+ bool noinherit = PQgetvalue(result, i, 2)[0] == 't';
bool islocal = PQgetvalue(result, i, 3)[0] == 't';
bool inherited = PQgetvalue(result, i, 4)[0] == 't';
- printfPQExpBuffer(&buf, " \"%s\" NOT NULL \"%s\"%s",
+ printfPQExpBuffer(&buf, " \"%s\" NOT NULL \"%s\"",
PQgetvalue(result, i, 0),
- PQgetvalue(result, i, 1),
- PQgetvalue(result, i, 2)[0] == 't' ?
- " NO INHERIT" :
- islocal && inherited ? _(" (local, inherited)") :
- inherited ? _(" (inherited)") : "");
+ PQgetvalue(result, i, 1));
+ if (inherited || noinherit)
+ {
+ appendPQExpBufferChar(&buf, ':');
+
+ if (inherited)
+ {
+ if (islocal)
+ appendPQExpBufferStr(&buf, _(" local"));
+ appendPQExpBufferStr(&buf, _(" inherited"));
+ }
+ if (noinherit)
+ appendPQExpBufferStr(&buf, _(" uninheritable"));
+ }
printTableAddFooter(&cont, buf.data);
}
diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out
index b7de50ad6a..d4adbaa518 100644
--- a/src/test/regress/expected/constraints.out
+++ b/src/test/regress/expected/constraints.out
@@ -914,7 +914,7 @@ ALTER TABLE cnn_parent ADD PRIMARY KEY (b);
a | integer | | | | plain | |
b | integer | | not null | | plain | |
Not-null constraints:
- "cnn_grandchild_b_not_null" NOT NULL "b" (local, inherited)
+ "cnn_grandchild_b_not_null" NOT NULL "b": local inherited
Inherits: cnn_child
Child tables: cnn_grandchild2
@@ -925,7 +925,7 @@ Child tables: cnn_grandchild2
a | integer | | | | plain | |
b | integer | | not null | | plain | |
Not-null constraints:
- "cnn_grandchild_b_not_null" NOT NULL "b" (inherited)
+ "cnn_grandchild_b_not_null" NOT NULL "b": inherited
Inherits: cnn_grandchild,
cnn_child2
@@ -951,7 +951,7 @@ ERROR: multiple primary keys for table "cnn_parent" are not allowed
a | integer | | | | plain | |
b | integer | | not null | | plain | |
Not-null constraints:
- "cnn_grandchild_b_not_null" NOT NULL "b" (local, inherited)
+ "cnn_grandchild_b_not_null" NOT NULL "b": local inherited
Inherits: cnn_child
Child tables: cnn_grandchild2
@@ -962,7 +962,7 @@ Child tables: cnn_grandchild2
a | integer | | | | plain | |
b | integer | | not null | | plain | |
Not-null constraints:
- "cnn_grandchild_b_not_null" NOT NULL "b" (inherited)
+ "cnn_grandchild_b_not_null" NOT NULL "b": inherited
Inherits: cnn_grandchild,
cnn_child2
@@ -988,7 +988,7 @@ ALTER TABLE cnn_parent ADD PRIMARY KEY USING INDEX b_uq;
a | integer | | | | plain | |
b | integer | | not null | | plain | |
Not-null constraints:
- "cnn_grandchild_b_not_null" NOT NULL "b" (local, inherited)
+ "cnn_grandchild_b_not_null" NOT NULL "b": local inherited
Inherits: cnn_child
Child tables: cnn_grandchild2
@@ -999,7 +999,7 @@ Child tables: cnn_grandchild2
a | integer | | | | plain | |
b | integer | | not null | | plain | |
Not-null constraints:
- "cnn_grandchild_b_not_null" NOT NULL "b" (inherited)
+ "cnn_grandchild_b_not_null" NOT NULL "b": inherited
Inherits: cnn_grandchild,
cnn_child2
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 344d05233a..293e4292e9 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -855,7 +855,7 @@ drop table test_part_coll_posix;
Partition of: parted FOR VALUES IN ('b')
Partition constraint: ((a IS NOT NULL) AND (a = 'b'::text))
Not-null constraints:
- "part_b_b_not_null" NOT NULL "b" (local, inherited)
+ "part_b_b_not_null" NOT NULL "b": local inherited
-- Both partition bound and partition key in describe output
\d+ part_c
@@ -868,7 +868,7 @@ Partition of: parted FOR VALUES IN ('c')
Partition constraint: ((a IS NOT NULL) AND (a = 'c'::text))
Partition key: RANGE (b)
Not-null constraints:
- "part_c_b_not_null" NOT NULL "b" (local, inherited)
+ "part_c_b_not_null" NOT NULL "b": local inherited
Partitions: part_c_1_10 FOR VALUES FROM (1) TO (10)
-- a level-2 partition's constraint will include the parent's expressions
@@ -881,7 +881,7 @@ Partitions: part_c_1_10 FOR VALUES FROM (1) TO (10)
Partition of: part_c FOR VALUES FROM (1) TO (10)
Partition constraint: ((a IS NOT NULL) AND (a = 'c'::text) AND (b IS NOT NULL) AND (b >= 1) AND (b < 10))
Not-null constraints:
- "part_c_b_not_null" NOT NULL "b" (inherited)
+ "part_c_b_not_null" NOT NULL "b": inherited
-- Show partition count in the parent's describe output
-- Tempted to include \d+ output listing partitions with bound info but
diff --git a/src/test/regress/expected/create_table_like.out b/src/test/regress/expected/create_table_like.out
index 61956773ff..6604d0c7fa 100644
--- a/src/test/regress/expected/create_table_like.out
+++ b/src/test/regress/expected/create_table_like.out
@@ -360,7 +360,7 @@ NOTICE: merging constraint "ctlt1_a_check" with inherited definition
Check constraints:
"ctlt1_a_check" CHECK (length(a) > 2)
Not-null constraints:
- "ctlt1_inh_a_not_null" NOT NULL "a" (local, inherited)
+ "ctlt1_inh_a_not_null" NOT NULL "a": local inherited
Inherits: ctlt1
SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt1_inh'::regclass;
@@ -383,7 +383,7 @@ Check constraints:
"ctlt3_a_check" CHECK (length(a) < 5)
"ctlt3_c_check" CHECK (length(c) < 7)
Not-null constraints:
- "ctlt13_inh_a_not_null" NOT NULL "a" (inherited)
+ "ctlt13_inh_a_not_null" NOT NULL "a": inherited
Inherits: ctlt1,
ctlt3
@@ -403,7 +403,7 @@ Check constraints:
"ctlt3_a_check" CHECK (length(a) < 5)
"ctlt3_c_check" CHECK (length(c) < 7)
Not-null constraints:
- "ctlt13_like_a_not_null" NOT NULL "a" (inherited)
+ "ctlt13_like_a_not_null" NOT NULL "a": inherited
Inherits: ctlt1
SELECT description FROM pg_description, pg_constraint c WHERE classoid = 'pg_constraint'::regclass AND objoid = c.oid AND c.conrelid = 'ctlt13_like'::regclass;
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 1dfe23cc1e..17cdd7e5b3 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -1421,7 +1421,7 @@ Child tables: ft2, FOREIGN
c2 | text | | | | | extended | |
c3 | date | | | | | plain | |
Not-null constraints:
- "fd_pt1_c1_not_null" NOT NULL "c1" (inherited)
+ "fd_pt1_c1_not_null" NOT NULL "c1": inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1474,7 +1474,7 @@ Child tables: ft2, FOREIGN
c2 | text | | | | | extended | |
c3 | date | | | | | plain | |
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1497,7 +1497,7 @@ NOTICE: merging column "c3" with inherited definition
c2 | text | | | | | extended | |
c3 | date | | | | | plain | |
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1512,7 +1512,7 @@ Child tables: ct3,
c2 | text | | | | extended | |
c3 | date | | | | plain | |
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (inherited)
+ "ft2_c1_not_null" NOT NULL "c1": inherited
Inherits: ft2
\d+ ft3
@@ -1523,7 +1523,7 @@ Inherits: ft2
c2 | text | | | | | extended | |
c3 | date | | | | | plain | |
Not-null constraints:
- "ft3_c1_not_null" NOT NULL "c1" (local, inherited)
+ "ft3_c1_not_null" NOT NULL "c1": local inherited
Server: s0
Inherits: ft2
@@ -1563,8 +1563,8 @@ Child tables: ft2, FOREIGN
c7 | integer | | not null | | | plain | |
c8 | integer | | | | | plain | |
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
- "fd_pt1_c7_not_null" NOT NULL "c7" (inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
+ "fd_pt1_c7_not_null" NOT NULL "c7": inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1584,8 +1584,8 @@ Child tables: ct3,
c7 | integer | | not null | | plain | |
c8 | integer | | | | plain | |
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (inherited)
- "fd_pt1_c7_not_null" NOT NULL "c7" (inherited)
+ "ft2_c1_not_null" NOT NULL "c1": inherited
+ "fd_pt1_c7_not_null" NOT NULL "c7": inherited
Inherits: ft2
\d+ ft3
@@ -1601,8 +1601,8 @@ Inherits: ft2
c7 | integer | | not null | | | plain | |
c8 | integer | | | | | plain | |
Not-null constraints:
- "ft3_c1_not_null" NOT NULL "c1" (local, inherited)
- "fd_pt1_c7_not_null" NOT NULL "c7" (inherited)
+ "ft3_c1_not_null" NOT NULL "c1": local inherited
+ "fd_pt1_c7_not_null" NOT NULL "c7": inherited
Server: s0
Inherits: ft2
@@ -1649,8 +1649,8 @@ Child tables: ft2, FOREIGN
c7 | integer | | | | | plain | |
c8 | text | | | | | external | |
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
- "fd_pt1_c6_not_null" NOT NULL "c6" (inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
+ "fd_pt1_c6_not_null" NOT NULL "c6": inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1682,7 +1682,7 @@ Child tables: ft2, FOREIGN
c2 | text | | | | | extended | |
c3 | date | | | | | plain | |
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1729,7 +1729,7 @@ Child tables: ft2, FOREIGN
Check constraints:
"fd_pt1chk2" CHECK (c2 <> ''::text)
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1780,7 +1780,7 @@ Child tables: ft2, FOREIGN
Check constraints:
"fd_pt1chk2" CHECK (c2 <> ''::text)
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1815,7 +1815,7 @@ Check constraints:
"fd_pt1chk2" CHECK (c2 <> ''::text)
"fd_pt1chk3" CHECK (c2 <> ''::text) NOT VALID
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1846,7 +1846,7 @@ Check constraints:
"fd_pt1chk2" CHECK (c2 <> ''::text)
"fd_pt1chk3" CHECK (c2 <> ''::text)
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "c1" (local, inherited)
+ "ft2_c1_not_null" NOT NULL "c1": local inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1881,7 +1881,7 @@ Check constraints:
"f2_check" CHECK (f2 <> ''::text)
"fd_pt1chk2" CHECK (f2 <> ''::text)
Not-null constraints:
- "ft2_c1_not_null" NOT NULL "f1" (local, inherited)
+ "ft2_c1_not_null" NOT NULL "f1": local inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
Inherits: fd_pt1
@@ -1942,7 +1942,7 @@ Partitions: fd_pt2_1 FOR VALUES IN (1), FOREIGN
Partition of: fd_pt2 FOR VALUES IN (1)
Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
Not-null constraints:
- "fd_pt2_c1_not_null" NOT NULL "c1" (inherited)
+ "fd_pt2_c1_not_null" NOT NULL "c1": inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
@@ -2024,7 +2024,7 @@ Partitions: fd_pt2_1 FOR VALUES IN (1), FOREIGN
Partition of: fd_pt2 FOR VALUES IN (1)
Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
Not-null constraints:
- "fd_pt2_1_c1_not_null" NOT NULL "c1" (inherited)
+ "fd_pt2_1_c1_not_null" NOT NULL "c1": inherited
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
@@ -2058,7 +2058,7 @@ Partition constraint: ((c1 IS NOT NULL) AND (c1 = 1))
Check constraints:
"p21chk" CHECK (c2 <> ''::text)
Not-null constraints:
- "fd_pt2_1_c1_not_null" NOT NULL "c1" (inherited)
+ "fd_pt2_1_c1_not_null" NOT NULL "c1": inherited
"fd_pt2_1_c3_not_null" NOT NULL "c3"
Server: s0
FDW options: (delimiter ',', quote '"', "be quoted" 'value')
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index dc97ed3fe0..4a1744f129 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -316,7 +316,7 @@ NOTICE: merging column "b" with inherited definition
b | integer | | | generated always as (a * 22) stored | plain | |
x | integer | | | | plain | |
Not-null constraints:
- "gtestx_a_not_null" NOT NULL "a" (inherited)
+ "gtestx_a_not_null" NOT NULL "a": inherited
Inherits: gtest1
CREATE TABLE gtestxx_1 (a int NOT NULL, b int);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 6daca12340..c80e398f71 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2007,7 +2007,7 @@ Child tables: cc2
f4 | double precision | | | | plain | |
a2 | integer | | not null | | plain | |
Not-null constraints:
- "nn" NOT NULL "a2" (inherited)
+ "nn" NOT NULL "a2": inherited
Inherits: pp1,
cc1
@@ -2031,7 +2031,7 @@ Child tables: cc1,
f3 | integer | | | | plain | |
a2 | integer | | not null | | plain | |
Not-null constraints:
- "pp1_f1_not_null" NOT NULL "f1" (inherited)
+ "pp1_f1_not_null" NOT NULL "f1": inherited
"nn" NOT NULL "a2"
Inherits: pp1
Child tables: cc2
@@ -2046,8 +2046,8 @@ Child tables: cc2
f4 | double precision | | | | plain | |
a2 | integer | | not null | | plain | |
Not-null constraints:
- "pp1_f1_not_null" NOT NULL "f1" (inherited)
- "nn" NOT NULL "a2" (inherited)
+ "pp1_f1_not_null" NOT NULL "f1": inherited
+ "nn" NOT NULL "a2": inherited
Inherits: pp1,
cc1
@@ -2065,7 +2065,7 @@ alter table cc1 alter column a2 drop not null;
f3 | integer | | | | plain | |
a2 | integer | | | | plain | |
Not-null constraints:
- "pp1_f1_not_null" NOT NULL "f1" (inherited)
+ "pp1_f1_not_null" NOT NULL "f1": inherited
Inherits: pp1
Child tables: cc2
@@ -2082,7 +2082,7 @@ ERROR: cannot drop inherited constraint "pp1_f1_not_null" of relation "cc2"
f4 | double precision | | | | plain | |
a2 | integer | | | | plain | |
Not-null constraints:
- "pp1_f1_not_null" NOT NULL "f1" (inherited)
+ "pp1_f1_not_null" NOT NULL "f1": inherited
Inherits: pp1,
cc1
@@ -2112,8 +2112,8 @@ create table inh_child () inherits (inh_parent1, inh_parent2);
a | integer | | not null | | plain | |
b | integer | | not null | | plain | |
Not-null constraints:
- "nn" NOT NULL "a" (inherited)
- "inh_child_b_not_null" NOT NULL "b" (inherited)
+ "nn" NOT NULL "a": inherited
+ "inh_child_b_not_null" NOT NULL "b": inherited
Inherits: inh_parent1,
inh_parent2
@@ -2147,9 +2147,9 @@ select conrelid::regclass, conname, contype, conkey,
d | integer | | not null | | plain | |
e | integer | | | | plain | |
Not-null constraints:
- "inh_child_a_not_null" NOT NULL "a" (inherited)
- "inh_child_b_not_null" NOT NULL "b" (inherited)
- "inh_child_d_not_null" NOT NULL "d" (inherited)
+ "inh_child_a_not_null" NOT NULL "a": inherited
+ "inh_child_b_not_null" NOT NULL "b": inherited
+ "inh_child_d_not_null" NOT NULL "d": inherited
Inherits: inh_parent1,
inh_parent2
@@ -2188,7 +2188,7 @@ Inherits: inh_nn_parent
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | not null | | plain | |
Not-null constraints:
- "inh_nn_parent_a_not_null" NOT NULL "a" NO INHERIT
+ "inh_nn_parent_a_not_null" NOT NULL "a": uninheritable
Child tables: inh_nn_child,
inh_nn_child2
@@ -2223,7 +2223,7 @@ Child tables: inh_child1
--------+---------+-----------+----------+---------+---------+--------------+-------------
f1 | integer | | not null | | plain | |
Not-null constraints:
- "inh_child1_f1_not_null" NOT NULL "f1" (local, inherited)
+ "inh_child1_f1_not_null" NOT NULL "f1": local inherited
Inherits: inh_parent
Child tables: inh_child2
@@ -2233,7 +2233,7 @@ Child tables: inh_child2
--------+---------+-----------+----------+---------+---------+--------------+-------------
f1 | integer | | not null | | plain | |
Not-null constraints:
- "inh_child2_f1_not_null" NOT NULL "f1" (local, inherited)
+ "inh_child2_f1_not_null" NOT NULL "f1": local inherited
Inherits: inh_child1
select conrelid::regclass, conname, contype, coninhcount, conislocal
@@ -2277,7 +2277,7 @@ Child tables: inh_child2,
--------+---------+-----------+----------+---------+---------+--------------+-------------
f1 | integer | | not null | | plain | |
Not-null constraints:
- "inh_child2_f1_not_null" NOT NULL "f1" (local, inherited)
+ "inh_child2_f1_not_null" NOT NULL "f1": local inherited
Inherits: inh_child1
select conrelid::regclass, conname, contype, coninhcount, conislocal
--
2.39.3
0002-Add-tests-for-d-not-null-constraints.patchtext/x-patch; charset=us-asciiDownload
From 606eb7074f18d330833d044d262eeadfe7f6ac12 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Mon, 28 Aug 2023 15:27:53 +0900
Subject: [PATCH 2/2] Add tests for \d+ not-null constraints
Separated for improved readability.
---
src/test/regress/expected/inherit.out | 35 ++++++++++++++++++++++++---
src/test/regress/sql/inherit.sql | 7 +++---
2 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index c80e398f71..99f260b1a5 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2155,10 +2155,14 @@ Inherits: inh_parent1,
drop table inh_parent1, inh_parent2, inh_child;
-- NOT NULL NO INHERIT
-create table inh_nn_parent(a int);
+create table inh_nn_parent(a int, b int);
create table inh_nn_child() inherits (inh_nn_parent);
alter table inh_nn_parent add not null a no inherit;
-create table inh_nn_child2() inherits (inh_nn_parent);
+create table inh_nn_child2(c int not null) inherits (inh_nn_parent);
+create table inh_nn_child3(a int not null, b int not null no inherit, c int not null no inherit) inherits (inh_nn_child2);
+NOTICE: merging column "a" with inherited definition
+NOTICE: merging column "b" with inherited definition
+NOTICE: merging column "c" with inherited definition
select conrelid::regclass, conname, contype, conkey,
(select attname from pg_attribute where attrelid = conrelid and attnum = conkey[1]),
coninhcount, conislocal, connoinherit
@@ -2167,32 +2171,55 @@ select conrelid::regclass, conname, contype, conkey,
order by 2, 1;
conrelid | conname | contype | conkey | attname | coninhcount | conislocal | connoinherit
---------------+--------------------------+---------+--------+---------+-------------+------------+--------------
+ inh_nn_child2 | inh_nn_child2_c_not_null | n | {3} | c | 0 | t | f
+ inh_nn_child3 | inh_nn_child3_a_not_null | n | {1} | a | 0 | t | f
+ inh_nn_child3 | inh_nn_child3_b_not_null | n | {2} | b | 0 | t | t
+ inh_nn_child3 | inh_nn_child3_c_not_null | n | {3} | c | 1 | t | t
inh_nn_parent | inh_nn_parent_a_not_null | n | {1} | a | 0 | t | t
-(1 row)
+(5 rows)
\d+ inh_nn*
Table "public.inh_nn_child"
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain | |
+ b | integer | | | | plain | |
Inherits: inh_nn_parent
Table "public.inh_nn_child2"
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | | | plain | |
+ b | integer | | | | plain | |
+ c | integer | | not null | | plain | |
+Not-null constraints:
+ "inh_nn_child2_c_not_null" NOT NULL "c"
Inherits: inh_nn_parent
+Child tables: inh_nn_child3
+
+ Table "public.inh_nn_child3"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ a | integer | | not null | | plain | |
+ b | integer | | not null | | plain | |
+ c | integer | | not null | | plain | |
+Not-null constraints:
+ "inh_nn_child3_a_not_null" NOT NULL "a"
+ "inh_nn_child3_b_not_null" NOT NULL "b": uninheritable
+ "inh_nn_child3_c_not_null" NOT NULL "c": local inherited uninheritable
+Inherits: inh_nn_child2
Table "public.inh_nn_parent"
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
a | integer | | not null | | plain | |
+ b | integer | | | | plain | |
Not-null constraints:
"inh_nn_parent_a_not_null" NOT NULL "a": uninheritable
Child tables: inh_nn_child,
inh_nn_child2
-drop table inh_nn_parent, inh_nn_child, inh_nn_child2;
+drop table inh_nn_parent, inh_nn_child, inh_nn_child2, inh_nn_child3;
--
-- test inherit/deinherit
--
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index d8fae92a53..82e9118745 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -777,10 +777,11 @@ select conrelid::regclass, conname, contype, conkey,
drop table inh_parent1, inh_parent2, inh_child;
-- NOT NULL NO INHERIT
-create table inh_nn_parent(a int);
+create table inh_nn_parent(a int, b int);
create table inh_nn_child() inherits (inh_nn_parent);
alter table inh_nn_parent add not null a no inherit;
-create table inh_nn_child2() inherits (inh_nn_parent);
+create table inh_nn_child2(c int not null) inherits (inh_nn_parent);
+create table inh_nn_child3(a int not null, b int not null no inherit, c int not null no inherit) inherits (inh_nn_child2);
select conrelid::regclass, conname, contype, conkey,
(select attname from pg_attribute where attrelid = conrelid and attnum = conkey[1]),
coninhcount, conislocal, connoinherit
@@ -788,7 +789,7 @@ select conrelid::regclass, conname, contype, conkey,
conrelid::regclass::text like 'inh\_nn\_%'
order by 2, 1;
\d+ inh_nn*
-drop table inh_nn_parent, inh_nn_child, inh_nn_child2;
+drop table inh_nn_parent, inh_nn_child, inh_nn_child2, inh_nn_child3;
--
-- test inherit/deinherit
--
2.39.3
On 2023-Aug-28, Kyotaro Horiguchi wrote:
But with these tables:
create table p (a int, b int not null default 0);
create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);I get:
Not-null constraints:
"c1_b_not_null" NOT NULL "b" *NO INHERIT*Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
"coninhcount <> 0" align with "local" and "inherited". For a clearer
picuture, those values for c1 are as follows.
Hmm, I think the bug here is that we let you create a constraint in c1
that is NOINHERIT. If the parent already has one INHERIT constraint
in that column, then the child must have that one also; it's not
possible to have both a constraint that inherits and one that doesn't.
I understand that there are only three possibilities for a NOT NULL
constraint in a column:
- There's a NO INHERIT constraint. A NO INHERIT constraint is always
defined locally in that table. In this case, if there is a parent
relation, then it must either not have a NOT NULL constraint in that
column, or it may also have a NO INHERIT one. Therefore, it's
correct to print NO INHERIT and nothing else. We could also print
"(local)" but I see no point in doing that.
- A constraint comes inherited from one or more parent tables and has no
local definition. In this case, the constraint always inherits
(otherwise, the parent wouldn't have given it to this table). So
printing "(inherited)" and nothing else is correct.
- A constraint can have a local definition and also be inherited. In
this case, printing "(local, inherited)" is correct.
Have I missed other cases?
The NO INHERIT bit is part of the syntax, which is why I put it in
uppercase and not marked it for translation. The other two are
informational, so they are translatable.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them." (Freeman Dyson)
At Mon, 28 Aug 2023 13:36:00 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in
On 2023-Aug-28, Kyotaro Horiguchi wrote:
But with these tables:
create table p (a int, b int not null default 0);
create table c1 (a int, b int not null NO INHERIT default 1) inherits (p);I get:
Not-null constraints:
"c1_b_not_null" NOT NULL "b" *NO INHERIT*Here, "NO INHERIT" is mapped from connoinherit, and conislocal and
"coninhcount <> 0" align with "local" and "inherited". For a clearer
picuture, those values for c1 are as follows.Hmm, I think the bug here is that we let you create a constraint in c1
that is NOINHERIT. If the parent already has one INHERIT constraint
in that column, then the child must have that one also; it's not
possible to have both a constraint that inherits and one that doesn't.
Yeah, I had the same question about the coexisting of the two.
I understand that there are only three possibilities for a NOT NULL
constraint in a column:- There's a NO INHERIT constraint. A NO INHERIT constraint is always
defined locally in that table. In this case, if there is a parent
relation, then it must either not have a NOT NULL constraint in that
column, or it may also have a NO INHERIT one. Therefore, it's
correct to print NO INHERIT and nothing else. We could also print
"(local)" but I see no point in doing that.- A constraint comes inherited from one or more parent tables and has no
local definition. In this case, the constraint always inherits
(otherwise, the parent wouldn't have given it to this table). So
printing "(inherited)" and nothing else is correct.- A constraint can have a local definition and also be inherited. In
this case, printing "(local, inherited)" is correct.Have I missed other cases?
Seems correct. I don't see another case given that NO INHERIT is
inhibited when a table has an inherited constraint.
The NO INHERIT bit is part of the syntax, which is why I put it in
uppercase and not marked it for translation. The other two are
informational, so they are translatable.
Given the conditions above, I agree with you.
Attached is the initial version of the patch. It prevents "CREATE
TABLE" from executing if there is an inconsisntent not-null
constraint. Also I noticed that "ALTER TABLE t ADD NOT NULL c NO
INHERIT" silently ignores the "NO INHERIT" part and fixed it.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
inhibit_inconsistent_not_null_no_inherit.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index b534da7d80..39fbb0f151 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2533,7 +2533,7 @@ AddRelationNewConstraints(Relation rel,
* update its catalog status and we're done.
*/
if (AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
- cdef->inhcount))
+ cdef->inhcount, cdef->is_no_inherit))
continue;
/*
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 2a725f6280..bd589c0e7c 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -669,7 +669,8 @@ extractNotNullColumn(HeapTuple constrTup)
* If no not-null constraint is found for the column, return false.
*/
bool
-AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count)
+AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
+ bool is_no_inherit)
{
HeapTuple tup;
@@ -681,6 +682,14 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count)
pg_constraint = table_open(ConstraintRelationId, RowExclusiveLock);
conform = (Form_pg_constraint) GETSTRUCT(tup);
+
+ if (is_no_inherit)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot change NO INHERIT status of inherited NOT NULL constraint \"%s\" on relation \"%s\"",
+ NameStr(conform->conname), get_rel_name(relid)));
+
+
if (count > 0)
conform->coninhcount += count;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 47c900445c..ae5ef6e115 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -350,8 +350,8 @@ static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
static void truncate_check_activity(Relation rel);
static void RangeVarCallbackForTruncate(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
-static List *MergeAttributes(List *schema, List *supers, char relpersistence,
- bool is_partition, List **supconstr,
+static List *MergeAttributes(List *schema, List *supers, List *nnconstr,
+ char relpersistence, bool is_partition, List **supconstr,
List **supnotnulls);
static bool MergeCheckConstraint(List *constraints, char *name, Node *expr);
static void MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel);
@@ -873,7 +873,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* modified by MergeAttributes.)
*/
stmt->tableElts =
- MergeAttributes(stmt->tableElts, inheritOids,
+ MergeAttributes(stmt->tableElts, inheritOids, stmt->nnconstraints,
stmt->relation->relpersistence,
stmt->partbound != NULL,
&old_constraints, &old_notnulls);
@@ -2318,6 +2318,7 @@ storage_name(char c)
* 'schema' is the column/attribute definition for the table. (It's a list
* of ColumnDef's.) It is destructively changed.
* 'supers' is a list of OIDs of parent relations, already locked by caller.
+ * 'child_nnconstr' is a list of not-null constraints on the child relation.
* 'relpersistence' is the persistence type of the table.
* 'is_partition' tells if the table is a partition.
*
@@ -2377,12 +2378,13 @@ storage_name(char c)
*----------
*/
static List *
-MergeAttributes(List *schema, List *supers, char relpersistence,
- bool is_partition, List **supconstr, List **supnotnulls)
+MergeAttributes(List *schema, List *supers, List *child_nnconstr,
+ char relpersistence, bool is_partition, List **supconstr,
+ List **supnotnulls)
{
List *inhSchema = NIL;
List *constraints = NIL;
- List *nnconstraints = NIL;
+ List *super_nnconstr = NIL;
bool have_bogus_defaults = false;
int child_attno;
static Node bogus_marker = {0}; /* marks conflicting defaults */
@@ -2718,7 +2720,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
nn->inhcount = 1;
nn->is_no_inherit = false;
- nnconstraints = lappend(nnconstraints, nn);
+ super_nnconstr = lappend(super_nnconstr, nn);
}
/*
@@ -2807,7 +2809,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
nn->inhcount = 1;
nn->is_no_inherit = false;
- nnconstraints = lappend(nnconstraints, nn);
+ super_nnconstr = lappend(super_nnconstr, nn);
}
}
@@ -2967,7 +2969,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
nn->is_local = false;
nn->inhcount = 1;
- nnconstraints = lappend(nnconstraints, nn);
+ super_nnconstr = lappend(super_nnconstr, nn);
}
free_attrmap(newattmap);
@@ -3096,6 +3098,30 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
errdetail("%s versus %s", def->compression, newdef->compression)));
}
+ /*
+ * Don't allow NO INHERIT when a not-null constraint is
+ * inherited
+ */
+ if (def->is_not_null)
+ {
+ ListCell *lcnnconstr;
+
+ foreach(lcnnconstr, child_nnconstr)
+ {
+ Constraint *con = (Constraint *) lfirst(lcnnconstr);
+ char *child_colname = strVal(linitial(con->keys));
+
+ if (con->is_no_inherit &&
+ strncmp(child_colname, attributeName,
+ NAMEDATALEN) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("cannot define not-null constraint on column \"%s\" with NO INHERIT",
+ attributeName),
+ errdetail("The cloumn has an inherited not-null constraint.")));
+ }
+ }
+
/*
* Merge of not-null constraints = OR 'em together
*/
@@ -3281,7 +3307,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
}
*supconstr = constraints;
- *supnotnulls = nnconstraints;
+ *supnotnulls = super_nnconstr;
return schema;
}
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index f6f5796fe0..f366f8cd14 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -248,7 +248,8 @@ extern char *ChooseConstraintName(const char *name1, const char *name2,
extern HeapTuple findNotNullConstraintAttnum(Oid relid, AttrNumber attnum);
extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
-extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count);
+extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
+ bool is_no_inherit);
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 6daca12340..7ec00aaebc 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2051,6 +2051,15 @@ Not-null constraints:
Inherits: pp1,
cc1
+-- cannot create table with inconsistent NO INHERIT contraint: fails
+create table cc3 (a2 int not null no inherit) inherits (cc1);
+NOTICE: moving and merging column "a2" with inherited definition
+DETAIL: User-specified column moved to the position of the inherited column.
+ERROR: cannot define not-null constraint on column "a2" with NO INHERIT
+DETAIL: The cloumn 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"
-- 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"
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index d8fae92a53..b58a9286f3 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -736,6 +736,12 @@ alter table pp1 alter column f1 set not null;
\d+ cc1
\d+ cc2
+-- cannot create table with inconsistent NO INHERIT contraint: fails
+create table cc3 (a2 int not null no inherit) inherits (cc1);
+
+-- change NO INHERIT status of inherited constraint: no dice, it's inherited
+alter table cc2 add not null a2 no inherit;
+
-- remove constraint from cc2: no dice, it's inherited
alter table cc2 alter column a2 drop not null;
On 2023-Aug-29, Kyotaro Horiguchi wrote:
Attached is the initial version of the patch. It prevents "CREATE
TABLE" from executing if there is an inconsisntent not-null
constraint. Also I noticed that "ALTER TABLE t ADD NOT NULL c NO
INHERIT" silently ignores the "NO INHERIT" part and fixed it.
Great, thank you. I pushed it after modifying it a bit -- instead of
throwing the error in MergeAttributes, I did it in
AddRelationNotNullConstraints(). It seems cleaner this way, mostly
because we already have to match these two constraints there. (I guess
you could argue that we waste catalog-insertion work before the error is
reported and the whole thing is aborted; but I don't think this is a
serious problem in practice.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)
At Tue, 29 Aug 2023 19:28:28 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in
On 2023-Aug-29, Kyotaro Horiguchi wrote:
Attached is the initial version of the patch. It prevents "CREATE
TABLE" from executing if there is an inconsisntent not-null
constraint. Also I noticed that "ALTER TABLE t ADD NOT NULL c NO
INHERIT" silently ignores the "NO INHERIT" part and fixed it.Great, thank you. I pushed it after modifying it a bit -- instead of
throwing the error in MergeAttributes, I did it in
AddRelationNotNullConstraints(). It seems cleaner this way, mostly
because we already have to match these two constraints there. (I guess
I agree that it is cleaner.
you could argue that we waste catalog-insertion work before the error is
reported and the whole thing is aborted; but I don't think this is a
serious problem in practice.)
Given the rarity and the speed required, I agree that early-catching
is not that crucial here. Thanks for clearing that up.
regardes.
--
Kyotaro Horiguchi
NTT Open Source Software Center