Re: not null constraints, again - Mailing list pgsql-hackers
From | Alvaro Herrera |
---|---|
Subject | Re: not null constraints, again |
Date | |
Msg-id | 202409252014.74iepgsyuyws@alvherre.pgsql Whole thread Raw |
In response to | Re: not null constraints, again (jian he <jian.universality@gmail.com>) |
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/
pgsql-hackers by date: