subject:"Re\: cataloguing NOT NULL constraints"

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/
>From 6a9feb7800675983198fbb3928c3f34360ac5a49 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
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 
Discussion: https://postgr.es/m/045dec3f-9b3d-aa44-0c99-85f699230...@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

>From e871a4a991762ec5312239aa1a1e0ff918d6ce90 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
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

Re: cataloguing NOT NULL constraints

On 2024-Feb-05, Alvaro Herrera wrote:

> 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 think this change is OK.  In the partition, the primary key is created
in the partition anyway (as expected) which marks the column as
attnotnull[*], and the table is scanned for presence of NULLs if there's
no not-null constraint, and not scanned if there's one.  (The actual
scan is inevitable anyway because we must check the partition
constraint).  This seems the behavior we want.

[*] This attnotnull constraint is lost if you DETACH the partition and
drop the primary key, which is also the behavior we want.

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.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)

Re: cataloguing NOT NULL constraints

On 2024-Feb-05, Alvaro Herrera wrote:

> 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.

Eh, this needs to read "multiple inheritance" rather than "multiple
recursion".  (I'd also need to describe the change for the partitioning
cases in the commit message.)

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Someone said that it is at least an order of magnitude more work to do
production software than a prototype. I think he is wrong by at least
an order of magnitude."  (Brian Kernighan)

Re: cataloguing NOT NULL constraints

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/
>From 486f6bfa8faa6162257c5f12b52180b5a3d89704 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
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 
Discussion: https://postgr.es/m/045dec3f-9b3d-aa44-0c99-85f699230...@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

Re: cataloguing NOT NULL constraints

On 2024-Feb-05, Michael Paquier wrote:

> On Fri, Feb 02, 2024 at 07:00:01PM +0300, Alexander Lakhin wrote:
> > results in:
> > NOTICE:  merging definition of column "i" for child "b"
> > NOTICE:  merging definition of column "i" for child "c"
> > ERROR:  tuple already updated by self
> > 
> > (This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this
> > case.)
> 
> Still I suspect that the fix should be similar, so I'll go put a coin
> on a missing CCI().

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.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php

Re: cataloguing NOT NULL constraints

2024-02-04 Thread Michael Paquier

On Fri, Feb 02, 2024 at 07:00:01PM +0300, Alexander Lakhin wrote:
> results in:
> NOTICE:  merging definition of column "i" for child "b"
> NOTICE:  merging definition of column "i" for child "c"
> ERROR:  tuple already updated by self
> 
> (This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this
> case.)

Still I suspect that the fix should be similar, soI'll go put a coin
on a missing CCI().
--
Michael


signature.asc
Description: PGP signature

Re: cataloguing NOT NULL constraints

2024-02-02 Thread Alexander Lakhin


Hello Alvaro,

Please look at an anomaly introduced with b0e96f311.
The following script:
CREATE TABLE a ();
CREATE TABLE b (i int) INHERITS (a);
CREATE TABLE c () INHERITS (a, b);

ALTER TABLE a ADD COLUMN i int NOT NULL;

results in:
NOTICE:  merging definition of column "i" for child "b"
NOTICE:  merging definition of column "i" for child "c"
ERROR:  tuple already updated by self

(This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this
case.)

Best regards,
Alexander

Re: cataloguing NOT NULL constraints

2024-01-25 Thread Andrew Bille

Hi Alvaro,
25.08.2023 14:38, Alvaro Herrera wrote:
> I have now pushed this again. Hopefully it'll stick this time.

Starting from b0e96f31, pg_upgrade fails with inherited NOT NULL constraint:
For example upgrade from 9c13b6814a (or REL_12_STABLE .. REL_16_STABLE) to
b0e96f31 (or master) with following two tables (excerpt from
src/test/regress/sql/rules.sql)

create table test_0 (id serial primary key);
create table test_1 (id integer primary key) inherits (test_0);

I get the failure:

Setting frozenxid and minmxid counters in new cluster ok
Restoring global objects in the new cluster   ok
Restoring database schemas in the new cluster
 test
*failure*

Consult the last few lines of
"new/pg_upgrade_output.d/20240125T151231.112/log/pg_upgrade_dump_16384.log"
for
the probable cause of the failure.
Failure, exiting

In log:

pg_restore: connecting to database for restore
pg_restore: creating DATABASE "test"
pg_restore: connecting to new database "test"
pg_restore: creating DATABASE PROPERTIES "test"
pg_restore: connecting to new database "test"
pg_restore: creating pg_largeobject "pg_largeobject"
pg_restore: creating COMMENT "SCHEMA "public""
pg_restore: creating TABLE "public.test_0"
pg_restore: creating SEQUENCE "public.test_0_id_seq"
pg_restore: creating SEQUENCE OWNED BY "public.test_0_id_seq"
pg_restore: creating TABLE "public.test_1"
pg_restore: creating DEFAULT "public.test_0 id"
pg_restore: executing SEQUENCE SET test_0_id_seq
pg_restore: creating CONSTRAINT "public.test_0 test_0_pkey"
pg_restore: creating CONSTRAINT "public.test_1 test_1_pkey"
pg_restore: while PROCESSING TOC:
pg_restore: from TOC entry 3200; 2606 16397 CONSTRAINT test_1 test_1_pkey
andrew
pg_restore: error: could not execute query: ERROR:  cannot drop inherited
constraint "pgdump_throwaway_notnull_0" of relation "test_1"
Command was:
-- For binary upgrade, must preserve pg_class oids and relfilenodes
SELECT
pg_catalog.binary_upgrade_set_next_index_pg_class_oid('16396'::pg_catalog.oid);

SELECT
pg_catalog.binary_upgrade_set_next_index_relfilenode('16396'::pg_catalog.oid);


ALTER TABLE ONLY "public"."test_1"
   ADD CONSTRAINT "test_1_pkey" PRIMARY KEY ("id");

ALTER TABLE ONLY "public"."test_1" DROP CONSTRAINT
pgdump_throwaway_notnull_0;

Thanks!




On Thu, Jan 25, 2024 at 3:06 PM Alvaro Herrera 
wrote:

> I have now pushed this again.  Hopefully it'll stick this time.
>
> We may want to make some further tweaks to the behavior in some cases --
> for example, don't disallow ALTER TABLE DROP NOT NULL when the
> constraint is both inherited and has a local definition; the other
> option is to mark the constraint as no longer having a local definition.
> I left it the other way because that's what CHECK does; maybe we would
> like to change both at once.
>
> I ran it through CI, and the pg_upgrade test with a dump from 14's
> regression test database and everything worked well, but it's been a
> while since I tested the sepgsql part of it, so that might the first
> thing to explode.
>
> --
> Álvaro Herrera   48°01'N 7°57'E  —
> https://www.EnterpriseDB.com/
>
>
>
>
>

Re: cataloguing NOT NULL constraints

2023-11-08 Thread Alvaro Herrera

On 2023-Oct-12, Alexander Lakhin wrote:

Hello,

> I've discovered that that commit added several recursive functions, and
> some of them are not protected from stack overflow.

True.  I reproduced the first two, but didn't attempt to reproduce the
third one -- patching all these to check for stack depth is cheap
protection.  I also patched ATAddCheckNNConstraint:

> (ATAddCheckNNConstraint() is protected because it calls
> AddRelationNewConstraints(), which in turn calls StoreRelCheck() ->
> CreateConstraintEntry() ->  recordDependencyOnSingleRelExpr() ->
> find_expr_references_walker() ->  expression_tree_walker() ->
> expression_tree_walker() -> check_stack_depth().)

because it seems uselessly risky to rely on depth checks that exist on
completely unrelated pieces of code, when the function visibly recurses
on itself.  Especially so since the test cases that demonstrate crashes
are so expensive to run, which means we're not going to detect it if at
some point that other stack depth check stops being called for whatever
reason.

BTW probably the tests could be made much cheaper by running the server
with a lower "ulimit -s" setting.  I didn't try.

I noticed one more crash while trying to "drop table" one of the
hierarchies your scripts create.  But it's a preexisting issue which
needs a backpatched fix, and I think Egor already reported it in the
other thread.

Thank you

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees."  (E. Dijkstra)

Re: cataloguing NOT NULL constraints

2023-10-12 Thread Alexander Lakhin


Hi Alvaro,

25.08.2023 14:38, Alvaro Herrera wrote:

I have now pushed this again.  Hopefully it'll stick this time.


I've discovered that that commit added several recursive functions, and
some of them are not protected from stack overflow.

Namely, with "max_locks_per_transaction = 600" and default ulimit -s (8192),
I observe server crashes with the following scripts:
# ATExecSetNotNull()
(n=4; printf "create table t0 (a int, b int);";
for ((i=1;i<=$n;i++)); do printf "create table t$i() inherits(t$(( $i - 1 ))); 
"; done;
printf "alter table t0 alter b set not null;" ) | psql >psql.log

# dropconstraint_internal()
(n=2; printf "create table t0 (a int, b int not null);";
for ((i=1;i<=$n;i++)); do printf "create table t$i() inherits(t$(( $i - 1 ))); 
"; done;
printf "alter table t0 alter b drop not null;" ) | psql >psql.log

# set_attnotnull()
(n=11; printf "create table tp (a int, b int, primary key(a, b)) partition by range (a); create table tp0 (a int 
primary key, b int) partition by range (a);";
for ((i=1;i<=$n;i++)); do printf "create table tp$i partition of tp$(( $i - 1 )) for values from ($i) to (100) 
partition by range (a);"; done;
printf "alter table tp attach partition tp0 for values from (0) to (100);") | psql >psql.log # this takes half an 
hour on my machine


May be you would find appropriate to add check_stack_depth() to these
functions.

(ATAddCheckNNConstraint() is protected because it calls
AddRelationNewConstraints(), which in turn calls StoreRelCheck() ->
CreateConstraintEntry() ->  recordDependencyOnSingleRelExpr() ->
find_expr_references_walker() ->  expression_tree_walker() ->
expression_tree_walker() -> check_stack_depth().)

(There were patches prepared for similar cases [1], but they don't cover new
functions, of course, and I'm not sure how to handle all such instances.)

[1] https://commitfest.postgresql.org/45/4239/

Best regards,
Alexander

Re: cataloguing NOT NULL constraints

2023-09-05 Thread Alvaro Herrera

On 2023-Sep-05, Peter Eisentraut wrote:

> The following information schema views are affected by the not-null
> constraint catalog entries:
> 
> 1. CHECK_CONSTRAINTS
> 2. CONSTRAINT_COLUMN_USAGE
> 3. DOMAIN_CONSTRAINTS
> 4. TABLE_CONSTRAINTS
> 
> Note that 1 and 3 also contain domain constraints.  So as long as the domain
> not-null constraints are not similarly catalogued, we can't delete the
> separate not-null union branch.  (3 never had one, so arguably a bit buggy.)
> 
> I think we can fix up 4 by just deleting the not-null union branch.
> 
> For 2, the simple fix is also easy, but there are some other options, as you
> discuss above.
> 
> How do you want to proceed?

I posted as a patch in a separate thread[1].  Let me fix up the
definitions for views 1 and 3 for domains per your comments, and I'll
post in that thread again.

[1] https://postgr.es/m/202309041710.psytrxlsiqex@alvherre.pgsql

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)

Re: cataloguing NOT NULL constraints

2023-09-05 Thread Peter Eisentraut


On 31.08.23 12:02, Alvaro Herrera wrote:

In constraint_column_usage, you're adding a relkind to the catalog scan
that goes through pg_depend for CHECK constraints.  Here we can rely on
a simple conkey[1] check and a separate UNION ALL arm[q5]; this is also
faster when there are many tables.

The third view definition looks ok.  It's certainly very nice to be able
to delete XXX comments there.


The following information schema views are affected by the not-null 
constraint catalog entries:


1. CHECK_CONSTRAINTS
2. CONSTRAINT_COLUMN_USAGE
3. DOMAIN_CONSTRAINTS
4. TABLE_CONSTRAINTS

Note that 1 and 3 also contain domain constraints.  So as long as the 
domain not-null constraints are not similarly catalogued, we can't 
delete the separate not-null union branch.  (3 never had one, so 
arguably a bit buggy.)


I think we can fix up 4 by just deleting the not-null union branch.

For 2, the simple fix is also easy, but there are some other options, as 
you discuss above.


How do you want to proceed?

Re: cataloguing NOT NULL constraints

2023-09-04 Thread Alvaro Herrera

Looking at your 0001 patch, which adds tests for some of the
information_schema views, I think it's a bad idea to put them in
whatever other regression .sql files; they would be subject to
concurrent changes depending on what other tests are being executed in
the same parallel test.  I suggest to put them all in a separate .sql
file, and schedule that to run in the last concurrent group, together
with the tablespace test.  This way, it would capture all the objects
left over by other test files.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: cataloguing NOT NULL constraints

2023-09-01 Thread Alvaro Herrera

On 2023-Aug-31, Alvaro Herrera wrote:

> Hmm, that's some weird code I left there all right.  Can you please try
> this patch?  (Not final; I'll review it more completely later,
> particularly to add this test case.)

The change in MergeAttributesIntoExisting turned out to be close but not
quite there, so I pushed another version of the fix.

In case you're wondering, the change in MergeConstraintsIntoExisting is
a related but different case, for which I added the other test case you
see there.

I also noticed, while looking at this, that there's another problem when
a child has a NO INHERIT not-null constraint and the parent has a
primary key in the same column.  It should refuse, or take over by
marking it no longer NO INHERIT.  But it just accepts silently and all
appears to be good.  The problems appear when you add a child to that
child.  I'll look into this later; it's not exactly the same code.  At
least it's not a crasher.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/

Re: cataloguing NOT NULL constraints

2023-08-31 Thread Alexander Lakhin


31.08.2023 13:26, Alvaro Herrera wrote:

Hmm, that's some weird code I left there all right.  Can you please try
this patch?  (Not final; I'll review it more completely later,
particularly to add this test case.)


Yes, your patch fixes the issue. I get the same error now:
ERROR:  column "a" in child table must be marked NOT NULL

Thank you!

Best regards,
Alexander

Re: cataloguing NOT NULL constraints

2023-08-31 Thread Alvaro Herrera

Hello Alexander,

Thanks for testing.

On 2023-Aug-31, Alexander Lakhin wrote:

> 25.08.2023 14:38, Alvaro Herrera wrote:
> > I have now pushed this again.  Hopefully it'll stick this time.
> 
> I've found that after that commit the following query:
> CREATE TABLE t(a int PRIMARY KEY) PARTITION BY RANGE (a);
> CREATE TABLE tp1(a int);
> ALTER TABLE t ATTACH PARTITION tp1 FOR VALUES FROM (0) to (1);
> 
> triggers a server crash:

Hmm, that's some weird code I left there all right.  Can you please try
this patch?  (Not final; I'll review it more completely later,
particularly to add this test case.)

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
 It does it in a really, really complicated way
 why does it need to be complicated?
 Because it's MakeMaker.
>From ab241913dec84265ca64d3cb76d1509bb7ce1808 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Thu, 31 Aug 2023 12:24:18 +0200
Subject: [PATCH] Fix not-null constraint test

Per report from Alexander Lakhin
---
 src/backend/commands/tablecmds.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d097da3c78..5941d0a4be 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15750,7 +15750,8 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel)
 
 contup = findNotNullConstraintAttnum(RelationGetRelid(parent_rel),
 	 attribute->attnum);
-if (!((Form_pg_constraint) GETSTRUCT(contup))->connoinherit)
+
+if (!HeapTupleIsValid(contup))
 	ereport(ERROR,
 			(errcode(ERRCODE_DATATYPE_MISMATCH),
 			 errmsg("column \"%s\" in child table must be marked NOT NULL",
@@ -15975,10 +15976,20 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
 		systable_endscan(child_scan);
 
 		if (!found)
+		{
+			if (parent_con->contype == CONSTRAINT_NOTNULL)
+ereport(ERROR,
+		errcode(ERRCODE_DATATYPE_MISMATCH),
+		errmsg("column \"%s\" in child table must be marked NOT NULL",
+			   get_attname(parent_relid,
+		   extractNotNullColumn(parent_tuple),
+		   false)));
+
 			ereport(ERROR,
 	(errcode(ERRCODE_DATATYPE_MISMATCH),
 	 errmsg("child table is missing constraint \"%s\"",
 			NameStr(parent_con->conname;
+		}
 	}
 
 	systable_endscan(parent_scan);
-- 
2.39.2

Re: cataloguing NOT NULL constraints

2023-08-31 Thread Alvaro Herrera

On 2023-Mar-29, Peter Eisentraut wrote:

> On 27.03.23 15:55, Peter Eisentraut wrote:
> > The information schema should be updated.  I think the following views:
> > 
> > - CHECK_CONSTRAINTS
> > - CONSTRAINT_COLUMN_USAGE
> > - DOMAIN_CONSTRAINTS
> > - TABLE_CONSTRAINTS
> > 
> > It looks like these have no test coverage; maybe that could be addressed
> > at the same time.
> 
> Here are patches for this.  I haven't included the expected files for the
> tests; this should be checked again that output is correct or the changes
> introduced by this patch set are as expected.
> 
> The reason we didn't have tests for this before was probably in part because
> the information schema made up names for not-null constraints involving
> OIDs, so the test wouldn't have been stable.
> 
> Feel free to integrate this, or we can add it on afterwards.

I'm eyeing patch 0002 here.  I noticed that in view check_constraints it
defines the not-null constraint definition as substrings over the
pg_get_constraintdef() function[q1], so I wondered whether it might be
better to join to pg_attribute instead.  I see two options:

1. add a scalar subselect in the select list for each constraint [q2]
2. add a LEFT JOIN to pg_attribute to the main FROM list [q3]
   ON con.conrelid=att.attrelid AND con.conkey[1] = con.attrelid

With just the regression test tables in place, these forms are all
pretty much the same in execution time.  I then created 20k tables with
6 columns each and NOT NULL constraint on each column[4].  That's not a
huge amount but it's credible for a medium-size database with a bunch of
partitions (it's amazing what passes for a medium-size database these
days).  I was surprised to find out that q3 (~130ms) is three times
faster than q2 (~390ms), which is in turn more than twice faster than
your proposed q1 (~870ms).  So unless you have another reason to prefer
it, I think we should use q3 here.

In constraint_column_usage, you're adding a relkind to the catalog scan
that goes through pg_depend for CHECK constraints.  Here we can rely on
a simple conkey[1] check and a separate UNION ALL arm[q5]; this is also
faster when there are many tables.

The third view definition looks ok.  It's certainly very nice to be able
to delete XXX comments there.

[q1]
SELECT current_database()::information_schema.sql_identifier AS 
constraint_catalog,
rs.nspname::information_schema.sql_identifier AS constraint_schema,
con.conname::information_schema.sql_identifier AS constraint_name,
CASE con.contype
WHEN 'c'::"char" THEN 
"left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer)
WHEN 'n'::"char" THEN SUBSTRING(pg_get_constraintdef(con.oid) FROM 
10) || ' IS NOT NULL'::text 
ELSE NULL::text
END::information_schema.character_data AS check_clause
   FROM pg_constraint con
 LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
 LEFT JOIN pg_class c ON c.oid = con.conrelid
 LEFT JOIN pg_type t ON t.oid = con.contypid

  WHERE pg_has_role(COALESCE(c.relowner, t.typowner), 'USAGE'::text) AND 
(con.contype = ANY (ARRAY['c'::"char", 'n'::"char"]));

[q2]
SELECT current_database()::information_schema.sql_identifier AS 
constraint_catalog,
rs.nspname::information_schema.sql_identifier AS constraint_schema,
con.conname::information_schema.sql_identifier AS constraint_name,
CASE con.contype
WHEN 'c'::"char" THEN 
"left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer)
WHEN 'n'::"char" THEN FORMAT('CHECK (%s IS NOT NULL)',
 (SELECT attname FROM pg_attribute 
WHERE attrelid = conrelid AND attnum = conkey[1]))
ELSE NULL::text
END::information_schema.character_data AS check_clause
   FROM pg_constraint con
 LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
 LEFT JOIN pg_class c ON c.oid = con.conrelid
 LEFT JOIN pg_type t ON t.oid = con.contypid
  WHERE pg_has_role(COALESCE(c.relowner, t.typowner), 'USAGE'::text) AND 
(con.contype = ANY (ARRAY['c'::"char", 'n'::"char"]));

[q3]
SELECT current_database()::information_schema.sql_identifier AS 
constraint_catalog,
rs.nspname::information_schema.sql_identifier AS constraint_schema,
con.conname::information_schema.sql_identifier AS constraint_name,
CASE con.contype
WHEN 'c'::"char" THEN 
"left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer)
WHEN 'n'::"char" THEN FORMAT('CHECK (%s IS NOT NULL)', at.attname)  

ELSE NULL::text
END::information_schema.character_data AS check_clause
   FROM pg_constraint con
 LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
 LEFT JOIN pg_class c ON c.oid = con.conrelid

Re: cataloguing NOT NULL constraints

2023-08-31 Thread Alexander Lakhin


Hi Alvaro,

25.08.2023 14:38, Alvaro Herrera wrote:

I have now pushed this again.  Hopefully it'll stick this time.


I've found that after that commit the following query:
CREATE TABLE t(a int PRIMARY KEY) PARTITION BY RANGE (a);
CREATE TABLE tp1(a int);
ALTER TABLE t ATTACH PARTITION tp1 FOR VALUES FROM (0) to (1);

triggers a server crash:
Core was generated by `postgres: law regression [local] ALTER TABLE 
 '.
Program terminated with signal SIGSEGV, Segmentation fault.

warning: Section `.reg-xstate/2194811' in core file too small.
#0  0x556007711d77 in MergeAttributesIntoExisting (child_rel=0x7fc30ba309d8,
    parent_rel=0x7fc30ba33f18) at tablecmds.c:15771
15771   if (!((Form_pg_constraint) 
GETSTRUCT(contup))->connoinherit)
(gdb) bt
#0  0x556007711d77 in MergeAttributesIntoExisting (child_rel=0x7fc30ba309d8,
    parent_rel=0x7fc30ba33f18) at tablecmds.c:15771
#1  0x5560077118d4 in CreateInheritance (child_rel=0x7fc30ba309d8, 
parent_rel=0x7fc30ba33f18)
    at tablecmds.c:15631
...

(gdb) print contup
$1 = (HeapTuple) 0x0

On b0e96f311~1 I get:
ERROR:  column "a" in child table must be marked NOT NULL

Best regards,
Alexander

Re: cataloguing NOT NULL constraints

2023-08-28 Thread Alvaro Herrera

On 2023-Aug-28, Peter Eisentraut wrote:

> It looks like we forgot about domain constraints?  For example,
> 
> create domain testdomain as int not null;
> 
> should create a row in pg_constraint?

Well, at some point I purposefully left them out; they were sufficiently
different from the ones in tables that doing both things at the same
time was not saving any effort.  I guess we could try to bake them in
now.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem."  (Tom Lane)

Re: cataloguing NOT NULL constraints

2023-08-28 Thread Peter Eisentraut


On 25.08.23 13:38, Alvaro Herrera wrote:

I have now pushed this again.  Hopefully it'll stick this time.

We may want to make some further tweaks to the behavior in some cases --
for example, don't disallow ALTER TABLE DROP NOT NULL when the
constraint is both inherited and has a local definition; the other
option is to mark the constraint as no longer having a local definition.
I left it the other way because that's what CHECK does; maybe we would
like to change both at once.

I ran it through CI, and the pg_upgrade test with a dump from 14's
regression test database and everything worked well, but it's been a
while since I tested the sepgsql part of it, so that might the first
thing to explode.


It looks like we forgot about domain constraints?  For example,

create domain testdomain as int not null;

should create a row in pg_constraint?

Re: cataloguing NOT NULL constraints

2023-08-25 Thread Alvaro Herrera

On 2023-Aug-25, Alvaro Herrera wrote:

> I have now pushed this again.  Hopefully it'll stick this time.

Hmm, failed under the Czech locale[1]; apparently "inh_grandchld" sorts
earlier than "inh_child1" there.  I think I'll rename inh_grandchld to
inh_child3 or something like that.

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hippopotamus=2023-08-25%2011%3A33%3A07

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: cataloguing NOT NULL constraints

2023-08-25 Thread Alvaro Herrera

I have now pushed this again.  Hopefully it'll stick this time.

We may want to make some further tweaks to the behavior in some cases --
for example, don't disallow ALTER TABLE DROP NOT NULL when the
constraint is both inherited and has a local definition; the other
option is to mark the constraint as no longer having a local definition.
I left it the other way because that's what CHECK does; maybe we would
like to change both at once.

I ran it through CI, and the pg_upgrade test with a dump from 14's
regression test database and everything worked well, but it's been a
while since I tested the sepgsql part of it, so that might the first
thing to explode.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: cataloguing NOT NULL constraints

2023-08-16 Thread Peter Eisentraut


I have two small patches that you can integrate into your patch set:

The first just changes the punctuation of "Not-null constraints" in the 
psql output to match what the documentation mostly uses.


The second has some changes to ddl.sgml to reflect that not-null 
constraints are now named and can be operated on like other constraints. 
 You might want to read that again to make sure it matches your latest 
intentions, but I think it catches all the places that are required to 
change.From 324f0050eee51c47e4c558867e6cc832652b39bb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 16 Aug 2023 10:26:09 +0200
Subject: [PATCH 1/2] fixup! Have psql print the NOT NULL constraints on \d+

---
 contrib/test_decoding/expected/ddl.out|  8 +-
 src/bin/psql/describe.c   |  2 +-
 src/test/regress/expected/create_table.out|  6 +-
 .../regress/expected/create_table_like.out| 10 +--
 src/test/regress/expected/foreign_data.out| 84 +--
 src/test/regress/expected/generated.out   |  2 +-
 src/test/regress/expected/identity.out|  2 +-
 src/test/regress/expected/publication.out |  6 +-
 .../regress/expected/replica_identity.out |  6 +-
 src/test/regress/expected/rowsecurity.out |  2 +-
 10 files changed, 64 insertions(+), 64 deletions(-)

diff --git a/contrib/test_decoding/expected/ddl.out 
b/contrib/test_decoding/expected/ddl.out
index 95a0722c33..bcd1f74b2b 100644
--- a/contrib/test_decoding/expected/ddl.out
+++ b/contrib/test_decoding/expected/ddl.out
@@ -492,7 +492,7 @@ WITH (user_catalog_table = true)
  options  | text[]  |   |  |   
   | extended |  | 
 Indexes:
 "replication_metadata_pkey" PRIMARY KEY, btree (id)
-Not null constraints:
+Not-null constraints:
 "replication_metadata_id_not_null" NOT NULL "id"
 "replication_metadata_relation_not_null" NOT NULL "relation"
 Options: user_catalog_table=true
@@ -509,7 +509,7 @@ ALTER TABLE replication_metadata RESET (user_catalog_table);
  options  | text[]  |   |  |   
   | extended |  | 
 Indexes:
 "replication_metadata_pkey" PRIMARY KEY, btree (id)
-Not null constraints:
+Not-null constraints:
 "replication_metadata_id_not_null" NOT NULL "id"
 "replication_metadata_relation_not_null" NOT NULL "relation"
 
@@ -525,7 +525,7 @@ ALTER TABLE replication_metadata SET (user_catalog_table = 
true);
  options  | text[]  |   |  |   
   | extended |  | 
 Indexes:
 "replication_metadata_pkey" PRIMARY KEY, btree (id)
-Not null constraints:
+Not-null constraints:
 "replication_metadata_id_not_null" NOT NULL "id"
 "replication_metadata_relation_not_null" NOT NULL "relation"
 Options: user_catalog_table=true
@@ -547,7 +547,7 @@ ALTER TABLE replication_metadata SET (user_catalog_table = 
false);
  rewritemeornot | integer |   |  | 
 | plain|  | 
 Indexes:
 "replication_metadata_pkey" PRIMARY KEY, btree (id)
-Not null constraints:
+Not-null constraints:
 "replication_metadata_id_not_null" NOT NULL "id"
 "replication_metadata_relation_not_null" NOT NULL "relation"
 Options: user_catalog_table=false
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index d1dc8fa066..4d36e0cfd8 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3072,7 +3072,7 @@ describeOneTableDetails(const char *schemaname,
tuples = PQntuples(result);
 
if (tuples > 0)
-   printTableAddFooter(, _("Not null 
constraints:"));
+   printTableAddFooter(, _("Not-null 
constraints:"));
 
/* Might be an empty set - that's ok */
for (i = 0; i < tuples; i++)
diff --git a/src/test/regress/expected/create_table.out 
b/src/test/regress/expected/create_table.out
index 3f6516c3f8..477e8839e9 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -854,7 +854,7 @@ drop table test_part_coll_posix;
  b  | integer |   | not null | 1   | plain|  | 
 Partition of: parted FOR VALUES IN ('b')
 Partition constraint: ((a IS NOT NULL) AND (a = 'b'::text))
-Not null constraints:
+Not-null constraints:
 "part_b_b_not_null" NOT NULL "b"
 
 -- Both partition bound and partition key in describe output
@@ -867,7 +867,7 @@ Not null constraints:
 Partition of: parted FOR VALUES IN ('c')
 Partition constraint: ((a IS NOT NULL) AND (a = 'c'::text))
 Partition key: RANGE (b)
-Not null constraints:
+Not-null constraints:
 "part_c_b_not_null" NOT NULL "b"
 Partitions: part_c_1_10 FOR VALUES FROM (1) TO (10)
 
@@ -880,7 +880,7 @@ Partitions: part_c_1_10 FOR

Re: cataloguing NOT NULL constraints

2023-08-16 Thread Peter Eisentraut


On 15.08.23 11:57, Dean Rasheed wrote:

Something else I noticed when reading the SQL standard is that a
user-defined CHECK (col IS NOT NULL) constraint should be recognised
by the system as also making the column not null (setting its
"nullability characteristic" to "known not nullable"). I think that's
more than just an artefact of how they say NOT NULL constraints should
be implemented, because the effect of such a CHECK constraint should
be exposed in the "columns" view of the information schema -- the
value of "is_nullable" should be "NO" if the column is "known not
nullable".


Nullability determination is different from not-null constraints.  The 
nullability characteristic of a column can be derived from multiple 
sources, including not-null constraints, check constraints, primary key 
constraints, domain constraints, as well as more complex rules in case 
of views, joins, etc.  But this is all distinct and separate from the 
issue of not-null constraints that we are discussing here.

Re: cataloguing NOT NULL constraints

2023-08-15 Thread Alvaro Herrera

On 2023-Aug-15, Dean Rasheed wrote:

> I think perhaps for ALTER TABLE INHERIT, it should check that the
> child has a NOT NULL constraint, and error out if not. That's the
> current behaviour, and also matches other constraints types (e.g.,
> CHECK constraints).

Yeah, I reached the same conclusion yesterday while trying it out, so
that's what I implemented.  I'll post later today.

> More generally though, I'm worried that this is starting to get very
> complicated. I wonder if there might be a different, simpler approach.
> One vague idea is to have a new attribute on the column that counts
> the number of constraints (local and inherited PK and NOT NULL
> constraints) that make the column not null.

Hmm.  I grant that this is different, but I don't see that it is
simpler.

> Something else I noticed when reading the SQL standard is that a
> user-defined CHECK (col IS NOT NULL) constraint should be recognised
> by the system as also making the column not null (setting its
> "nullability characteristic" to "known not nullable").

I agree with this view actually, but I've refrained from implementing
it(*) because our SQL-standards people have advised against it.  Insider
knowledge?  I don't know.  I think this is a comparatively smaller
consideration though, and we can adjust for it afterwards.

(*) Rather: at some point I removed the implementation of that from the
patch.

> I'm also wondering whether creating a pg_constraint entry for *every*
> not-nullable column is actually going too far. If we were to
> distinguish between "defined as NOT NULL" and being not null as a
> result of one or more constraints, in the way that the standard seems
> to suggest, perhaps the former (likely to be much more common) could
> simply be a new attribute stored on the column. I think we actually
> only need to create pg_constraint entries if a constraint name or any
> additional constraint properties such as NOT VALID are specified. That
> would lead to far fewer new constraints, less catalog bloat, and less
> noise in the \d output.

There is a problem if we do this, though, which is that we cannot use
the constraints for the things that we want them for -- for example,
remove_useless_groupby_columns() would like to use unique constraints,
not just primary keys; but it depends on the NOT NULL rows being there
for invalidation reasons (namely: if the NOT NULL constraint is dropped,
we need to be able to replan.  Without catalog rows, we don't have a
mechanism to let that happen).

If we don't add all those redundant catalog rows, then this is all for
naught.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.  (Don Knuth)

Re: cataloguing NOT NULL constraints

2023-08-15 Thread Dean Rasheed

On Fri, 11 Aug 2023 at 14:54, Alvaro Herrera  wrote:
>
> Right, in the end I got around to that point of view.  I abandoned the
> idea of adding these dependency links, and I'm back at relying on the
> coninhcount/conislocal markers.  But there were a couple of bugs in the
> accounting for that, so I've fixed some of those, but it's not yet
> complete:
>
> - ALTER TABLE parent ADD PRIMARY KEY
>   needs to create NOT NULL constraints in children.  I added this, but
>   I'm not yet sure it works correctly (for example, if a child already
>   has a NOT NULL constraint, we need to bump its inhcount, but we
>   don't.)
> - ALTER TABLE parent ADD PRIMARY KEY USING index
>   Not sure if this is just as above or needs separate handling
> - ALTER TABLE DROP PRIMARY KEY
>   needs to decrement inhcount or drop the constraint if there are no
>   other sources for that constraint to exist.  I've adjusted the drop
>   constraint code to do this.
> - ALTER TABLE INHERIT
>   needs to create a constraint on the new child, if parent has PK. Not
>   implemented
> - ALTER TABLE NO INHERIT
>   needs to delink any constraints (decrement inhcount, possibly drop
>   the constraint).
>

I think perhaps for ALTER TABLE INHERIT, it should check that the
child has a NOT NULL constraint, and error out if not. That's the
current behaviour, and also matches other constraints types (e.g.,
CHECK constraints).

More generally though, I'm worried that this is starting to get very
complicated. I wonder if there might be a different, simpler approach.
One vague idea is to have a new attribute on the column that counts
the number of constraints (local and inherited PK and NOT NULL
constraints) that make the column not null.

Something else I noticed when reading the SQL standard is that a
user-defined CHECK (col IS NOT NULL) constraint should be recognised
by the system as also making the column not null (setting its
"nullability characteristic" to "known not nullable"). I think that's
more than just an artefact of how they say NOT NULL constraints should
be implemented, because the effect of such a CHECK constraint should
be exposed in the "columns" view of the information schema -- the
value of "is_nullable" should be "NO" if the column is "known not
nullable".

In this sense, the standard does allow multiple not null constraints
on a column, independently of whether the column is "defined as NOT
NULL". My understanding of the standard is that ALTER COLUMN ...
SET/DROP NOT NULL change whether or not the column is "defined as NOT
NULL", and manage a single system-generated constraint, but there may
be any number of other user-defined constraints that also make the
column "known not nullable", and they need to be tracked in some way.

I'm also wondering whether creating a pg_constraint entry for *every*
not-nullable column is actually going too far. If we were to
distinguish between "defined as NOT NULL" and being not null as a
result of one or more constraints, in the way that the standard seems
to suggest, perhaps the former (likely to be much more common) could
simply be a new attribute stored on the column. I think we actually
only need to create pg_constraint entries if a constraint name or any
additional constraint properties such as NOT VALID are specified. That
would lead to far fewer new constraints, less catalog bloat, and less
noise in the \d output.

Regards,
Dean

Re: cataloguing NOT NULL constraints

2023-08-09 Thread Alvaro Herrera

On 2023-Aug-09, Peter Eisentraut wrote:

> I wonder whether the root of these problems is that we mix together primary
> key constraints and not-null constraints.  I understand that right now, with
> the proposed patch, when a table inherits from a parent table with a primary
> key constraint, we generate not-null constraints on the child, in order to
> enforce the not-nullness.  What if we did something like this instead: In
> the child table, we don't generate a not-null constraint, but instead a
> primary key constraint entry.  But we mark the primary key constraint
> somehow to say, this is just for the purpose of inheritance, don't enforce
> uniqueness, but enforce not-nullness.  Would that work?

Hmm.  One table can have many parents, and many of them can have primary
keys.  If we tried to model it the way you suggest, the child table
would need to have several primary keys.  I don't think this would work.

But I think I just need to stare at the dependency graph a little while
longer.  Maybe I just need to add some extra edges to make it work
correctly.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: cataloguing NOT NULL constraints

2023-08-09 Thread Peter Eisentraut


On 05.08.23 21:50, Dean Rasheed wrote:

Anyway, I was at the same time fixing the other problem you reported
with inheritance (namely, adding a PK ends up with the child column
being marked NOT NULL but no corresponding constraint).

At some point I wondered if the easy way out wouldn't be to give up on
the idea that creating a PK causes the child columns to be marked
not-nullable.  However, IIRC I decided against that because it breaks
restoring of old dumps, so it wouldn't be acceptable.

To make matters worse: pg_dump creates the PK as

   ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )

note the ONLY there.  It seems I'm forced to cause the PK to affect
children even though ONLY is given.  This is undesirable but I don't see
a way out of that.

It is all a bit of a rat's nest.



I wonder if that could be made to work in the same way as inherited
CHECK constraints -- dump the child's inherited NOT NULL constraints,
and then manually update conislocal in pg_constraint.


I wonder whether the root of these problems is that we mix together 
primary key constraints and not-null constraints.  I understand that 
right now, with the proposed patch, when a table inherits from a parent 
table with a primary key constraint, we generate not-null constraints on 
the child, in order to enforce the not-nullness.  What if we did 
something like this instead: In the child table, we don't generate a 
not-null constraint, but instead a primary key constraint entry.  But we 
mark the primary key constraint somehow to say, this is just for the 
purpose of inheritance, don't enforce uniqueness, but enforce 
not-nullness.  Would that work?

Re: cataloguing NOT NULL constraints

2023-08-05 Thread Dean Rasheed

On Sat, 5 Aug 2023 at 18:37, Alvaro Herrera  wrote:
>
> Yeah, something like that.  However, if the child had a NOT NULL
> constraint of its own, then it should not be deleted when the
> PK-on-parent is, but merely marked as no longer inherited.  (This is
> also what happens with a straight NOT NULL constraint.)  I think what
> this means is that at some point during the deletion of the PK we must
> remove the dependency link rather than letting it be followed.  I'm not
> yet sure how to do this.
>

I'm not sure that adding that new dependency was the right thing to
do. I think perhaps this could just be made to work using conislocal
and coninhcount to track whether the child constraint needs to be
deleted, or just updated.

> Anyway, I was at the same time fixing the other problem you reported
> with inheritance (namely, adding a PK ends up with the child column
> being marked NOT NULL but no corresponding constraint).
>
> At some point I wondered if the easy way out wouldn't be to give up on
> the idea that creating a PK causes the child columns to be marked
> not-nullable.  However, IIRC I decided against that because it breaks
> restoring of old dumps, so it wouldn't be acceptable.
>
> To make matters worse: pg_dump creates the PK as
>
>   ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )
>
> note the ONLY there.  It seems I'm forced to cause the PK to affect
> children even though ONLY is given.  This is undesirable but I don't see
> a way out of that.
>
> It is all a bit of a rat's nest.
>

I wonder if that could be made to work in the same way as inherited
CHECK constraints -- dump the child's inherited NOT NULL constraints,
and then manually update conislocal in pg_constraint.

Regards,
Dean

Re: cataloguing NOT NULL constraints

2023-08-05 Thread Alvaro Herrera

On 2023-Aug-05, Dean Rasheed wrote:

> Hmm, thinking about this some more, I think this might be the wrong
> approach to fixing the original problem. I think it was probably OK
> that the NOT NULL constraint on the child was marked as inherited, but
> I think what should have happened is that dropping the PRIMARY KEY
> constraint on the parent should have caused the NOT NULL constraint on
> the child to have been deleted (in the same way as it would have been,
> if it had been a NOT NULL constraint on the parent).

Yeah, something like that.  However, if the child had a NOT NULL
constraint of its own, then it should not be deleted when the
PK-on-parent is, but merely marked as no longer inherited.  (This is
also what happens with a straight NOT NULL constraint.)  I think what
this means is that at some point during the deletion of the PK we must
remove the dependency link rather than letting it be followed.  I'm not
yet sure how to do this.

Anyway, I was at the same time fixing the other problem you reported
with inheritance (namely, adding a PK ends up with the child column
being marked NOT NULL but no corresponding constraint).

At some point I wondered if the easy way out wouldn't be to give up on
the idea that creating a PK causes the child columns to be marked
not-nullable.  However, IIRC I decided against that because it breaks
restoring of old dumps, so it wouldn't be acceptable.

To make matters worse: pg_dump creates the PK as 

  ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )

note the ONLY there.  It seems I'm forced to cause the PK to affect
children even though ONLY is given.  This is undesirable but I don't see
a way out of that.

It is all a bit of a rat's nest.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

Re: cataloguing NOT NULL constraints

2023-08-05 Thread Dean Rasheed

On Fri, 4 Aug 2023 at 19:10, Alvaro Herrera  wrote:
>
> On 2023-Jul-28, Alvaro Herrera wrote:
>
> > To avoid that, one option would be to make this NN constraint
> > undroppable ...  but I don't see how.  One option might be to add a
> > pg_depend row that links the NOT NULL constraint to its PK constraint.
> > But this will be a strange case that occurs nowhere else, since other
> > NOT NULL constraint don't have such pg_depend rows.  Also, I won't know
> > how pg_dump likes this until I implement it.
>
> I've been completing the implementation for this.  It seems to work
> reasonably okay; pg_dump requires somewhat strange contortions, but they
> are similar to what we do in flagInhTables already, so I don't feel too
> bad about that.
>
> What *is* odd and bothersome is that it also causes a problem dropping
> the child table.
>

Hmm, thinking about this some more, I think this might be the wrong
approach to fixing the original problem. I think it was probably OK
that the NOT NULL constraint on the child was marked as inherited, but
I think what should have happened is that dropping the PRIMARY KEY
constraint on the parent should have caused the NOT NULL constraint on
the child to have been deleted (in the same way as it would have been,
if it had been a NOT NULL constraint on the parent).

Regards,
Dean

Re: cataloguing NOT NULL constraints

2023-08-04 Thread Alvaro Herrera

On 2023-Jul-28, Alvaro Herrera wrote:

> To avoid that, one option would be to make this NN constraint
> undroppable ...  but I don't see how.  One option might be to add a
> pg_depend row that links the NOT NULL constraint to its PK constraint.
> But this will be a strange case that occurs nowhere else, since other
> NOT NULL constraint don't have such pg_depend rows.  Also, I won't know
> how pg_dump likes this until I implement it.

I've been completing the implementation for this.  It seems to work
reasonably okay; pg_dump requires somewhat strange contortions, but they
are similar to what we do in flagInhTables already, so I don't feel too
bad about that.

What *is* odd and bothersome is that it also causes a problem dropping
the child table.  For example,

CREATE TABLE parent (a int primary key);
CREATE TABLE child () INHERITS (parent);
\d+ child

 Tabla «public.child»
 Columna │  Tipo   │ Ordenamiento │ Nulable  │ Por omisión │ Almacenamiento │ 
Compresión │ Estadísticas │ Descripción 
─┼─┼──┼──┼─┼┼┼──┼─
 a   │ integer │  │ not null │ │ plain  │   
 │  │ 
Not null constraints:
"child_a_not_null" NOT NULL "a"
Hereda: parent
Método de acceso: heap

This is the behavior that I think we wanted to prevent drop of the child
constraint, and it seems okay to me:

=# alter table child drop constraint child_a_not_null;
ERROR:  cannot drop constraint child_a_not_null on table child because 
constraint parent_pkey on table parent requires it
SUGERENCIA:  You can drop constraint parent_pkey on table parent instead.

But the problem is this:

=# drop table child;
ERROR:  cannot drop table child because other objects depend on it
DETALLE:  constraint parent_pkey on table parent depends on table child
SUGERENCIA:  Use DROP ... CASCADE to drop the dependent objects too.

To be clear, what my patch is doing is add one new dependency:

dep │  ref  
 │ deptype 
┼┼─
 type foo   │ table foo 
 │ i
 table foo  │ schema public 
 │ n
 constraint foo_pkey on table foo   │ column a of table foo 
 │ a
 type bar   │ table bar 
 │ i
 table bar  │ schema public 
 │ n
 table bar  │ table foo 
 │ n
 constraint bar_a_not_null on table bar │ column a of table bar 
 │ a
 constraint child_a_not_null on table child │ column a of table child   
 │ a
 constraint child_a_not_null on table child │ constraint parent_pkey on table 
parent │ i

the last row here is what is new.  I'm not sure what's the right fix.
Maybe I need to invert the direction of that dependency.

Even with that fixed, I'd still need to write more code so that ALTER
TABLE INHERIT adds the link (I already patched the DROP INHERIT part).
Not sure what else might I be missing.

Separately, I also noticed that some code that's currently
dropconstraint_internal needs to be moved to DropConstraintById, because
if the PK is dropped for some other reason than ALTER TABLE DROP
CONSTRAINT, some ancillary actions are not taken.

Sigh.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
 Are you not unsure you want to delete Firefox?
   [Not unsure] [Not not unsure][Cancel]
   http://smylers.hates-software.com/2008/01/03/566e45b2.html

Re: cataloguing NOT NULL constraints

2023-08-02 Thread Peter Eisentraut


On 24.07.23 12:32, Alvaro Herrera wrote:

However, 11.16 ( as part of 11.12 ), says that DROP NOT NULL causes the indication of
the column as NOT NULL to be removed.  This, to me, says that if you do
have multiple such constraints, you'd better remove them all with that
command.  All in all, I lean towards allowing just one as best as we
can.


Another clue is in 11.15 , which says

1) Let C be the column identified by the  CN in the
containing . If the column descriptor of C
does not contain an indication that C is defined as NOT NULL, then:

[do things]

Otherwise it does nothing.  So there can only be one such constraint per 
table.

Re: cataloguing NOT NULL constraints

2023-07-28 Thread Alvaro Herrera

> > Given the following sequence:
> > 
> > drop table if exists p,c;
> > create table p(a int primary key);
> > create table c() inherits (p);
> > alter table p drop constraint p_pkey;

> > However, c.a remains non-nullable, with a NOT NULL constraint that
> > claims to be inherited:
> > 
> > \d+ c
> > Table "public.c"
> >  Column |  Type   | Collation | Nullable | Default | Storage |
> > Compression | Stats target | Description
> > +-+---+--+-+-+-+--+-
> >  a  | integer |   | not null | | plain   |
> > |  |
> > Not null constraints:
> > "c_a_not_null" NOT NULL "a" (inherited)
> > Inherits: p
> > Access method: heap
> > 
> > That's a problem, because now the NOT NULL constraint on c cannot be
> > dropped (attempting to drop it on c errors out because it thinks it's
> > inherited, but it can't be dropped via p, because p.a is already
> > nullable).

So I implemented a fix for this (namely: fix the inhcount to be 0
initially), and it works well, but it does cause a definitional problem:
any time we create a child table that inherits from another table that
has a primary key, all the columns in the child table will get normal,
visible, droppable NOT NULL constraints.  Thus, pg_dump for example will
output that constraint exactly as if the user had specified it in the
child's CREATE TABLE command.  By itself this doesn't bother me, though
I admit it seems a little odd.

When you restore such a setup from pg_dump, things work perfectly -- I
mean, you don't get a second constraint.  But if you do drop the
constraint, then it will be reinstated by the next pg_dump as if you
hadn't dropped it, by way of it springing to life from the PK.

To avoid that, one option would be to make this NN constraint
undroppable ...  but I don't see how.  One option might be to add a
pg_depend row that links the NOT NULL constraint to its PK constraint.
But this will be a strange case that occurs nowhere else, since other
NOT NULL constraint don't have such pg_depend rows.  Also, I won't know
how pg_dump likes this until I implement it.

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

Re: cataloguing NOT NULL constraints

2023-07-26 Thread Alvaro Herrera

On 2023-Jul-26, Alvaro Herrera wrote:

> On 2023-Jul-26, Dean Rasheed wrote:
> 
> > The new \d+ output certainly makes testing and reviewing easier,
> > though I do understand people's concerns that this may make the output
> > significantly longer in many real-world cases.
> 
> Yeah, at this point I'm inclined to get the \d+ version committed
> immediately after the main patch, and we can tweak the psql UI after the
> fact -- for instance so that they are only shown in \d++, or some other
> idea we may come across.

(For example, maybe we could add \dtc [PATTERN] or some such, that lists
all the constraints of all kinds in tables matching PATTERN.)

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"If you want to have good ideas, you must have many ideas.  Most of them
will be wrong, and what you have to learn is which ones to throw away."
 (Linus Pauling)

Re: cataloguing NOT NULL constraints

2023-07-26 Thread Alvaro Herrera

Thanks for spending so much time with this patch -- really appreciated.

On 2023-Jul-26, Dean Rasheed wrote:

> On Tue, 25 Jul 2023 at 13:36, Alvaro Herrera  wrote:
> >
> > Okay then, I've made these show up in the footer of \d+.  This is in
> > patch 0003 here.  Please let me know what do you think of the regression
> > changes.
> 
> The new \d+ output certainly makes testing and reviewing easier,
> though I do understand people's concerns that this may make the output
> significantly longer in many real-world cases. I don't think it would
> be a good idea to filter the list in any way though, because I think
> that will only lead to confusion. I think it should be all-or-nothing,
> though I'm not necessarily opposed to using something like \d++ to
> enable it, if that turns out to be the least-bad option.

Yeah, at this point I'm inclined to get the \d+ version committed
immediately after the main patch, and we can tweak the psql UI after the
fact -- for instance so that they are only shown in \d++, or some other
idea we may come across.

> Going back to this example:
> 
> drop table if exists p1, p2, foo;
> create table p1(a int not null check (a > 0));
> create table p2(a int not null check (a > 0));
> create table foo () inherits (p1,p2);

> I remain of the opinion that that should create 2 NOT NULL constraints
> on foo, for consistency with CHECK constraints, and the misleading
> name that results if p1_a_not_null is dropped from p1. That way, the
> names of inherited NOT NULL constraints could be kept in sync, as they
> are for other constraint types, making it easier to keep track of
> where they come from, and it wouldn't be necessary to treat them
> differently (e.g., matching by column number, when dropping NOT NULL
> constraints).

I think having two constraints is more problematic, UI-wise.  Previous
versions of this patchset did it that way, and it's not great: for
example ALTER TABLE ALTER COLUMN DROP NOT NULL fails and tells you to
choose which exact constraint you want to drop and use DROP CONSTRAINT
instead.  And when searching for the not-null constraints for a column,
the code had to consider the case of there being multiple ones, which
led to strange contortions.  Allowing a single one is simpler and covers
all important cases well.

Anyway, you still can't drop the doubly-inherited constraint directly,
because it'll complain that it is an inherited constraint.  So you have
to deinherit first and only then can you drop the constraint.

Now, one possible improvement here would be to ignore the parent
constraint's name, and have 'foo' recompute its own constraint name from
scratch, inheriting the name only if one of the parents had a
manually-specified constraint name (and we would choose the first one,
if there's more than one).  I think complicating things more than that
is unnecessary -- particularly considering that legacy inheritance is,
well, legacy, and I doubt people are relying too much on it.

> Given the following sequence:
> 
> drop table if exists p,c;
> create table p(a int primary key);
> create table c() inherits (p);
> alter table p drop constraint p_pkey;
> 
> p.a ends up being nullable, where previously it would have been left
> non-nullable. That change makes sense, and is presumably one of the
> benefits of tying the nullability of columns to pg_constraint entries.

Right.

> However, c.a remains non-nullable, with a NOT NULL constraint that
> claims to be inherited:
> 
> \d+ c
> Table "public.c"
>  Column |  Type   | Collation | Nullable | Default | Storage |
> Compression | Stats target | Description
> +-+---+--+-+-+-+--+-
>  a  | integer |   | not null | | plain   |
> |  |
> Not null constraints:
> "c_a_not_null" NOT NULL "a" (inherited)
> Inherits: p
> Access method: heap
> 
> That's a problem, because now the NOT NULL constraint on c cannot be
> dropped (attempting to drop it on c errors out because it thinks it's
> inherited, but it can't be dropped via p, because p.a is already
> nullable).

Oh, I think the bug here is just that this constraint should not claim
to be inherited, but standalone.  So you can drop it afterwards; but if
you drop it and end up with NULL values in your PK-labelled column in
the parent table, that's on you.

> I wonder if NOT NULL constraints created as a result of inherited PKs
> should have names based on the PK name (e.g.,
> __not_null), to make it more obvious where they
> came from. That would be more consistent with the way NOT NULL
> constraint names are inherited.

Hmm, interesting idea.  I'll play with it.  (It may quickly lead to
constraint names that are too long, though.)

> Given the following sequence:
> 
> drop table if exists p,c;
> create table p(a int);
> create table c() inherits (p);
> alter table p add primary key (a);
> 
> c.a ends up

Re: cataloguing NOT NULL constraints

2023-07-26 Thread Dean Rasheed

On Tue, 25 Jul 2023 at 13:36, Alvaro Herrera  wrote:
>
> Okay then, I've made these show up in the footer of \d+.  This is in
> patch 0003 here.  Please let me know what do you think of the regression
> changes.
>

The new \d+ output certainly makes testing and reviewing easier,
though I do understand people's concerns that this may make the output
significantly longer in many real-world cases. I don't think it would
be a good idea to filter the list in any way though, because I think
that will only lead to confusion. I think it should be all-or-nothing,
though I'm not necessarily opposed to using something like \d++ to
enable it, if that turns out to be the least-bad option.

Going back to this example:

drop table if exists p1, p2, foo;
create table p1(a int not null check (a > 0));
create table p2(a int not null check (a > 0));
create table foo () inherits (p1,p2);
\d+ foo

   Table "public.foo"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
+-+---+--+-+-+-+--+-
 a  | integer |   | not null | | plain   |
|  |
Check constraints:
"p1_a_check" CHECK (a > 0)
"p2_a_check" CHECK (a > 0)
Not null constraints:
"p1_a_not_null" NOT NULL "a" (inherited)
Inherits: p1,
  p2
Access method: heap

I remain of the opinion that that should create 2 NOT NULL constraints
on foo, for consistency with CHECK constraints, and the misleading
name that results if p1_a_not_null is dropped from p1. That way, the
names of inherited NOT NULL constraints could be kept in sync, as they
are for other constraint types, making it easier to keep track of
where they come from, and it wouldn't be necessary to treat them
differently (e.g., matching by column number, when dropping NOT NULL
constraints).

Doing a little more testing, I found some other issues.

Given the following sequence:

drop table if exists p,c;
create table p(a int primary key);
create table c() inherits (p);
alter table p drop constraint p_pkey;

p.a ends up being nullable, where previously it would have been left
non-nullable. That change makes sense, and is presumably one of the
benefits of tying the nullability of columns to pg_constraint entries.
However, c.a remains non-nullable, with a NOT NULL constraint that
claims to be inherited:

\d+ c
Table "public.c"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
+-+---+--+-+-+-+--+-
 a  | integer |   | not null | | plain   |
|  |
Not null constraints:
"c_a_not_null" NOT NULL "a" (inherited)
Inherits: p
Access method: heap

That's a problem, because now the NOT NULL constraint on c cannot be
dropped (attempting to drop it on c errors out because it thinks it's
inherited, but it can't be dropped via p, because p.a is already
nullable).

I wonder if NOT NULL constraints created as a result of inherited PKs
should have names based on the PK name (e.g.,
__not_null), to make it more obvious where they
came from. That would be more consistent with the way NOT NULL
constraint names are inherited.

Given the following sequence:

drop table if exists p,c;
create table p(a int);
create table c() inherits (p);
alter table p add primary key (a);

c.a ends up non-nullable, but there is no pg_constraint entry
enforcing the constraint:

\d+ c
Table "public.c"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
+-+---+--+-+-+-+--+-
 a  | integer |   | not null | | plain   |
|  |
Inherits: p
Access method: heap

Given a database containing these 2 tables:

create table p(a int primary key);
create table c() inherits (p);

doing a pg_dump and restore fails to restore the NOT NULL constraint
on c, because all constraints created by the dump are local to p.

That's it for now. I'll try to do more testing later.

Regards,
Dean

Re: cataloguing NOT NULL constraints

2023-07-25 Thread Robert Haas

On Tue, Jul 25, 2023 at 3:07 PM Isaac Morland  wrote:
> OK, I suppose ALTER CONSTRAINT to change the deferrable status and validity 
> (that is why we're doing this, right?) needs the constraint name. But the 
> constraint name is formulaic by default, and my proposal is to suppress it 
> only when it matches the formula, so you could just construct the constraint 
> name using the documented formula if it's not explicitly listed.
>
> I really don’t see it as a good use of space to add n lines to the \d+ 
> display just to confirm that the "not null" designations in the "Nullable" 
> column are implemented by named constraints with the expected names.

Yeah, I mean, I get that. That was my initial concern, too. But I also
think if there's some complicated rule that determines what gets
displayed and what doesn't, nobody's going to remember it, and then
when you don't see something, you're never going to be sure exactly
what's going on. Displaying everything is going to be clunky
especially if, like me, you tend to be careful to mark columns NOT
NULL when they are, but when something goes wrong, the last thing you
want to do is run a \d command and have it show you incomplete
information.

I can't count the number of times that somebody's shown me the output
of a query against pg_locks or pg_stat_activity that had been filtered
to remove irrelevant information and it turned out that the hidden
information was not so irrelevant as the person who wrote the query
thought. It happens all the time. I don't want to create the same kind
of situation here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: cataloguing NOT NULL constraints

2023-07-25 Thread Isaac Morland

On Tue, 25 Jul 2023 at 14:59, Robert Haas  wrote:

> On Tue, Jul 25, 2023 at 1:33 PM Isaac Morland 
> wrote:
> > My suggestion is for \d+ to show NOT NULL constraints only if there is
> something weird going on (wrong name, duplicate constraints, …). If there
> is nothing weird about the constraint then explicitly listing it provides
> absolutely no information that is not given by "not null" in the "Nullable"
> column. Easier said than done I suppose. I'm just worried about my \d+
> displays becoming less useful.
>
> I mean, the problem is that if you want to ALTER TABLE .. DROP
> CONSTRAINT, you need to know what the valid arguments to that command
> are, and the names of these constraints will be just as valid as the
> names of any other constraints.
>

Can't I just ALTER TABLE … DROP NOT NULL still?

OK, I suppose ALTER CONSTRAINT to change the deferrable status and validity
(that is why we're doing this, right?) needs the constraint name. But the
constraint name is formulaic by default, and my proposal is to suppress it
only when it matches the formula, so you could just construct the
constraint name using the documented formula if it's not explicitly listed.

I really don’t see it as a good use of space to add n lines to the \d+
display just to confirm that the "not null" designations in the "Nullable"
column are implemented by named constraints with the expected names.

Re: cataloguing NOT NULL constraints

2023-07-25 Thread Robert Haas

On Tue, Jul 25, 2023 at 1:33 PM Isaac Morland  wrote:
> My suggestion is for \d+ to show NOT NULL constraints only if there is 
> something weird going on (wrong name, duplicate constraints, …). If there is 
> nothing weird about the constraint then explicitly listing it provides 
> absolutely no information that is not given by "not null" in the "Nullable" 
> column. Easier said than done I suppose. I'm just worried about my \d+ 
> displays becoming less useful.

I mean, the problem is that if you want to ALTER TABLE .. DROP
CONSTRAINT, you need to know what the valid arguments to that command
are, and the names of these constraints will be just as valid as the
names of any other constraints.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: cataloguing NOT NULL constraints

2023-07-25 Thread Isaac Morland

On Tue, 25 Jul 2023 at 12:24, Alvaro Herrera 
wrote:

> On 2023-Jul-25, Isaac Morland wrote:
>
> > I agree. I definitely do *not* want a bunch of NOT NULL constraint names
> > cluttering up displays. Can we legislate that all NOT NULL implementing
> > constraints are named by mashing together the table name, column name,
> and
> > something to identify it as a NOT NULL constraint?
>
> All constraints are named like that already, and NOT NULL constraints
> just inherited the same idea.  The names are __not_null
> for NOT NULL constraints.  pg_dump goes great lengths to avoid printing
> constraint names when they have this pattern.
>

OK, this is helpful. Can \d do the same thing? I use a lot of NOT NULL
constraints and I very seriously do not want \d (including \d+) to have an
extra line for almost every column. It's just noise, and while my screen is
large, it's still not infinite.

I do not want these constraint names cluttering the output either.
> That's why I propose moving them to a new \d++ command, where they will
> only bother you if you absolutely need them.  But so far I have only one
> vote supporting that idea.

My suggestion is for \d+ to show NOT NULL constraints only if there is
something weird going on (wrong name, duplicate constraints, …). If there
is nothing weird about the constraint then explicitly listing it provides
absolutely no information that is not given by "not null" in the "Nullable"
column. Easier said than done I suppose. I'm just worried about my \d+
displays becoming less useful.

Re: cataloguing NOT NULL constraints

2023-07-25 Thread Alvaro Herrera

On 2023-Jul-25, Isaac Morland wrote:

> I agree. I definitely do *not* want a bunch of NOT NULL constraint names
> cluttering up displays. Can we legislate that all NOT NULL implementing
> constraints are named by mashing together the table name, column name, and
> something to identify it as a NOT NULL constraint?

All constraints are named like that already, and NOT NULL constraints
just inherited the same idea.  The names are __not_null
for NOT NULL constraints.  pg_dump goes great lengths to avoid printing
constraint names when they have this pattern.

I do not want these constraint names cluttering the output either.
That's why I propose moving them to a new \d++ command, where they will
only bother you if you absolutely need them.  But so far I have only one
vote supporting that idea.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: cataloguing NOT NULL constraints

2023-07-25 Thread Isaac Morland

On Tue, 25 Jul 2023 at 11:39, Robert Haas  wrote:

>
> I'm not really thrilled with the idea of every not-null constraint
> having a name, to be honest. Of all the kinds of constraints that we
> have in the system, NOT NULL constraints are probably the ones where
> naming them is least likely to be interesting, because they don't
> really have any interesting properties. A CHECK constraint has an
> expression; a foreign key constraint has columns that it applies to on
> each side plus the identity of the table and opclass information, but
> a NOT NULL constraint seems like it can never have any property other
> than which column. So it sort of seems like a waste to name it. But if
> we want it catalogued then we don't really have an option, so I
> suppose we just have to accept a bit of clutter as the price of doing
> business.
>

I agree. I definitely do *not* want a bunch of NOT NULL constraint names
cluttering up displays. Can we legislate that all NOT NULL implementing
constraints are named by mashing together the table name, column name, and
something to identify it as a NOT NULL constraint? Maybe even something
like pg_not_null_[relname]_[attname] (with some escaping), using the pg_
prefix to make the name reserved similar to schemas and tables? And then
don't show such constraints in \d, not even \d+ - just indicate it in
the Nullable column of the column listing as done now. Show a NOT NULL
constraint if there is something odd about it - for example, if it gets
renamed, or not renamed when the table is renamed.

Sorry for the noise if this has already been decided otherwise.

Re: cataloguing NOT NULL constraints

2023-07-25 Thread Robert Haas

On Tue, Jul 25, 2023 at 8:36 AM Alvaro Herrera  wrote:
> Okay then, I've made these show up in the footer of \d+.  This is in
> patch 0003 here.  Please let me know what do you think of the regression
> changes.

Seems OK.

I'm not really thrilled with the idea of every not-null constraint
having a name, to be honest. Of all the kinds of constraints that we
have in the system, NOT NULL constraints are probably the ones where
naming them is least likely to be interesting, because they don't
really have any interesting properties. A CHECK constraint has an
expression; a foreign key constraint has columns that it applies to on
each side plus the identity of the table and opclass information, but
a NOT NULL constraint seems like it can never have any property other
than which column. So it sort of seems like a waste to name it. But if
we want it catalogued then we don't really have an option, so I
suppose we just have to accept a bit of clutter as the price of doing
business.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: cataloguing NOT NULL constraints

2023-07-24 Thread Robert Haas

On Mon, Jul 24, 2023 at 6:33 AM Alvaro Herrera  wrote:
> That's the first thing I proposed actually.  I got one vote down from
> Robert Haas[1], but while the idea seems to have had support from Justin
> Pryzby (in \dt++) [2] and definitely did from Peter Eisentraut [3], I do
> not like it too much myself, mainly because the partition list has a
> very similar treatment and I find that one an annoyance.

I think I might want to retract my earlier -1 vote. I mean, I agree
with former me that having the \d+ output get a whole lot longer is
not super-appealing. But I also agree with Dean that having this
information available somewhere is probably important, and I also
agree with your point that inventing \d++ for this isn't necessarily a
good idea. I fear that will just result in having to type an extra
plus sign any time you want to see all of the table details, to make
sure that psql knows that you really mean it. So, maybe showing it in
the \d+ output as Dean proposes is the least of evils.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: cataloguing NOT NULL constraints

On Mon, 24 Jul 2023 at 17:42, Alvaro Herrera  wrote:
>
> On 2023-Jul-24, Dean Rasheed wrote:
>
> > Something else I noticed: the error message from ALTER TABLE ... ADD
> > CONSTRAINT in the case of a duplicate constraint name is not very
> > friendly:
> >
> > ERROR:  duplicate key value violates unique constraint
> > "pg_constraint_conrelid_contypid_conname_index"
> > DETAIL:  Key (conrelid, contypid, conname)=(16540, 0, nn) already exists.
> >

To reproduce this error, try to create 2 constraints with the same
name on different columns:

create table foo(a int, b int);
alter table foo add constraint nn not null a;
alter table foo add constraint nn not null b;

I found another, separate issue:

create table p1(a int not null);
create table p2(a int);
create table foo () inherits (p1,p2);
alter table p2 add not null a;

ERROR:  column "a" of table "foo" is already NOT NULL

whereas doing "alter table p2 alter column a set not null" works OK,
merging the constraints as expected.

Regards,
Dean

Re: cataloguing NOT NULL constraints

2023-07-24 Thread Vik Fearing


On 7/24/23 18:42, Alvaro Herrera wrote:

55490 17devel 3166154=# create table foo (a int constraint nn not null);
CREATE TABLE
55490 17devel 3166154=# alter table foo add constraint nn not null a;
ERROR:  column "a" of table "foo" is already NOT NULL


Surely this should be a WARNING or INFO?  I see no reason to ERROR here.
--
Vik Fearing

Re: cataloguing NOT NULL constraints

Hello,

While discussing the matter of multiple constraints with Vik Fearing, I
noticed that we were throwing an unnecessary error if you used

CREATE TABLE foo (a int NOT NULL NOT NULL);

That would die with "redundant NOT NULL declarations", but current
master doesn't do that; and we don't do it for UNIQUE UNIQUE either.
So I modified the patch to make it ignore the dupe and create a single
constraint.  This (and rebasing to current master) are the only changes
in v15.

I have not changed the psql presentation, but I'll do as soon as we have
rough consensus on what to do.  To reiterate, the options are:

1. Don't show the constraint names.  This is what the current patch does

2. Show the constraint name in \d+ in the "nullable" column.
   I did this early on, to much booing.

3. Show the constraint name in \d++ (a new command) tabular output

4. Show the constraint name in the footer of \d+
   I also did this at some point; there are some +1s and some -1s.

5. Show the constraint name in the footer of \d++


Many thanks, Dean, for the discussion so far.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.  (Don Knuth)

Re: cataloguing NOT NULL constraints

On 2023-Jul-24, Dean Rasheed wrote:

> Something else I noticed: the error message from ALTER TABLE ... ADD
> CONSTRAINT in the case of a duplicate constraint name is not very
> friendly:
> 
> ERROR:  duplicate key value violates unique constraint
> "pg_constraint_conrelid_contypid_conname_index"
> DETAIL:  Key (conrelid, contypid, conname)=(16540, 0, nn) already exists.
> 
> To match the error message for other constraint types, this should be:
> 
> ERROR:  constraint "nn" for relation "foo" already exists

Hmm, how did you get this one?  I can't reproduce it:

55490 17devel 3166154=# create table foo (a int constraint nn not null);
CREATE TABLE
55490 17devel 3166154=# alter table foo add constraint nn not null a;
ERROR:  column "a" of table "foo" is already NOT NULL

55490 17devel 3166154=# drop table foo;
DROP TABLE

55490 17devel 3166154=# create table foo (a int);
CREATE TABLE
Duración: 1,472 ms
55490 17devel 3166154=# alter table foo add constraint nn not null a, add 
constraint nn not null a;
ERROR:  column "a" of table "foo" is already NOT NULL

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El número de instalaciones de UNIX se ha elevado a 10,
y se espera que este número aumente" (UPM, 1972)

Re: cataloguing NOT NULL constraints

On 2023-Jul-24, Dean Rasheed wrote:

> Hmm, I'm not so sure. I think perhaps multiple NOT NULL constraints on
> the same column should just be allowed, otherwise things might get
> confusing. For example:
> 
 create table p1 (a int not null check (a > 0));
 create table p2 (a int not null check (a > 0));
 create table foo () inherits (p1, p2);

Have a look at the conislocal / coninhcount values.  These should
reflect the fact that the constraint has multiple sources; and the
constraint does disappear if you drop it from both sources.

> If I then drop the p1 constraints:
> 
> alter table p1 drop constraint p1_a_check;
> alter table p1 drop constraint p1_a_not_null;
> 
> I end up with column "a" still being not null, and the "p1_a_not_null"
> constraint still being there on foo, which seems even more
> counter-intuitive, because I just dropped that constraint, and it
> really should now be the "p2_a_not_null" constraint that makes "a" not
> null:

I can see that it might make sense to not inherit the constraint name in
some cases.  Perhaps:

1. never inherit a name.  Each table has its own constraint name always
2. only inherit if there's a single parent
3. always inherit the name from the first parent (current implementation)

> So I'd say that ALTER TABLE ... ADD NOT NULL should always add a
> constraint, even if there already is one. For example ALTER TABLE ...
> ADD UNIQUE does nothing to prevent multiple unique constraints on the
> same column(s). It seems pretty dumb, but maybe there is a reason to
> allow it, and it doesn't feel like we should be second-guessing what
> the user wants.

That was my initial implementation but I changed it to allowing a single
constraint because of the way the standard describes SET NOT NULL;
specifically, 11.15  says that "If the
column descriptor of C does not contain an indication that C is defined
as NOT NULL, then:" a constraint is added; otherwise (i.e., such an
indication does exist), nothing happens.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

Re: cataloguing NOT NULL constraints

On 2023-Jul-24, Dean Rasheed wrote:

> Hmm, I don't particularly like that approach, because I think it will
> be difficult to cram any additional details into the table, and also I
> don't know whether having multiple not null constraints for a
> particular column can be entirely ruled out.
> 
> I may well be in the minority here, but I think the best way is to
> list them in a separate footer section, in the same way as CHECK
> constraints, allowing other constraint properties to be included. So
> it might look something like:

That's the first thing I proposed actually.  I got one vote down from
Robert Haas[1], but while the idea seems to have had support from Justin
Pryzby (in \dt++) [2] and definitely did from Peter Eisentraut [3], I do
not like it too much myself, mainly because the partition list has a
very similar treatment and I find that one an annoyance.

> and also I don't know whether having multiple not null constraints for
> a particular column can be entirely ruled out.

I had another look at the standard.  In 11.26 () it says that "If [the constraint being removed]
causes some column COL to be known not nullable and no other constraint
causes COL to be known not nullable, then the nullability characteristic
of the column descriptor of COL is changed to possibly nullable".  Which
supports the idea that there might be multiple such constraints.
(However, we could also read this as meaning that the PK could be one
such constraint while NOT NULL is another one.)

However, 11.16 ( as part of 11.12 ), says that DROP NOT NULL causes the indication of
the column as NOT NULL to be removed.  This, to me, says that if you do
have multiple such constraints, you'd better remove them all with that
command.  All in all, I lean towards allowing just one as best as we
can.

[1] 
https://postgr.es/m/ca+tgmobnoxt83y1qesbnvarhfm-flwwkduyiv84e+psaydw...@mail.gmail.com
[2] https://postgr.es/m/20230301223214.GC4268%40telsasoft.com
[3] https://postgr.es/m/1c4f3755-2d10-cae9-647f-91a9f006410e%40enterprisedb.com

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)

Re: cataloguing NOT NULL constraints

Something else I noticed: the error message from ALTER TABLE ... ADD
CONSTRAINT in the case of a duplicate constraint name is not very
friendly:

ERROR:  duplicate key value violates unique constraint
"pg_constraint_conrelid_contypid_conname_index"
DETAIL:  Key (conrelid, contypid, conname)=(16540, 0, nn) already exists.

To match the error message for other constraint types, this should be:

ERROR:  constraint "nn" for relation "foo" already exists

Regards,
Dean

Re: cataloguing NOT NULL constraints

On Thu, 20 Jul 2023 at 16:31, Alvaro Herrera  wrote:
>
> On 2023-Jul-13, Dean Rasheed wrote:
>
> > Something else I noticed is that the result from "ALTER TABLE ...
> > ALTER COLUMN ... DROP NOT NULL" is no longer easily predictable -- if
> > there are multiple NOT NULL constraints on the column, it just drops
> > one (chosen at random?) and leaves the others. I think that it should
> > either drop all the constraints, or throw an error. Either way, I
> > would expect that if DROP NOT NULL succeeds, the result is that the
> > column is nullable.
>
> Hmm, there shouldn't be multiple NOT NULL constraints for the same
> column; if there's one, a further SET NOT NULL should do nothing.  At
> some point the code was creating two constraints, but I realized that
> trying to support multiple constraints caused other problems, and it
> seems to serve no purpose, so I removed it.  Maybe there are ways to end
> up with multiple constraints, but at this stage I would say that those
> are bugs to be fixed, rather than something we want to keep.
>

Hmm, I'm not so sure. I think perhaps multiple NOT NULL constraints on
the same column should just be allowed, otherwise things might get
confusing. For example:

create table p1 (a int not null check (a > 0));
create table p2 (a int not null check (a > 0));
create table foo () inherits (p1, p2);

causes foo to have 2 CHECK constraints, but only 1 NOT NULL constraint:

\d foo
Table "public.foo"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 a  | integer |   | not null |
Check constraints:
"p1_a_check" CHECK (a > 0)
"p2_a_check" CHECK (a > 0)
Inherits: p1,
  p2

select conname from pg_constraint where conrelid = 'foo'::regclass;
conname
---
 p1_a_not_null
 p2_a_check
 p1_a_check
(3 rows)

which I find a little counter-intuitive / inconsistent. If I then drop
the p1 constraints:

alter table p1 drop constraint p1_a_check;
alter table p1 drop constraint p1_a_not_null;

I end up with column "a" still being not null, and the "p1_a_not_null"
constraint still being there on foo, which seems even more
counter-intuitive, because I just dropped that constraint, and it
really should now be the "p2_a_not_null" constraint that makes "a" not
null:

\d foo
Table "public.foo"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 a  | integer |   | not null |
Check constraints:
"p2_a_check" CHECK (a > 0)
Inherits: p1,
  p2

select conname from pg_constraint where conrelid = 'foo'::regclass;
conname
---
 p1_a_not_null
 p2_a_check
(2 rows)

I haven't thought through various other cases in any detail, but I
can't help feeling that it would be simpler and more logical /
consistent to just allow multiple NOT NULL constraints on a column,
rather than trying to enforce a rule that only one is allowed. That
way, I think it would be easier for the user to keep track of why a
column is not null.

So I'd say that ALTER TABLE ... ADD NOT NULL should always add a
constraint, even if there already is one. For example ALTER TABLE ...
ADD UNIQUE does nothing to prevent multiple unique constraints on the
same column(s). It seems pretty dumb, but maybe there is a reason to
allow it, and it doesn't feel like we should be second-guessing what
the user wants.

Regards,
Dean

Re: cataloguing NOT NULL constraints

On Thu, 20 Jul 2023 at 16:31, Alvaro Herrera  wrote:
>
> On 2023-Jul-13, Dean Rasheed wrote:
>
> > I see that it's already been discussed, but I don't like the fact that
> > there is no way to get hold of the new constraint names in psql. I
> > think for the purposes of dropping named constraints, and also
> > possible future stuff like NOT VALID / DEFERRABLE constraints, having
> > some way to get their names will be important.
>
> Yeah, so there are two proposals:
>
> 1. Have \d+ replace the "not null" literal in the \d+ display with the
> constraint name; if the column is not nullable because of the primary
> key, it says "(primary key)" instead.  There's a patch for this in the
> thread somewhere.
>
> 2. Same, but use \d++ for this purpose
>
> Using ++ would be a novelty in psql, so I'm hesitant to make that an
> integral part of the current proposal.  However, to me (2) seems to most
> comfortable way forward, because while you're right that people do need
> the constraint name from time to time, this is very seldom the case, so
> polluting \d+ might inconvenience people for not much gain.  And people
> didn't like having the constraint name in \d+.
>
> Do you have an opinion on these ideas?
>

Hmm, I don't particularly like that approach, because I think it will
be difficult to cram any additional details into the table, and also I
don't know whether having multiple not null constraints for a
particular column can be entirely ruled out.

I may well be in the minority here, but I think the best way is to
list them in a separate footer section, in the same way as CHECK
constraints, allowing other constraint properties to be included. So
it might look something like:

\d foo
Table "public.foo"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 a  | integer |   | not null |
 b  | integer |   | not null |
 c  | integer |   | not null |
 d  | integer |   | not null |
Indexes:
"foo_pkey" PRIMARY KEY, btree (a, b)
Check constraints:
"foo_a_check" CHECK (a > 0)
"foo_b_check" CHECK (b > 0) NO INHERIT NOT VALID
Not null constraints:
"foo_c_not_null" NOT NULL c
"foo_d_not_null" NOT NULL d NO INHERIT

As for CHECK constraints, the contents of each constraint line would
match the "table_constraint" SQL syntax needed to reconstruct the
constraint. Doing it this way allows for things like NOT VALID and
DEFERRABLE to be added in the future.

I think that's probably too verbose for a plain "\d", but I think it
would be OK with "\d+".

Regards,
Dean

Re: cataloguing NOT NULL constraints

2023-07-13 Thread Dean Rasheed

On Wed, 12 Jul 2023 at 18:11, Alvaro Herrera  wrote:
>
> v13, because a conflict was just committed to alter_table.sql.
>
> Here I also call out the relcache.c change by making it a separate
> commit.  I'm likely to commit it that way, too.  To recap: the change is
> to have a partitioned table's index list include the primary key, even
> when said primary key is marked invalid.  This turns out to be necessary
> for the currently proposed pg_dump strategy to work; if this is not in
> place, attaching the per-partition PK indexes to the parent index fails
> because it sees that the columns are not marked NOT NULL.
>

Hmm, looking at that change, it looks a little ugly. I think someone
reading that code in the future will have no idea why it's including
some invalid indexes, and not others.

> There are no other changes from v12.  One thing I should probably get
> to, is fixing the constraint name comparison code in pg_dump.  Right now
> it's a bit dumb and will get in silly trouble with overlength
> table/column names (nothing that would actually break, just that it will
> emit constraint names when there's no need to.)
>

Yeah, that seems a bit ugly. Presumably, also, if something like a
column rename happens, the constraint name will no longer match.

I see that it's already been discussed, but I don't like the fact that
there is no way to get hold of the new constraint names in psql. I
think for the purposes of dropping named constraints, and also
possible future stuff like NOT VALID / DEFERRABLE constraints, having
some way to get their names will be important.

Something else I noticed is that the result from "ALTER TABLE ...
ALTER COLUMN ... DROP NOT NULL" is no longer easily predictable -- if
there are multiple NOT NULL constraints on the column, it just drops
one (chosen at random?) and leaves the others. I think that it should
either drop all the constraints, or throw an error. Either way, I
would expect that if DROP NOT NULL succeeds, the result is that the
column is nullable.

Regards,
Dean

Re: cataloguing NOT NULL constraints

2023-07-03 Thread Peter Eisentraut


On 30.06.23 13:44, Alvaro Herrera wrote:

OK, so here's a new attempt to get this working correctly.


Attached is a small fixup patch for the documentation.

Furthermore, there are a few outdated comments that are probably left 
over from previous versions of this patch set.



* src/backend/catalog/pg_constraint.c

Outdated comment:

+   /* only tuples for CHECK constraints should be given */
+   Assert(((Form_pg_constraint) GETSTRUCT(constrTup))->contype == 
CONSTRAINT_NOTNULL);



* src/backend/parser/gram.y

Should processCASbits() set >skip_validation, like in the CHECK
case?  _outConstraint() looks at the field, so it seems relevant.


* src/backend/parser/parse_utilcmd.c

The updated comment says

List   *ckconstraints;  /* CHECK and NOT NULL constraints */

but it seems to me that NOT NULL constraints are not actually added
there but in nnconstraints instead.

It would be nice if the field nnconstraints was listed after
ckconstraints consistently throughout the file.  It's a bit random
right now.

This comment is outdated:

+   /*
+* For NOT NULL declarations, we need to mark the column as
+* not nullable, and set things up to have a CHECK 
constraint

+* created.  Also, duplicate NOT NULL declarations are not
+* allowed.
+*/

About this:

case CONSTR_CHECK:
cxt->ckconstraints = lappend(cxt->ckconstraints, 
constraint);

+
+   /*
+* XXX If the user says CHECK (IS NOT NULL), should we turn
+* that into a regular NOT NULL constraint?
+*/
break;

I think this was decided against.

+   /*
+* Copy NOT NULL constraints, too (these do not require any option 
to have

+* been given).
+*/

Shouldn't that be governed by the INCLUDING CONSTRAINTS option?

Btw., there is some asymmetry here between check constraints and
not-null constraints: Check constraints are in the tuple descriptor,
but not-null constraints are not.  Should that be unified?  Or at
least explained?


* src/include/nodes/parsenodes.h

This comment appears to be outdated:

+ * intermixed in tableElts, and constraints and notnullcols are NIL.  After
+ * parse analysis, tableElts contains just ColumnDefs, notnullcols has been
+ * filled with not-nullable column names from various sources, and 
constraints

+ * contains just Constraint nodes (in fact, only CONSTR_CHECK nodes, in the
+ * present implementation).

There is no "notnullcolls".


* src/test/regress/parallel_schedule

This change appears to be correct, but unrelated to this patch, so I
suggest committing this separately.


* src/test/regress/sql/create_table.sql

-SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 
'part_b'::regclass;
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 
'part_b'::regclass ORDER BY coninhcount DESC, conname;


Maybe add conname to the select list here as well, for consistency with 
nearby queries.
From c481b58c3e48ff0ab4738e5cf2440d2ad6fc3e47 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Mon, 3 Jul 2023 15:44:59 +0200
Subject: [PATCH] fixup! Add pg_constraint rows for NOT NULL constraints

---
 doc/src/sgml/catalogs.sgml| 2 +-
 doc/src/sgml/ref/alter_table.sgml | 9 -
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 9137b1bc58..36460126f4 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -2552,7 +2552,7 @@ pg_constraint 
Columns
   
c = check constraint,
f = foreign key constraint,
-   n = not null constraint,
+   n = not-null constraint,
p = primary key constraint,
u = unique constraint,
t = constraint trigger,
diff --git a/doc/src/sgml/ref/alter_table.sgml 
b/doc/src/sgml/ref/alter_table.sgml
index 0b65731b1f..2c4138e4e9 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -113,13 +113,12 @@
 
 [ CONSTRAINT constraint_name ]
 { CHECK ( expression ) [ NO 
INHERIT ] |
+  NOT NULL column_name [ NO 
INHERIT ] |
   UNIQUE [ NULLS [ NOT ] DISTINCT ] ( column_name [, ... ] ) index_parameters |
   PRIMARY KEY ( column_name [, 
... ] ) index_parameters |
   EXCLUDE [ USING index_method ] 
( exclude_element WITH 
operator [, ... ] ) index_parameters [ WHERE ( predicate ) ] |
   FOREIGN KEY ( column_name [, 
... ] ) REFERENCES reftable [ ( 
refcolumn [, ... ] ) ]
-[ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ] [ ON DELETE referential_action ] [ ON UPDATE referential_action ] |
-  NOT NULL column_name [ NO 
INHERIT ]
-}
+[ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ] [ ON DELETE referential_action ] [ ON UPDATE referential_action ] }
 [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
 
 and table_constraint_using_index is:
@@ -1765,7 +1764,7 @@ Examples
   Compatibility

Re: cataloguing NOT NULL constraints

2023-07-03 Thread Alvaro Herrera

On 2023-Jun-30, Andres Freund wrote:

> On 2023-06-30 13:44:03 +0200, Alvaro Herrera wrote:
> 
> > The main novelty in this version of the patch, is that we now emit
> > "throwaway" NOT NULL constraints when a column is part of the primary
> > key.  Then, after the PK is created, we run a DROP for that constraint.
> > That lets us create the PK without having to scan the table during
> > pg_upgrade.
> 
> Have you considered extending the DDL statement for this purpose? We have
>   ALTER TABLE ... ADD CONSTRAINT ... PRIMARY KEY USING INDEX ...;
> we could just do something similar for the NOT NULL constraint?  Which would
> then delete the separate constraint NOT NULL constraint.

Hmm, I hadn't.  I think if we have to explicitly list the constraint
that we want dropped, then it's pretty much the same than as if we used
a comma-separated list of subcommands, like 

ALTER TABLE ... ADD CONSTRAINT .. PRIMARY KEY (a,b),
   DROP CONSTRAINT pgdump_throwaway_notnull_0,
   DROP CONSTRAINT pgdump_throwaway_notnull_1;

However, I think it would be ideal if we *don't* have to specify the
list of constraints: we would do this on any ALTER TABLE .. ADD
CONSTRAINT PRIMARY KEY, without having any additional clause.

But how to distinguish which NOT NULL markings to drop?  Maybe we would
have to specify a flag at NOT NULL constraint creation time.  So pg_dump
would emit something like

CREATE TABLE foo (a int CONSTRAINT NOT NULL THROWAWAY);
... (much later) ...
ALTER TABLE foo ADD CONSTRAINT .. PRIMARY KEY;

and by the time this second command is run, those throwaway constraints
are removed.  The problems now are 1) how to make this CREATE statement
more SQL-conformant (answer: make pg_dump emit a separate ALTER TABLE
command for the constraint addition; it already knows how to do this, so
it'd be very little code); but also 2) where to store the flag
server-side flag that says this constraint has this property.  I think
it'd have to be a new pg_constraint column, and I don't like to add one
for such a minor issue.

On 2023-Jun-30, Alvaro Herrera wrote:

> Scanning this thread, I think I left one reported issue unfixed related
> to tables created LIKE others.  I'll give it a look later.  Other than
> that I think all bases are covered, but I intend to leave the patch open
> until near the end of the CF, in case someone wants to play with it.

So it was [1] that I meant, where this example was provided:

# create table t1 (c int primary key null unique);  

# create table t2 (like t1);

# alter table t2 alter c drop not null; 

ERROR:  no NOT NULL constraint found to drop

The problem here is that because we didn't give INCLUDING INDEXES in the
LIKE clause, we end up with a column marked NOT NULL for which we have
no pg_constraint row.  Okay, I thought, we can just make sure *not* to
mark that case as not null; that works fine and looks reasonable.
However, it breaks the following use case, which is already in use in
the regression tests and possibly by users:

 CREATE TABLE pk (a int PRIMARY KEY) PARTITION BY RANGE (a);
 CREATE TABLE pk4 (LIKE pk);
 ALTER TABLE pk ATTACH PARTITION pk4 FOR VALUES FROM (3000) TO (4000);
+ERROR:  column "a" in child table must be marked NOT NULL

The problem here is that we were assuming, by the time the third command
is run, that the column had been marked NOT NULL by the second command.
So my solution above is simply not acceptable.  What we must do, in
order to handle this backward-compatibly, is to ensure that a column
part of a PK automatically gets a NOT NULL constraint for all the PK
columns, for the case where INCLUDING INDEXES is not given.  This is the
same we do for regular INHERITS children and PKs.

I'll go write this code now; should be simple enough.

[1] 
https://postgr.es/m/CAMbWs48astPDb3K+L89wb8Yju0jM_Czm8svmU=uzd+wm61c...@mail.gmail.com

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
 It does it in a really, really complicated way
 why does it need to be complicated?
 Because it's MakeMaker.

Re: cataloguing NOT NULL constraints

2023-06-30 Thread Andres Freund

Hi,

On 2023-06-30 13:44:03 +0200, Alvaro Herrera wrote:
> OK, so here's a new attempt to get this working correctly.

Thanks for continuing to work on this!

> The main novelty in this version of the patch, is that we now emit
> "throwaway" NOT NULL constraints when a column is part of the primary
> key.  Then, after the PK is created, we run a DROP for that constraint.
> That lets us create the PK without having to scan the table during
> pg_upgrade.

Have you considered extending the DDL statement for this purpose? We have
  ALTER TABLE ... ADD CONSTRAINT ... PRIMARY KEY USING INDEX ...;
we could just do something similar for the NOT NULL constraint?  Which would
then delete the separate constraint NOT NULL constraint.

Greetings,

Andres Freund

Re: cataloguing NOT NULL constraints

2023-04-09 Thread Tom Lane

Alvaro Herrera  writes:
>> I'm inclined to think that this idea of suppressing the implied
>> NOT NULL from PRIMARY KEY is a nonstarter and we should just
>> go ahead and make such a constraint.  Another idea could be for
>> pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY
>> KEY, and then ALTER DROP NOT NULL.

> I like that second idea, yeah.  It might be tough to make it work, but
> I'll try.

Yeah, I've been thinking more about it, and this might also yield a
workable solution for the TestUpgrade breakage.  The idea would be,
roughly, for pg_dump to emit NOT NULL column decoration in all the
same places it does now, and then to drop it again immediately after
doing ADD PRIMARY KEY if it judges that there was no other reason
to have it.  This gets rid of the inconsistency for --binary-upgrade
which I think is what is causing the breakage.

I also ran into something else I didn't much care for:

regression=# create table foo(f1 int primary key, f2 int);
CREATE TABLE
regression=# create table foochild() inherits(foo);
CREATE TABLE
regression=# alter table only foo alter column f2 set not null;
ERROR:  cannot add constraint only to table with inheritance children
HINT:  Do not specify the ONLY keyword.

Previous versions accepted this case, and I don't really see why
we can't do so with this new implementation -- isn't this exactly
what pg_constraint.connoinherit was invented to represent?  Moreover,
existing pg_dump files can contain precisely this construct, so
blowing it off isn't going to be zero-cost.

regards, tom lane

Re: cataloguing NOT NULL constraints

2023-04-09 Thread Alvaro Herrera

On 2023-Apr-09, Tom Lane wrote:

> In the new dispensation, pg_dump omits the NOT NULL clauses.
> Great, you say, that makes the output more like what the user wrote.
> I'm not so sure.  This means that the ALTER TABLE will be compelled
> to perform a full-table scan to verify that there are no nulls in the
> already-loaded data before it can add the missing NOT NULL constraint.

Yeah, I agree that this unintended consequence isn't very palatable.  I
think the other pg_upgrade problem is easily fixed (haven't tried yet),
but having to rethink the pg_dump representation would likely take
longer than we'd like.

> I'm inclined to think that this idea of suppressing the implied
> NOT NULL from PRIMARY KEY is a nonstarter and we should just
> go ahead and make such a constraint.  Another idea could be for
> pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY
> KEY, and then ALTER DROP NOT NULL.

I like that second idea, yeah.  It might be tough to make it work, but
I'll try.

> In any case, I wonder whether that's the sort of redesign we should
> be doing post-feature-freeze.  It might be best to revert and try
> again in v17.

Yeah, sounds like reverting for now and retrying in v17 with the
discussed changes might be better.

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)

Re: cataloguing NOT NULL constraints

2023-04-09 Thread Tom Lane

Andres Freund writes:
> I think
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2023-04-07%2021%3A16%3A04
> might point out a problem with the pg_dump or pg_upgrade backward compat
> paths:

Yeah, this patch has broken every single upgrade-from-back-branch test.

I think there's a second problem, though: even without considering
back branches, this has changed pg_dump output in a way that
I fear is unacceptable. Consider for instance this table definition
(from rules.sql):

create table rule_and_refint_t1 (
id1a integer,
id1b integer,
primary key (id1a, id1b)
);

This used to be dumped as

CREATE TABLE public.rule_and_refint_t1 (
id1a integer NOT NULL,
id1b integer NOT NULL
);
...
... load data ...
...
ALTER TABLE ONLY public.rule_and_refint_t1
ADD CONSTRAINT rule_and_refint_t1_pkey PRIMARY KEY (id1a, id1b);

In the new dispensation, pg_dump omits the NOT NULL clauses.
Great, you say, that makes the output more like what the user wrote.
I'm not so sure. This means that the ALTER TABLE will be compelled
to perform a full-table scan to verify that there are no nulls in the
already-loaded data before it can add the missing NOT NULL constraint.
The old dump output was carefully designed to avoid the need for that
scan. Admittedly, we have to do a scan anyway to build the index,
so this is strictly less than a 2X penalty on the ALTER, but is
that acceptable? It might be all right in the context of regular
dump/restore, where we're surely doing a lot of per-row work anyway
to load the data and make the index. In the context of pg_upgrade,
though, it seems absolutely disastrous: there will now be a per-row
cost where there was none before, and that is surely a deal-breaker.

BTW, I note from testing that the NOT NULL clauses *are* still
emitted in at least some cases when doing --binary-upgrade from an old
version. (This may be directly related to the buildfarm failures,
not sure.) That's no solution though, because now what you get in
pg_constraint will differ depending on which way you upgraded,
which seems unacceptable too.

I'm inclined to think that this idea of suppressing the implied
NOT NULL from PRIMARY KEY is a nonstarter and we should just
go ahead and make such a constraint. Another idea could be for
pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY
KEY, and then ALTER DROP NOT NULL.

In any case, I wonder whether that's the sort of redesign we should
be doing post-feature-freeze. It might be best to revert and try
again in v17.

regards, tom lane

Re: cataloguing NOT NULL constraints

Hi,

On 2023-04-07 17:19:42 -0700, Andres Freund wrote:
> I think
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2023-04-07%2021%3A16%3A04
> might point out a problem with the pg_dump or pg_upgrade backward compat
> paths:
> 
> --- C:\\prog\\bf/root/upgrade.drongo/HEAD/origin-REL9_5_STABLE.sql.fixed  
> 2023-04-07 23:51:27.641328600 +
> +++ 
> C:\\prog\\bf/root/upgrade.drongo/HEAD/converted-REL9_5_STABLE-to-HEAD.sql.fixed
>2023-04-07 23:51:27.672571900 +
> @@ -416,9 +416,9 @@
>  -- Name: entry; Type: TABLE; Schema: public; Owner: buildfarm
>  --
>  CREATE TABLE public.entry (
> -accession text,
> -eid integer,
> -txid smallint
> +accession text NOT NULL,
> +eid integer NOT NULL,
> +txid smallint NOT NULL
>  );
>  ALTER TABLE public.entry OWNER TO buildfarm;
>  --
> 
> Looks like we're making up NOT NULL constraints when migrating from 9.5, for
> some reason?

My compiler complains:

../../../../home/andres/src/postgresql/src/backend/catalog/heap.c: In function 
‘AddRelationNotNullConstraints’:
../../../../home/andres/src/postgresql/src/backend/catalog/heap.c:2829:37: 
warning: ‘conname’ may be used uninitialized [-Wmaybe-uninitialized]
 2829 | if (strcmp(lfirst(lc2), conname) == 0)
  | ^~~~
../../../../home/andres/src/postgresql/src/backend/catalog/heap.c:2802:29: 
note: ‘conname’ was declared here
 2802 | char   *conname;
  | ^~~

I think the compiler may be right - I think the first use of conname might
have been intended as constr->conname?

Greetings,

Andres Freund

Re: cataloguing NOT NULL constraints

Hi,

On 2023-04-07 18:26:28 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2023-04-07 17:46:33 -0400, Tom Lane wrote:
> >> After quickly eyeing the diffs, I'm just going to take the new output
> >> as good.  I'm not surprised that there are additional output messages
> >> given the additional catalog entries this made.  I *am* a bit surprised
> >> that some messages seem to have disappeared --- are there places where
> >> this resulted in fewer catalog accesses than before?  Nonetheless,
> >> there's no good reason to assume this test is exposing any bugs.
> 
> > I wonder if the issue is that the new paths miss a hook invocation.
> 
> Perhaps.  I'm content to silence the buildfarm for today; we can
> investigate more closely later.

Makes sense.

I think
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2023-04-07%2021%3A16%3A04
might point out a problem with the pg_dump or pg_upgrade backward compat
paths:

--- C:\\prog\\bf/root/upgrade.drongo/HEAD/origin-REL9_5_STABLE.sql.fixed
2023-04-07 23:51:27.641328600 +
+++ 
C:\\prog\\bf/root/upgrade.drongo/HEAD/converted-REL9_5_STABLE-to-HEAD.sql.fixed 
2023-04-07 23:51:27.672571900 +
@@ -416,9 +416,9 @@
 -- Name: entry; Type: TABLE; Schema: public; Owner: buildfarm
 --
 CREATE TABLE public.entry (
-accession text,
-eid integer,
-txid smallint
+accession text NOT NULL,
+eid integer NOT NULL,
+txid smallint NOT NULL
 );
 ALTER TABLE public.entry OWNER TO buildfarm;
 --

Looks like we're making up NOT NULL constraints when migrating from 9.5, for
some reason?

Greetings,

Andres Freund

Re: cataloguing NOT NULL constraints

2023-04-07 Thread Tom Lane

... BTW, shouldn't
https://commitfest.postgresql.org/42/3869/
now get closed as committed?

regards, tom lane

Re: cataloguing NOT NULL constraints

2023-04-07 Thread Tom Lane

Andres Freund  writes:
> On 2023-04-07 17:46:33 -0400, Tom Lane wrote:
>> After quickly eyeing the diffs, I'm just going to take the new output
>> as good.  I'm not surprised that there are additional output messages
>> given the additional catalog entries this made.  I *am* a bit surprised
>> that some messages seem to have disappeared --- are there places where
>> this resulted in fewer catalog accesses than before?  Nonetheless,
>> there's no good reason to assume this test is exposing any bugs.

> I wonder if the issue is that the new paths miss a hook invocation.

Perhaps.  I'm content to silence the buildfarm for today; we can
investigate more closely later.

regards, tom lane

Re: cataloguing NOT NULL constraints

Hi,

On 2023-04-07 17:46:33 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote:
> >> Ah, cool, no worries.  I would have stopped indeed, but I had to stay
> >> around in case of any test failures.
> 
> > Looks like there's work for you if you want ;)
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros=2023-04-07%2018%3A52%3A13
> 
> > But IMO fixing sepgsql can easily wait till tomorrow.
> 
> I can deal with that one -- it's a bit annoying to work with sepgsql
> if you're not on a Red Hat platform.

Indeed. I tried to get them running a while back, to enable the tests with
meson, without lot of success. Then I realized that they're also not wired up
in make... ;)


> After quickly eyeing the diffs, I'm just going to take the new output
> as good.  I'm not surprised that there are additional output messages
> given the additional catalog entries this made.  I *am* a bit surprised
> that some messages seem to have disappeared --- are there places where
> this resulted in fewer catalog accesses than before?  Nonetheless,
> there's no good reason to assume this test is exposing any bugs.

I wonder if the issue is that the new paths miss a hook invocation.

@@ -160,11 +160,7 @@
 ALTER TABLE regtest_table ALTER b SET DEFAULT 'XYZ';-- not supported yet
 ALTER TABLE regtest_table ALTER b DROP DEFAULT; -- not supported yet
 ALTER TABLE regtest_table ALTER b SET NOT NULL;
-LOG:  SELinux: allowed { setattr } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column 
name="regtest_schema_2.regtest_table.b" permissive=0
-LOG:  SELinux: allowed { setattr } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column 
name="regtest_schema.regtest_table_2.b" permissive=0
 ALTER TABLE regtest_table ALTER b DROP NOT NULL;
-LOG:  SELinux: allowed { setattr } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column 
name="regtest_schema_2.regtest_table.b" permissive=0
-LOG:  SELinux: allowed { setattr } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column 
name="regtest_schema.regtest_table_2.b" permissive=0
 ALTER TABLE regtest_table ALTER b SET STATISTICS -1;
 LOG:  SELinux: allowed { setattr } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column 
name="regtest_schema_2.regtest_table.b" permissive=0
 LOG:  SELinux: allowed { setattr } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column 
name="regtest_schema.regtest_table_2.b" permissive=0

The 'not supported yet' cases don't emit messages. Previously SET NOT NULL
wasn't among that set, but seemingly it now is.

Greetings,

Andres Freund

Re: cataloguing NOT NULL constraints

2023-04-07 Thread Tom Lane

Andres Freund  writes:
> On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote:
>> Ah, cool, no worries.  I would have stopped indeed, but I had to stay
>> around in case of any test failures.

> Looks like there's work for you if you want ;)
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros=2023-04-07%2018%3A52%3A13

> But IMO fixing sepgsql can easily wait till tomorrow.

I can deal with that one -- it's a bit annoying to work with sepgsql
if you're not on a Red Hat platform.

After quickly eyeing the diffs, I'm just going to take the new output
as good.  I'm not surprised that there are additional output messages
given the additional catalog entries this made.  I *am* a bit surprised
that some messages seem to have disappeared --- are there places where
this resulted in fewer catalog accesses than before?  Nonetheless,
there's no good reason to assume this test is exposing any bugs.

regards, tom lane

Re: cataloguing NOT NULL constraints

Hi,

On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote:
> On 2023-Apr-07, Andres Freund wrote:
> 
> > I just pushed a fix - sorry, I thought you might have stopped working for 
> > the
> > day and CI finished with the modification a few seconds before your email
> > arrived...
> 
> Ah, cool, no worries.  I would have stopped indeed, but I had to stay
> around in case of any test failures.

Looks like there's work for you if you want ;)
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros=2023-04-07%2018%3A52%3A13

But IMO fixing sepgsql can easily wait till tomorrow.

Greetings,

Andres Freund

Re: cataloguing NOT NULL constraints

2023-04-07 Thread Alvaro Herrera

On 2023-Apr-07, Andres Freund wrote:

> I just pushed a fix - sorry, I thought you might have stopped working for the
> day and CI finished with the modification a few seconds before your email
> arrived...

Ah, cool, no worries.  I would have stopped indeed, but I had to stay
around in case of any test failures.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo." (Augusto Pinochet a una corte de justicia)

Re: cataloguing NOT NULL constraints

Hi,

On 2023-04-07 23:00:01 +0200, Alvaro Herrera wrote:
> On 2023-Apr-07, Andres Freund wrote:
> 
> > src/test/regress/sql/triggers.sql
> > 2127:create table child partition of parent for values in ('AAA');
> > 2266:create table child () inherits (parent);
> > 2759:create table child () inherits (parent);
> > 
> > The inherit.sql part is new.
> 
> Yeah.
> 
> > I'll see how hard it is to fix.
> 
> Running the tests for it now -- it's a short fix.

I just pushed a fix - sorry, I thought you might have stopped working for the
day and CI finished with the modification a few seconds before your email
arrived...

Greetings,

Andres Freund

Re: cataloguing NOT NULL constraints

2023-04-07 Thread Alvaro Herrera

On 2023-Apr-07, Andres Freund wrote:

> src/test/regress/sql/triggers.sql
> 2127:create table child partition of parent for values in ('AAA');
> 2266:create table child () inherits (parent);
> 2759:create table child () inherits (parent);
> 
> The inherit.sql part is new.

Yeah.

> I'll see how hard it is to fix.

Running the tests for it now -- it's a short fix.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Learn about compilers. Then everything looks like either a compiler or
a database, and now you have two problems but one of them is fun."
https://twitter.com/thingskatedid/status/1456027786158776329

Re: cataloguing NOT NULL constraints

Hi,

On 2023-04-07 13:38:43 -0700, Andres Freund wrote:
> I suspect there's a naming conflict between tests in different groups.

Yep:

test: create_aggregate create_function_sql create_cast constraints triggers 
select inherit typed_table vacuum drop_if_exists updatable_views roleattributes 
create_am hash_func errors infinite_recurse

src/test/regress/sql/inherit.sql
851:create table child(f1 int not null, f2 text not null) 
inherits(inh_parent_1, inh_parent_2);

src/test/regress/sql/triggers.sql
2127:create table child partition of parent for values in ('AAA');
2266:create table child () inherits (parent);
2759:create table child () inherits (parent);

The inherit.sql part is new.

I'll see how hard it is to fix.

Greetings,

Andres Freund

Re: cataloguing NOT NULL constraints