cataloguing NOT NULL constraints

References to past discussions and patches:

/messages/by-id/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com
/messages/by-id/1343682669-sup-2532@alvh.no-ip.org
/messages/by-id/20160109030002.GA671800@alvherre.pgsql

I started this time around from the newest of my patches in those
threads, but the implementation has changed considerably from what's
there.

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

I started this time around from the newest of my patches in those
threads, but the implementation has changed considerably from what's
there.

I don´t know exactly what will be the scope of this process you're working
on, but there is a gap on foreign key constraint too.
It is possible to have wrong values on a FK constraint if you disable
checking of it with session_replication_role or disable trigger all
I know you can create that constraint with "not valid" and it'll be checked
when turned on. But if I just forgot that ...
So would be good to have validate constraints which checks, even if it's
already valid

drop table if exists tb_pk cascade;create table tb_pk(key integer not null
primary key);
drop table if exists tb_fk cascade;create table tb_fk(fk_key integer);
alter table tb_fk add constraint fk_pk foreign key (fk_key) references
tb_pk (key);
insert into tb_pk values(1);
alter table tb_fk disable trigger all; --can be with
session_replication_role too.
insert into tb_fk values(5); --wrong values on that table

Then, you could check

alter table tb_fk validate constraint fk_pk
or
alter table tb_fk validate all constraints

On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote:

I've been working on having NOT NULL constraints have pg_constraint
rows.

Everything is working now.  Some things are a bit weird, and I would
like opinions on them:

1. In my implementation, you can have more than one NOT NULL
   pg_constraint row for a column.  What should happen if the user does
   ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL;
   ?  Currently it throws an error about the ambiguity (ie. which
   constraint to drop).

I'd say that is a good solution, particularly if there is a hint to drop
the constraint instead, similar to when you try to drop an index that
implements a constraint.

   Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull'
   bit is lost when the last one such constraint goes away.

Wouldn't it be the correct solution to set "attnotnumm" to FALSE only
when the last NOT NULL constraint is dropped?

2. If a table has a primary key, and a table is created that inherits
   from it, then the child has its column(s) marked attnotnull but there
   is no pg_constraint row for that.  This is not okay.  But what should
   happen?

   1. a CHECK(col IS NOT NULL) constraint is created for each column
   2. a PRIMARY KEY () constraint is created

I think it would be best to create a primary key constraint on the
partition.

Yours,
Laurenz Albe

On 2022-Aug-18, Laurenz Albe wrote:

On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote:

1. In my implementation, you can have more than one NOT NULL
   pg_constraint row for a column.  What should happen if the user does
   ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL;
   ?  Currently it throws an error about the ambiguity (ie. which
   constraint to drop).

I'd say that is a good solution, particularly if there is a hint to drop
the constraint instead, similar to when you try to drop an index that
implements a constraint.

Ah, I didn't think about the hint. I'll add that, thanks.

   Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull'
   bit is lost when the last one such constraint goes away.

Wouldn't it be the correct solution to set "attnotnumm" to FALSE only
when the last NOT NULL constraint is dropped?

... when the last NOT NULL or PRIMARY KEY constraint is dropped. We
have to keep attnotnull set when a PK exists even if there's no specific
NOT NULL constraint.

2. If a table has a primary key, and a table is created that inherits
   from it, then the child has its column(s) marked attnotnull but there
   is no pg_constraint row for that.  This is not okay.  But what should
   happen?

   1. a CHECK(col IS NOT NULL) constraint is created for each column
   2. a PRIMARY KEY () constraint is created

I think it would be best to create a primary key constraint on the
partition.

Sorry, I wasn't specific enough. This applies to legacy inheritance
only; partitioning has its own solution (as you say: the PK constraint
exists), but legacy inheritance works differently. Creating a PK in
children tables is not feasible (because unicity cannot be maintained),
but creating a CHECK (NOT NULL) constraint is possible.

I think a PRIMARY KEY should not be allowed to exist in an inheritance
parent, precisely because of this problem, but it seems too late to add
that restriction now. This behavior is absurd, but longstanding:

55432 16devel 1787364=# create table parent (a int primary key);
CREATE TABLE
55432 16devel 1787364=# create table child () inherits (parent);
CREATE TABLE
55432 16devel 1787364=# insert into parent values (1);
INSERT 0 1
55432 16devel 1787364=# insert into child values (1);
INSERT 0 1
55432 16devel 1787364=# select * from parent;
a
───
1
1
(2 filas)

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"But static content is just dynamic content that isn't moving!"
http://smylers.hates-software.com/2007/08/15/fe244d0c.html

On Thu, 2022-08-18 at 11:04 +0200, Alvaro Herrera wrote:

On 2022-Aug-18, Laurenz Albe wrote:

On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote:

   Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull'
   bit is lost when the last one such constraint goes away.

Wouldn't it be the correct solution to set "attnotnumm" to FALSE only
when the last NOT NULL constraint is dropped?

... when the last NOT NULL or PRIMARY KEY constraint is dropped.  We
have to keep attnotnull set when a PK exists even if there's no specific
NOT NULL constraint.

Of course, I forgot that.
I hope that is not too hard to implement.

2. If a table has a primary key, and a table is created that inherits
   from it, then the child has its column(s) marked attnotnull but there
   is no pg_constraint row for that.  This is not okay.  But what should
   happen?

   1. a CHECK(col IS NOT NULL) constraint is created for each column
   2. a PRIMARY KEY () constraint is created

I think it would be best to create a primary key constraint on the
partition.

Sorry, I wasn't specific enough.  This applies to legacy inheritance
only; partitioning has its own solution (as you say: the PK constraint
exists), but legacy inheritance works differently.  Creating a PK in
children tables is not feasible (because unicity cannot be maintained),
but creating a CHECK (NOT NULL) constraint is possible.

I think a PRIMARY KEY should not be allowed to exist in an inheritance
parent, precisely because of this problem, but it seems too late to add
that restriction now.  This behavior is absurd, but longstanding:

My mistake; you clearly said "inherits".

Since such an inheritance child currently does not have a primary key, you
can insert duplicates. So automatically adding a NUT NULL constraint on the
inheritance child seems the only solution that does not break backwards
compatibility. pg_upgrade would have to be able to cope with that.

Forcing a primary key constraint on the inheritance child could present an
upgrade problem. Even if that is probably a rare and strange case, I don't
think we should risk that. Moreover, if we force a primary key on the
inheritance child, using ALTER TABLE ... INHERIT might have to create a
unique index on the table, which can be cumbersome if the table is large.

So I think a NOT NULL constraint is the least evil.

Yours,
Laurenz Albe

On Thu, Aug 18, 2022 at 6:04 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Aug-18, Laurenz Albe wrote:

On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote:

2. If a table has a primary key, and a table is created that inherits
from it, then the child has its column(s) marked attnotnull but there
is no pg_constraint row for that. This is not okay. But what should
happen?

1. a CHECK(col IS NOT NULL) constraint is created for each column
2. a PRIMARY KEY () constraint is created

I think it would be best to create a primary key constraint on the
partition.

Sorry, I wasn't specific enough. This applies to legacy inheritance
only; partitioning has its own solution (as you say: the PK constraint
exists), but legacy inheritance works differently. Creating a PK in
children tables is not feasible (because unicity cannot be maintained),
but creating a CHECK (NOT NULL) constraint is possible.

Yeah, I think it makes sense to think of the NOT NULL constraints on
their own in this case, without worrying about the PK constraint that
created them in the first place.

BTW, maybe you are aware, but the legacy inheritance implementation is
not very consistent about wanting to maintain the same NULLness for a
given column in all members of the inheritance tree. For example, it
allows one to alter the NULLness of an inherited column:

create table p (a int not null);
create table c (a int) inherits (p);
\d c
Table "public.c"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | not null |
Inherits: p

alter table c alter a drop not null ;
ALTER TABLE
\d c
Table "public.c"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
Inherits: p

Contrast that with the partitioning implementation:

create table pp (a int not null) partition by list (a);
create table cc partition of pp default;
\d cc
Table "public.cc"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | not null |
Partition of: pp DEFAULT

alter table cc alter a drop not null ;
ERROR: column "a" is marked NOT NULL in parent table

IIRC, I had tried to propose implementing the same behavior for legacy
inheritance back in the day, but maybe we left it alone for not
breaking compatibility.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-Aug-22, Amit Langote wrote:

Yeah, I think it makes sense to think of the NOT NULL constraints on
their own in this case, without worrying about the PK constraint that
created them in the first place.

Cool, that's enough votes that I'm comfortable implementing things that
way.

BTW, maybe you are aware, but the legacy inheritance implementation is
not very consistent about wanting to maintain the same NULLness for a
given column in all members of the inheritance tree. For example, it
allows one to alter the NULLness of an inherited column:

Right ... I think what gives this patch most of its complexity is the
number of odd, inconsistent cases that have to preserve historical
behavior. Luckily I think this particular behavior is easy to
implement.

IIRC, I had tried to propose implementing the same behavior for legacy
inheritance back in the day, but maybe we left it alone for not
breaking compatibility.

Yeah, that wouldn't be surprising.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"The problem with the facetime model is not just that it's demoralizing, but
that the people pretending to work interrupt the ones actually working."
(Paul Graham)

So I was wrong in thinking that "this case was simple to implement" as I
replied upthread. Doing that actually required me to rewrite large
parts of the patch. I think it ended up being a good thing, because in
hindsight the approach I was using was somewhat bogus anyway, and the
current one should be better. Please find it attached.

There are still a few problems, sadly. Most notably, I ran out of time
trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode.
I have to review that again, but I think it'll need a deeper rethink of
how we pg_upgrade inherited constraints. So the pg_upgrade tests are
known to fail. I'm not aware of any other tests failing, but I'm sure
the cfbot will prove me wrong.

I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull,
to allow setting pg_attribute.attnotnull without adding a CHECK
constraint (only used internally). I would like to find a better way to
go about this, so I may remove it again, therefore it's not fully
implemented.

There are *many* changed regress expect files and I didn't carefully vet
all of them. Mostly it's the addition of CHECK constraints in the
footers of many \d listings and stuff like that. At a quick glance they
appear valid, but I need to review them more carefully still.

We've had pg_constraint.conparentid for a while now, but for some
constraints we continue to use conislocal/coninhcount. I think we
should get rid of that and rely on conparentid completely.

An easily fixed issue is that of constraint naming.
ChooseConstraintName has an argument for passing known constraint names,
but this patch doesn't use it and it must.

One issue that I don't currently know how to fix, is the fact that we
need to know whether a column is a row type or not (because they need a
different null test). At table creation time that's easy to know,
because we have the descriptor already built by the time we add the
constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT
then we don't.

Some ancient code comments suggest that allowing a child table's NOT
NULL constraint acquired from parent shouldn't be independently
droppable. This patch doesn't change that, but it's easy to do if we
decide to. However, that'd be a compatibility break, so I'd rather not
do it in the same patch that introduces the feature.

Overall, there's a lot more work required to get this to a good shape.
That said, I think it's the right direction.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

On Wed, Aug 31, 2022 at 3:19 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

So I was wrong in thinking that "this case was simple to implement" as I
replied upthread. Doing that actually required me to rewrite large
parts of the patch. I think it ended up being a good thing, because in
hindsight the approach I was using was somewhat bogus anyway, and the
current one should be better. Please find it attached.

There are still a few problems, sadly. Most notably, I ran out of time
trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode.
I have to review that again, but I think it'll need a deeper rethink of
how we pg_upgrade inherited constraints. So the pg_upgrade tests are
known to fail. I'm not aware of any other tests failing, but I'm sure
the cfbot will prove me wrong.

I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull,
to allow setting pg_attribute.attnotnull without adding a CHECK
constraint (only used internally). I would like to find a better way to
go about this, so I may remove it again, therefore it's not fully
implemented.

There are *many* changed regress expect files and I didn't carefully vet
all of them. Mostly it's the addition of CHECK constraints in the
footers of many \d listings and stuff like that. At a quick glance they
appear valid, but I need to review them more carefully still.

We've had pg_constraint.conparentid for a while now, but for some
constraints we continue to use conislocal/coninhcount. I think we
should get rid of that and rely on conparentid completely.

An easily fixed issue is that of constraint naming.
ChooseConstraintName has an argument for passing known constraint names,
but this patch doesn't use it and it must.

One issue that I don't currently know how to fix, is the fact that we
need to know whether a column is a row type or not (because they need a
different null test). At table creation time that's easy to know,
because we have the descriptor already built by the time we add the
constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT
then we don't.

Some ancient code comments suggest that allowing a child table's NOT
NULL constraint acquired from parent shouldn't be independently
droppable. This patch doesn't change that, but it's easy to do if we
decide to. However, that'd be a compatibility break, so I'd rather not
do it in the same patch that introduces the feature.

Overall, there's a lot more work required to get this to a good shape.
That said, I think it's the right direction.

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el
sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

Hi,
For findNotNullConstraintAttnum():

+ if (multiple == NULL)
+ break;

Shouldn't `pfree(arr)` be called before breaking ?

+static Constraint *makeNNCheckConstraint(Oid nspid, char *constraint_name,

You used `NN` because there is method makeCheckNotNullConstraint, right ?
I think it would be better to expand `NN` so that its meaning is easy to
understand.

Cheers

On Wed, Aug 31, 2022 at 4:08 PM Zhihong Yu <zyu@yugabyte.com> wrote:

On Wed, Aug 31, 2022 at 3:19 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

So I was wrong in thinking that "this case was simple to implement" as I
replied upthread. Doing that actually required me to rewrite large
parts of the patch. I think it ended up being a good thing, because in
hindsight the approach I was using was somewhat bogus anyway, and the
current one should be better. Please find it attached.

There are still a few problems, sadly. Most notably, I ran out of time
trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode.
I have to review that again, but I think it'll need a deeper rethink of
how we pg_upgrade inherited constraints. So the pg_upgrade tests are
known to fail. I'm not aware of any other tests failing, but I'm sure
the cfbot will prove me wrong.

I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull,
to allow setting pg_attribute.attnotnull without adding a CHECK
constraint (only used internally). I would like to find a better way to
go about this, so I may remove it again, therefore it's not fully
implemented.

There are *many* changed regress expect files and I didn't carefully vet
all of them. Mostly it's the addition of CHECK constraints in the
footers of many \d listings and stuff like that. At a quick glance they
appear valid, but I need to review them more carefully still.

We've had pg_constraint.conparentid for a while now, but for some
constraints we continue to use conislocal/coninhcount. I think we
should get rid of that and rely on conparentid completely.

An easily fixed issue is that of constraint naming.
ChooseConstraintName has an argument for passing known constraint names,
but this patch doesn't use it and it must.

One issue that I don't currently know how to fix, is the fact that we
need to know whether a column is a row type or not (because they need a
different null test). At table creation time that's easy to know,
because we have the descriptor already built by the time we add the
constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT
then we don't.

Some ancient code comments suggest that allowing a child table's NOT
NULL constraint acquired from parent shouldn't be independently
droppable. This patch doesn't change that, but it's easy to do if we
decide to. However, that'd be a compatibility break, so I'd rather not
do it in the same patch that introduces the feature.

Overall, there's a lot more work required to get this to a good shape.
That said, I think it's the right direction.

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el
sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

Hi,
For findNotNullConstraintAttnum():

+ if (multiple == NULL)
+ break;

Shouldn't `pfree(arr)` be called before breaking ?

+static Constraint *makeNNCheckConstraint(Oid nspid, char *constraint_name,

You used `NN` because there is method makeCheckNotNullConstraint, right ?
I think it would be better to expand `NN` so that its meaning is easy to
understand.

Cheers

Hi,
For tryExtractNotNullFromNode , in the block for `if (rel == NULL)`:

+ return false;

I think you meant returning NULL since false is for boolean.

Cheers

There were a lot more problems in that submission than I at first
realized, and I had to rewrite a lot of code in order to fix them. I
have fixed all the user-visible problems I found in this version, and
reviewed the tests results more carefully so I am now more confident
that behaviourally it's doing the right thing; but

1. the pg_upgrade test problem is still unaddressed,
2. I haven't verified that catalog contents is correct, especially
regarding dependencies,
3. there are way too many XXX and FIXME comments sprinkled everywhere.

I'm sure a couple of these XXX comments can be left for later work, and
there's a few that should be dealt with by merely removing them; but the
others (and all FIXMEs) represent pending work.

Also, I'm not at all happy about having this new ConstraintNotNull
artificial node there; perhaps this can be solved by using a regular
Constraint with some new flag, or maybe it will even work without any
extra flags by the fact that the node appears where it appears. Anyway,
requires investigation. Also, the AT_SetAttNotNull continues to irk me.

test_ddl_deparse is also unhappy. This is probably an easy fix;
apparently, ATExecDropConstraint has been doing things wrong forever.

Anyway, here's version 2 of this, with apologies for those who spent
time reviewing version 1 with all its brokenness.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"On the other flipper, one wrong move and we're Fatal Exceptions"
(T.U.X.: Term Unit X - http://www.thelinuxreview.com/TUX/)

Hi,
w.r.t. the while loop in findNotNullConstraintAttnum():

+ if (multiple == NULL)
+ break;

I think `pfree(arr)` should be called before breaking.

+       if (constraint->cooked_expr != NULL)
+           return
tryExtractNotNullFromNode(stringToNode(constraint->cooked_expr), rel);
+       else
+           return tryExtractNotNullFromNode(constraint->raw_expr, rel);

nit: the `else` keyword is not needed.

+   if (isnull)
+       elog(ERROR, "null conbin for constraint %u", conForm->oid);

It would be better to expand `conbin` so that the user can better
understand the error.

Cheers

Hi Alvaro,

On Sat, Sep 10, 2022 at 2:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

There were a lot more problems in that submission than I at first
realized, and I had to rewrite a lot of code in order to fix them. I
have fixed all the user-visible problems I found in this version, and
reviewed the tests results more carefully so I am now more confident
that behaviourally it's doing the right thing; but

1. the pg_upgrade test problem is still unaddressed,
2. I haven't verified that catalog contents is correct, especially
regarding dependencies,
3. there are way too many XXX and FIXME comments sprinkled everywhere.

I'm sure a couple of these XXX comments can be left for later work, and
there's a few that should be dealt with by merely removing them; but the
others (and all FIXMEs) represent pending work.

Also, I'm not at all happy about having this new ConstraintNotNull
artificial node there; perhaps this can be solved by using a regular
Constraint with some new flag, or maybe it will even work without any
extra flags by the fact that the node appears where it appears. Anyway,
requires investigation. Also, the AT_SetAttNotNull continues to irk me.

test_ddl_deparse is also unhappy. This is probably an easy fix;
apparently, ATExecDropConstraint has been doing things wrong forever.

Anyway, here's version 2 of this, with apologies for those who spent
time reviewing version 1 with all its brokenness.

I have been testing this with the intention of understanding how you
made this work with inheritance. While doing so with the previous
version, I ran into an existing issue (bug?) that I reported at [1]/messages/by-id/CA+HiwqFggpjAvsVqNV06HUF6CUrU0Vo3pLgGWCViGbPkzTiofg@mail.gmail.com.

I ran into another while testing version 2 that I think has to do with
this patch. So this happens:

-- regular inheritance
create table foo (a int not null);
create table foo1 (a int not null);
alter table foo1 inherit foo;
alter table foo alter a drop not null ;
ERROR: constraint "foo_a_not_null" of relation "foo1" does not exist

-- partitioning
create table parted (a int not null) partition by list (a);
create table part1 (a int not null);
alter table parted attach partition part1 default;
alter table parted alter a drop not null;
ERROR: constraint "parted_a_not_null" of relation "part1" does not exist

In both of these cases, MergeConstraintsIntoExisting(), called by
CreateInheritance() when attaching the child to the parent, marks the
child's NOT NULL check constraint as the child constraint of the
corresponding constraint in parent, which seems fine and necessary.

However, ATExecDropConstraint_internal(), the new function called by
ATExecDropNotNull(), doesn't seem to recognize when recursing to the
child tables that a child's copy NOT NULL check constraint attached to
the parent's may have a different name, so scanning pg_constraint with
the parent's name is what gives the above error. I wonder if it
wouldn't be better for ATExecDropNotNull() to handle its own recursion
rather than delegating it to the DropConstraint()?

The same error does not occur when the NOT NULL constraint is added to
parent after-the-fact and thus recursively to the children:

-- regular inheritance
create table foo (a int);
create table foo1 (a int not null) inherits (foo);
alter table foo alter a set not null;
alter table foo alter a drop not null ;
ALTER TABLE

-- partitioning
create table parted (a int) partition by list (a);
create table part1 partition of parted (a not null) default;
alter table parted alter a set not null;
alter table parted alter a drop not null;
ALTER TABLE

And the reason for that seems a bit accidental, because
MergeWithExistingConstraint(), called by AddRelationNewConstraints()
when recursively adding the NOT NULL check constraint to a child, does
not have the code to find the child's already existing constraint that
matches with it. So, in this case, we get a copy of the parent's
constraint with the same name in the child. There is a line in the
header comments of both MergeWithExistingConstraint() and
MergeConstraintsIntoExisting() asking to keep their code in sync, so
maybe the patch missed adding the new NOT NULL check constraint logic
to the former?

Also, it seems that the inheritance recursion for SET NOT NULL is now
occurring both in the prep phase and exec phase due to the following
new code added to ATExecSetNotNull():

@@ -7485,6 +7653,50 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
    InvokeObjectPostAlterHook(RelationRelationId,
                              RelationGetRelid(rel), attnum);
 ...
+       /* See if there's one already, and skip this if so. */
+       constr = findNotNullConstraintAttnum(rel, attnum, NULL);
+       if (constr && direct)
+           heap_freetuple(constr); /* nothing to do */
+       else
+       {
+           Constraint *newconstr;
+           ObjectAddress addr;
+           List       *children;
+           List       *already_done_rels;
+
+           newconstr = makeCheckNotNullConstraint(rel->rd_rel->relnamespace,
+                                                  constrname,
+
NameStr(rel->rd_rel->relname),
+                                                  colName,
+                                                  false, /* XXX is_row */
+                                                  InvalidOid);
+
+           addr = ATAddCheckConstraint_internal(wqueue, tab, rel, newconstr,
+                                                false, false, false, lockmode);
+           already_done_rels = list_make1_oid(RelationGetRelid(rel));
+
+           /* and recurse into children, if there are any */
+           children =
find_inheritance_children(RelationGetRelid(rel), lockmode);
+           ATAddCheckConstraint_recurse(wqueue, children, newconstr,

It seems harmless because ATExecSetNotNull() set up to run on the
children by the prep phase becomes a no-op due to the work done by the
above code, but maybe we should keep one or the other.

Regarding the following bit:

-   /* If rel is partition, shouldn't drop NOT NULL if parent has the same */
+   /*
+    * If rel is partition, shouldn't drop NOT NULL if parent has the same.
+    * XXX is this consideration still valid?  Can we get rid of this by
+    * changing the type of dependency between the two constraints instead?
+    */
    if (rel->rd_rel->relispartition)
    {
        Oid         parentId =
get_partition_parent(RelationGetRelid(rel), false);

Yes, it seems we can now prevent dropping a partition's NOT NULL
constraint by seeing that it is inherited, so no need for this block
which was written when the NOT NULL constraints didn't have the
inherited marking.

BTW, have you thought about making DROP NOT NULL command emit a
different error message than what one gets now:

create table foo (a int);
create table foo1 (a int) inherits (foo);
alter table foo alter a set not null;
alter table foo1 alter a drop not null ;
ERROR: cannot drop inherited constraint "foo_a_not_null" of relation "foo1"

Like, say:

ERROR: cannot drop an inherited NOT NULL constraint

Maybe you did and thought that it's OK for it to spell out the
internally generated constraint name, because we already require users
to know that they exist, say to drop it using DROP CONSTRAINT.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

[1]: /messages/by-id/CA+HiwqFggpjAvsVqNV06HUF6CUrU0Vo3pLgGWCViGbPkzTiofg@mail.gmail.com

On 09.09.22 19:58, Alvaro Herrera wrote:

There were a lot more problems in that submission than I at first
realized, and I had to rewrite a lot of code in order to fix them. I
have fixed all the user-visible problems I found in this version, and
reviewed the tests results more carefully so I am now more confident
that behaviourally it's doing the right thing; but

Reading through the SQL standard again, I think this patch goes a bit
too far in folding NOT NULL and CHECK constraints together. The spec
says that you need to remember whether a column was defined as NOT NULL,
and that the commands DROP NOT NULL and SET NOT NULL only affect
constraints defined in that way. In this implementation, a constraint
defined as NOT NULL is converted to a CHECK (x IS NOT NULL) constraint
and the original definition is forgotten.

Besides that, I think that users are not going to like that pg_dump
rewrites their NOT NULL constraints into CHECK table constraints.

I suspect that this needs a separate contype for NOT NULL constraints
that is separate from CONSTRAINT_CHECK.

On 2022-Sep-14, Peter Eisentraut wrote:

Reading through the SQL standard again, I think this patch goes a bit too
far in folding NOT NULL and CHECK constraints together. The spec says that
you need to remember whether a column was defined as NOT NULL, and that the
commands DROP NOT NULL and SET NOT NULL only affect constraints defined in
that way. In this implementation, a constraint defined as NOT NULL is
converted to a CHECK (x IS NOT NULL) constraint and the original definition
is forgotten.

Hmm, I don't read it the same way. Reading SQL:2016, they talk about a
nullability characteristic (/known not nullable/ or /possibly
nullable/):

: 4.13 Columns, fields, and attributes
: [...]
: Every column has a nullability characteristic that indicates whether the
: value from that column can be the null value. A nullability characteristic
: is either known not nullable or possibly nullable.
: Let C be a column of a base table T. C is known not nullable if and only
: if at least one of the following is true:
: — There exists at least one constraint NNC that is enforced and not
: deferrable and that simply contains a <search condition> that is a
: <boolean value expression> that is a readily-known-not-null condition for C.
: [other possibilities]

then in the same section they explain that this is derived from a
table constraint:

: A column C is described by a column descriptor. A column descriptor
: includes:
: [...]
: — If C is a column of a base table, then an indication of whether it is
: defined as NOT NULL and, if so, the constraint name of the associated
: table constraint definition.

[aside: note that elsewhere (<boolean value expression>), they define
"readily-known-not-null" in Syntax Rule 3), of 6.39 <boolean value
expression>:

: 3) Let X denote either a column C or the <key word> VALUE. Given a
: <boolean value expression> BVE and X, the notion “BVE is a
: readily-known-not-null condition for X” is defined as follows.
: Case:
: a) If BVE is a <predicate> of the form “RVE IS NOT NULL”, where RVE is a
: <row value predicand> that is a <row value constructor predicand> that
: simply contains a <common value expression>, <boolean predicand>, or
: <row value constructor element> that is a <column reference> that
: references C, then BVE is a readily-known-not-null condition for C.
: b) If BVE is the <predicate> “VALUE IS NOT NULL”, then BVE is a
: readily-known-not-null condition for VALUE.
: c) Otherwise, BVE is not a readily-known-not-null condition for X.
edisa]

Later, <column definition> says literally that specifying NOT NULL in a
column is equivalent to the CHECK (.. IS NOT NULL) table constraint:

: 11.4 <column definition>
:
: Syntax Rules,
: 17) If a <column constraint definition> is specified, then let CND be
: the <constraint name definition> if one is specified and let CND be the
: zero-length character character string otherwise; let CA be the
: <constraint characteristics> if specified and let CA be the zero-length
: character string otherwise. The <column constraint definition> is
: equivalent to a <table constraint definition> as follows.
:
: Case:
:
: a) If a <column constraint definition> is specified that contains the
: <column constraint> NOT NULL, then it is equivalent to the following
: <table constraint definition>:
: CND CHECK ( C IS NOT NULL ) CA

In my reading of it, it doesn't follow that you have to remember whether
the table constraint was created by saying explicitly by doing CHECK (c
IS NOT NULL) or as a plain NOT NULL column constraint. The idea of
being able to do DROP NOT NULL when only a constraint defined as CHECK
(c IS NOT NULL) exists seems to follow from there; and also that you can
use DROP CONSTRAINT to remove one added via plain NOT NULL; and that
both these operations change the nullability characteristic of the
column. This is made more explicit by the fact that they do state that
the nullability characteristic can *not* be "destroyed" for other types
of constraints, in 11.26 <drop table constraint definition>, Syntax Rule
11)

: 11) Destruction of TC shall not cause the nullability characteristic of
: any of the following columns of T to change from known not nullable to
: possibly nullable:
:
: a) A column that is a constituent of the primary key of T, if any.
: b) The system-time period start column, if any.
: c) The system-time period end column, if any.
: d) The identity column, if any.

then General Rule 7) explains that this does indeed happen for columns
declared to have some sort of NOT NULL constraint, without saying
exactly how was that constraint defined:

: 7) If TC 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.

Besides that, I think that users are not going to like that pg_dump rewrites
their NOT NULL constraints into CHECK table constraints.

This is a good point, but we could get around it by decreeing that
pg_dump dumps the NOT NULL in the old way if the name is not changed
from whatever would be generated normally. This would require some
games to remove the CHECK one; and it would also mean that partitions
would not use the same constraint as the parent, but rather it'd have to
generate a new constraint name that uses its own table name, rather than
the parent's.

(This makes me wonder what should happen if you rename a table: should
we go around and rename all the automatically-named constraints as well?
Probably not, but this may annoy people that creates table under one
name, then rename them into their final places afterwards. pg_dump may
behave funny for those. We can tackle that later, if ever. But
consider that moving the table across schemas might cause even weirder
problems, since the standard says constraint names must not conflict
within a schema ...)

I suspect that this needs a separate contype for NOT NULL constraints that
is separate from CONSTRAINT_CHECK.

Maybe it is possible to read this in the way you propose, but I think
that interpretation is strictly less useful than the one I propose.
Also, see this reply from Tom to Vitaly Burovoy who was proposing
something that seems to derivate from this interpretation:
/messages/by-id/17684.1462339177@sss.pgh.pa.us

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Hay dos momentos en la vida de un hombre en los que no debería
especular: cuando puede permitírselo y cuando no puede" (Mark Twain)

On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint
printed by psql: (this is a bit more noisy that previously and it
changes a lot of regression tests output).

55489 16devel 1776237=# create table tab (a int not null);
CREATE TABLE
55489 16devel 1776237=# \d tab
Tabla «public.tab»
Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión
─────────┼─────────┼──────────────┼──────────┼─────────────
a │ integer │ │ not null │
Restricciones CHECK:
"tab_a_not_null" CHECK (a IS NOT NULL)

In a table with many columns, most of which are NOT NULL, this is
going to produce a ton of clutter. I don't like that.

I'm not sure what a good alternative would be, though.

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

On Mon, 19 Sept 2022 at 09:32, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint
printed by psql: (this is a bit more noisy that previously and it
changes a lot of regression tests output).

55489 16devel 1776237=# create table tab (a int not null);
CREATE TABLE
55489 16devel 1776237=# \d tab
Tabla «public.tab»
Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión
─────────┼─────────┼──────────────┼──────────┼─────────────
a │ integer │ │ not null │
Restricciones CHECK:
"tab_a_not_null" CHECK (a IS NOT NULL)

In a table with many columns, most of which are NOT NULL, this is
going to produce a ton of clutter. I don't like that.

I'm not sure what a good alternative would be, though.

I thought I saw some discussion about the SQL standard saying that there is
a difference between putting NOT NULL in a column definition, and CHECK
(column_name NOT NULL). So if we're going to take this seriously, I think
that means there needs to be a field in pg_constraint which identifies
whether a constraint is a "real" one created explicitly as a constraint, or
if it is just one created because a field is marked NOT NULL.

If this is correct, the answer is easy: don't show constraints that are
there only because of a NOT NULL in the \d or \d+ listings. I certainly
don't want to see that clutter and I'm having trouble seeing why anybody
else would want to see it either; the information is already there in the
"Nullable" column of the field listing.

The error message for a duplicate constraint name when creating a
constraint needs however to be very clear that the conflict is with a NOT
NULL constraint and which one, since I'm proposing leaving those ones off
the visible listing, and it would be very bad for somebody to get
"duplicate name" and then be unable to see the conflicting entry.

Isaac Morland <isaac.morland@gmail.com> writes:

I thought I saw some discussion about the SQL standard saying that there is
a difference between putting NOT NULL in a column definition, and CHECK
(column_name NOT NULL). So if we're going to take this seriously, I think
that means there needs to be a field in pg_constraint which identifies
whether a constraint is a "real" one created explicitly as a constraint, or
if it is just one created because a field is marked NOT NULL.

If we're going to go that way, I think that we should take the further
step of making not-null constraints be their own contype rather than
an artificially generated CHECK. The bloat in pg_constraint from CHECK
expressions made this way seems like an additional reason not to like
doing it like that.

regards, tom lane

On Mon, 19 Sept 2022 at 15:32, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint
printed by psql: (this is a bit more noisy that previously and it
changes a lot of regression tests output).

55489 16devel 1776237=# create table tab (a int not null);
CREATE TABLE
55489 16devel 1776237=# \d tab
Tabla «public.tab»
Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión
─────────┼─────────┼──────────────┼──────────┼─────────────
a │ integer │ │ not null │
Restricciones CHECK:
"tab_a_not_null" CHECK (a IS NOT NULL)

In a table with many columns, most of which are NOT NULL, this is
going to produce a ton of clutter. I don't like that.

I'm not sure what a good alternative would be, though.

I'm not sure on the 'good' part of this alternative, but we could go
with a single row-based IS NOT NULL to reduce such clutter, utilizing
the `ROW() IS NOT NULL` requirement of a row only matching IS NOT NULL
when all attributes are also IS NOT NULL:

Check constraints:
"tab_notnull_check" CHECK (ROW(a, b, c, d, e) IS NOT NULL)

instead of:

Check constraints:
"tab_a_not_null" CHECK (a IS NOT NULL)
"tab_b_not_null" CHECK (b IS NOT NULL)
"tab_c_not_null" CHECK (c IS NOT NULL)
"tab_d_not_null" CHECK (d IS NOT NULL)
"tab_e_not_null" CHECK (e IS NOT NULL)

But the performance of repeated row-casting would probably not be as
good as our current NULL checks if we'd use the current row
infrastructure, and constraint failure reports wouldn't be as helpful
as the current attribute NOT NULL failures.

Kind regards,

Matthias van de Meent