Re: not null constraints, again - Mailing list pgsql-hackers

On 2024-Sep-25, jian he wrote:

> copy from src/test/regress/sql/index_including.sql
> -- Unique index and unique constraint
> CREATE TABLE tbl_include_unique1 (c1 int, c2 int, c3 int, c4 box);
> INSERT INTO tbl_include_unique1 SELECT x, 2*x, 3*x, box('4,4,4,4')
> FROM generate_series(1,10) AS x;
> CREATE UNIQUE INDEX tbl_include_unique1_idx_unique ON
> tbl_include_unique1 using btree (c1, c2) INCLUDE (c3, c4);
> ALTER TABLE tbl_include_unique1 add UNIQUE USING INDEX
> tbl_include_unique1_idx_unique;
> \d+ tbl_include_unique1
> 
> transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
>                 /* Ensure these columns get a NOT NULL constraint */
>                 cxt->nnconstraints =
>                     lappend(cxt->nnconstraints,
>                             makeNotNullConstraint(makeString(attname)));
> the above code can only apply when (constraint->contype ==
> CONSTR_UNIQUE ) is false.
> The above sql example shows that (constraint->contype == CONSTR_UNIQUE
> ) can be true.

Doh, yeah.  Fixed and added a test for this.

> 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;
> alter table idxpart alter column a drop not null;
> 
> "alter table idxpart alter column a set not null;"
> will make idxpart0_a_not_null constraint islocal and inhertited,
> which is not OK?
> for partition trees, only the top level/root can be local for not-null
> constraint?
> 
> "alter table idxpart alter column a drop not null;"
> should cascade to idxpart0?

Hmm, I think this behaves OK.  It's valid to have a child with a
constraint that the parent doesn't have.  And then if the parent
acquires one and passes it down to the children, then deleting it from
the parent should not leave the child unprotected.  This is the whole
reason we have the "inhcount/islocal" system, after all.

One small glitch here is that detaching a partition (or removing
inheritance) does not remove the constraint, even if islocal=false and
inhcount reaches 0.  Instead, we turn islocal=true, so that the
constraint continues to exist.  This is a bit weird, but the intent is
to preserve properties and give the user an explicit choice; they can
still drop the constraint after detaching.  Also, columns also work that
way:

create table parent (a int);
create table child () inherits (parent);
select attrelid::regclass, attname, attislocal, attinhcount from pg_attribute where attname = 'a';
 attrelid │ attname │ attislocal │ attinhcount 
──────────┼─────────┼────────────┼─────────────
 parent   │ a       │ t          │           0
 child    │ a       │ f          │           1

alter table child no inherit parent;

select attrelid::regclass, attname, attislocal, attinhcount from pg_attribute where attname = 'a';
 attrelid │ attname │ attislocal │ attinhcount 
──────────┼─────────┼────────────┼─────────────
 parent   │ a       │ t          │           0
 child    │ a       │ t          │           0

Here the column on child, which didn't have a local definition, becomes
a local column during NO INHERIT.


>    <para>
>     However, a column can have at most one explicit not-null constraint.
>    </para>
> maybe we can add a sentence:
> "Adding not-null constraints on a column marked as not-null is a no-op."
> then we can easily explain case like:
> create table t(a int primary key , b int, constraint nn  not null a );
> the final not-null constraint name is "t_a_not_null1"

Yeah, I've been thinking about this in connection with the restriction I
just added to forbid two NOT NULLs with differing NO INHERIT flags: we
need to preserve a constraint name if it's specified, or raise an error
if two different names are specified.  This requires a change in
AddRelationNotNullConstraints() to propagate a name specified later in
the constraint list.   This made me realize that
transformColumnDefinition() also has a related problem, in that it
ignores a subsequent constraint if multiple ones are defined on the same
column, such as in
  create table notnull_tbl2 (a int primary key generated by default as
  identity constraint foo not null constraint foo not null no inherit);
here, the constraint lacks the NO INHERIT flag even though it was
specifically requested the second time.


>     /*
>      * Run through the constraints that need to generate an index, and do so.
>      *
>      * For PRIMARY KEY, in addition we set each column's attnotnull flag true.
>      * We do not create a separate not-null constraint, as that would be
>      * redundant: the PRIMARY KEY constraint itself fulfills that role.  Other
>      * constraint types don't need any not-null markings.
>      */
> the above comments in transformIndexConstraints is wrong
> and not necessary?
> "create table t(a int primary key)"
> we create a primary key and also do create separate a not-null
> constraint for "t"

I'm going to replace it with "For PRIMARY KEY, we queue not-null
constraints for each column."

>                 /*
>                  * column is defined in the new table.  For PRIMARY KEY, we
>                  * can apply the not-null constraint cheaply here.  Note that
>                  * this isn't effective in ALTER TABLE, unless the column is
>                  * being added in the same command.
>                  */
> in transformIndexConstraint, i am not sure the meaning of the third
> sentence in above comments

Yeah, this is mostly a preexisting comment (though it was originally
talking about tables OF TYPE, which is a completely different thing):

create type atype as (a int, b text);
create table atable of atype (not null a no inherit);
\d+ atable 
                                                Tabla «public.atable»
 Columna │  Tipo   │ Ordenamiento │ Nulable  │ Por omisión │ Almacenamiento │ Compresió>
─────────┼─────────┼──────────────┼──────────┼─────────────┼────────────────┼──────────>
 a       │ integer │              │ not null │             │ plain          │          >
 b       │ text    │              │          │             │ extended       │          >
Not-null constraints:
    "atable_a_not_null" NOT NULL "a" NO INHERIT
Tabla tipada de tipo: atype


Anyway, what this comment means is that if the ALTER TABLE is doing ADD
CONSTRAINT on columns that already exist on the table (as opposed to
doing it on columns that the same ALTER TABLE command is doing ADD
COLUMN for), then "this isn't effective" (ie. it doesn't do anything).
In reality, this comment is now wrong, because during ALTER TABLE the
NOT NULL constraints are added by ATPrepAddPrimaryKey, which occurs
before this code runs, so the column->is_not_null clause is always true
and this block is not executed.  This code is only used during CREATE
TABLE.  So the comment needs to be removed, or maybe done this way with
an extra assertion:

                /*
+                * column is defined in the new table.  For CREATE TABLE with
+                * a PRIMARY KEY, we can apply the not-null constraint cheaply
+                * here.  Note that ALTER TABLE never needs this, because
+                * those constraints have already been added by
+                * ATPrepAddPrimaryKey.
                 */
                if (constraint->contype == CONSTR_PRIMARY &&
                    !column->is_not_null)
                {
+                   Assert(!cxt->isalter);  /* doesn't occur in ALTER TABLE */
                    column->is_not_null = true;
                    cxt->nnconstraints =
                        lappend(cxt->nnconstraints,
                                makeNotNullConstraint(makeString(key)));
                }


> i see no error message like
> ERROR:  NOT NULL constraints cannot be marked NOT VALID
> ERROR:  not-null constraints for domains cannot be marked NO INHERIT
> in regress tests. we can add some in src/test/regress/sql/domain.sql
> like:
> 
> create domain d1 as text not null no inherit;
> create domain d1 as text constraint nn not null no inherit;
> create domain d1 as text constraint nn not null;
> ALTER DOMAIN d1 ADD constraint nn not null NOT VALID;
> drop domain d1;

Yeah, I too noticed the lack of tests for not-valid not-null constraints
on domains a few days ago.  While I was exploring that I noticed that
they have some NO INHERIT that seems to be doing nothing (as it should,
because what would it actually mean?), so we should remove the gram.y
bits that try to handle it.  We could add these tests you suggest
irrespective of this not-nulls patch in this thread.

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