Re: not null constraints, again

On 2024-Oct-01, jian he wrote:

> create table t2 (a int primary key constraint foo not null no inherit);
> primary key cannot coexist with not-null no inherit?
> here t2, pg_dump/restore will fail.

Yeah, this needs to throw an error. If you use a table constraint, it
does fail as expected:

create table notnull_tbl_fail (a int primary key, not null a no inherit);
ERROR: conflicting NO INHERIT declaration for not-null constraint on column "a"

I missed adding the check in the column constraint case.

> create table t7 (a int generated by default as identity, constraint foo not null a no inherit, b int);
> create table t7 (a int generated by default as identity not null no inherit, b int);
> first fail, second not fail. pg_dump output is:
>
> CREATE TABLE public.t7 (a integer NOT NULL NO INHERIT,b integer);
> ALTER TABLE public.t7 ALTER COLUMN a ADD GENERATED BY DEFAULT AS IDENTITY (
> SEQUENCE NAME public.t7_a_seq
> START WITH 1
> INCREMENT BY 1
> NO MINVALUE
> NO MAXVALUE
> CACHE 1
> );
> seems there is a consistency between column_constraint, table_constraint.
> but in this case, the public.t7 dump is fine.

Yeah. I don't see any reasonable way to avoid this problem; I mean, we
could add something on the Constraint node that's something like "the NO
INH flag of this constraint is unspecified and we don't care what it is"
(maybe change is_no_inherit from boolean to a tri-state), so that
AddRelationNotNullConstraints() allows a NO INHERIT constraint to
override a normal one. But this feels too much mechanism for such a
fringe feature. I'd rather we live with the wart. I don't think it's
very serious anyway.

To clarify. What happens in the second case is that we process both the
GENERATED and the NOT NULL clauses in a single transformColumnDefinition
pass; for GENERATED we see that we need a NOT NULL, and the NOT NULL is
right there (albeit with a NO INHERIT clause, but GENERATED doesn't
care); so all's well and it works.

In the first case, we see these two things separately. On one hand we
get GENERATED in transformColumnDefinition, which requires a not-null;
it adds one. Separately we have the NOT NULL NO INHERIT, which is
processed by transformTableConstraint. When adding this one, it doesn't
see that we already have a not-null constraint for the column, so we add
it to be processed later. Both constraint requests travel to
AddRelationNotNullConstraints, which is the first time we consider them
together. By then, there's no way to know that the one in GENERATED
would accept being NO INHERIT, we just see that it is not NO INHERIT, so
it conflicts with the other one, kaboom.

> -------------------------------------------------------------------------------
> + By contrast, a <literal>NOT NULL</literal> constraint that was created
> + as <literal>NO INHERIT</literal> will be changed to a normal inheriting
> + one during attach.
> Does this sentence don't have corresponding tests?
> i think you mean something like:
>
> drop table if exists idxpart,idxpart0,idxpart1 cascade;
> create table idxpart (a int not null) partition by list (a);
> create table idxpart0 (a int constraint foo not null no inherit);
> alter table idxpart attach partition idxpart0 for values in (0,1,NULL);

Yeah. We have the equivalent test for attaching an inheritance-child
rather than a partition, which is essentially the same thing, in
inherit.sql:

-- Can turn a NO INHERIT constraint on children into normal, but only if
-- there aren't children
create table inh_parent (a int not null);
create table inh_child (a int not null no inherit);
create table inh_grandchild () inherits (inh_child);
alter table inh_child inherit inh_parent; -- nope
drop table inh_child, inh_grandchild;
create table inh_child (a int not null no inherit);
alter table inh_child inherit inh_parent; -- now it works

I think we could just remove this behavior and nothing of value would be
lost. If I recall correctly, handling of NO INHERIT constraints in this
way was just added to support the old way of adding PRIMARY KEY, but it
feels like a wart that's easily fixed and not worth having, because it's
just weird. I mean, what's the motivation for having created the
partition (resp. child table) with a NO INHERIT constraint in the first
place?

> ---------------------------------------------------------------------------------
> the pg_dump of
> -------------
> drop table if exists idxpart, idxpart0 cascade;
> create table idxpart (a int) partition by range (a);
> create table idxpart0 (a int not null);
> alter table idxpart attach partition idxpart0 for values from (0) to (100);
> alter table idxpart alter column a set not null;
> -------------
> is
>
> CREATE TABLE public.idxpart (a integer NOT NULL)PARTITION BY RANGE (a);
> CREATE TABLE public.idxpart0 (a integer NOT NULL);
> ALTER TABLE ONLY public.idxpart ATTACH PARTITION public.idxpart0 FOR
> VALUES FROM (0) TO (100);
>
> After pu_dump, the attribute conislocal of constraint
> idxpart0_a_not_null changes from true to false,
> is this OK for attribute change after pg_dump in this case?

Good question. I don't think we care about that in practice (the
constraint becomes islocal=true if you happen to DETACH; and other than
that, the constraint cannot change state in any way). CHECK constraints
behave in the same way, and I'm not sure I want to deviate from that.
I think there are pg_upgrade woes that would become better if we did
though.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)