Re: cataloguing NOT NULL constraints

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 <alvherre@alvh.no-ip.org> 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.,
> <PK_name>_<col_name>_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 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

Oh, this one's a bad omission.  I'll fix it.


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

Strange.  I'll see about fixing this one too.

-- 
Á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)