Re: not null constraints, again
On 2024-Dec-12, jian he wrote: > patch attached. > > also change comments of heap_create_with_catalog, > StoreConstraints, MergeAttributes. > so we can clear idea what's kind of constraints we are dealing with > in these functions. Great catch! The patch looks good, I have pushed it. Thank you. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)
Re: not null constraints, again
On Wed, Dec 4, 2024 at 10:52 AM jian he wrote:
>
> hi.
>
> heap_create_with_catalog argument (cooked_constraints):
> passed as NIL in function {create_toast_table, make_new_heap}
> passed as list_concat(cookedDefaults,old_constraints) in DefineRelation
>
> in DefineRelation we have function call:
> MergeAttributes
> heap_create_with_catalog
> StoreConstraints
>
> StoreConstraints second argument: cooked_constraints, some is comes from
> DefineRelation->MergeAttributes old_constraints:
> {
> stmt->tableElts = MergeAttributes(stmt->tableElts, inheritOids,
> stmt->relation->relpersistence, stmt->partbound != NULL, &old_constraints,
> &old_notnulls);
> }
>
> My understanding from DefineRelation->MergeAttributes is that old_constraints
> will only have CHECK constraints.
> that means heap_create_with_catalog->StoreConstraints
> StoreConstraints didn't actually handle CONSTR_NOTNULL.
>
> heap_create_with_catalog comments also says:
> * cooked_constraints: list of precooked check constraints and defaults
>
> coverage https://coverage.postgresql.org/src/backend/catalog/heap.c.gcov.html
> also shows StoreConstraints, CONSTR_NOTNULL never being called,
> which is added by this thread.
>
>
> my question is can we remove StoreConstraints, CONSTR_NOTNULL handling.
> we have 3 functions {StoreConstraints, AddRelationNotNullConstraints,
> AddRelationNewConstraints} that will call StoreRelNotNull to store the
> not-null
> constraint. That means if we want to bullet proof that something is
> conflicting
> with not-null, we need to add code to check all these 3 places.
> removing StoreConstraints handling not-null seems helpful.
>
>
> also comments in MergeAttributes:
> * Output arguments:
> * 'supconstr' receives a list of constraints belonging to the parents,
> *updated as necessary to be valid for the child.
> * 'supnotnulls' receives a list of CookedConstraints that corresponds to
> *constraints coming from inheritance parents.
>
> can we be explicit that "supconstr" is only about CHECK constraint,
> "supnotnulls" is
> only about NOT-NULL constraint.
patch attached.
also change comments of heap_create_with_catalog,
StoreConstraints, MergeAttributes.
so we can clear idea what's kind of constraints we are dealing with
in these functions.
From 1a2d75b6d107eb372edfca8e9a2e7df19ba08a6e Mon Sep 17 00:00:00 2001
From: jian he
Date: Thu, 12 Dec 2024 10:45:58 +0800
Subject: [PATCH v1 1/1] remove StoreConstraints dealing with CONSTR_NOTNULL
StoreConstraints never need to deal with CONSTR_NOTNULL.
so remove that part.
because of this, change comments for functions: heap_create_with_catalog,
StoreConstraints, MergeAttributes.
so we can clear idea what's kind of constraints we are dealing with
in these functions.
discussion: https://postgr.es/m/CACJufxFxzqrCiUNfjJ0tQU+=nKQkQCGtGzUBude=smowj5v...@mail.gmail.com
---
src/backend/catalog/heap.c | 11 ++-
src/backend/commands/tablecmds.c | 8
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index d7b88b61dc..6a6c328a27 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1489,7 +1489,7 @@ heap_create_with_catalog(const char *relname,
InvokeObjectPostCreateHookArg(RelationRelationId, relid, 0, is_internal);
/*
- * Store any supplied constraints and defaults.
+ * Store any supplied CHECK constraints and defaults.
*
* NB: this may do a CommandCounterIncrement and rebuild the relcache
* entry, so the relation must be valid and self-consistent at this point.
@@ -2224,7 +2224,7 @@ StoreRelNotNull(Relation rel, const char *nnname, AttrNumber attnum,
}
/*
- * Store defaults and constraints (passed as a list of CookedConstraint).
+ * Store defaults and CHECK constraints (passed as a list of CookedConstraint).
*
* Each CookedConstraint struct is modified to store the new catalog tuple OID.
*
@@ -2268,13 +2268,6 @@ StoreConstraints(Relation rel, List *cooked_constraints, bool is_internal)
numchecks++;
break;
- case CONSTR_NOTNULL:
-con->conoid =
- StoreRelNotNull(rel, con->name, con->attnum,
- !con->skip_validation, con->is_local,
- con->inhcount, con->is_no_inherit);
-break;
-
default:
elog(ERROR, "unrecognized constraint type: %d",
(int) con->contype);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6ccae4cb4a..9142f8e4ad 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2433,10 +2433,10 @@ storage_name(char c)
* 'is_partition' tells if the table is a partition.
*
* Output arguments:
- * 'supconstr' receives a list of constraints belonging to the parents,
- * updated as necessary to be valid for the child.
- * 'supnotnulls' receives a list of CookedConstraints that corresponds to
- * constraints coming from inheritance parents.
+ * 'supconstr' receives a list of CookedConstraints CH
Re: not null constraints, again
hi.
heap_create_with_catalog argument (cooked_constraints):
passed as NIL in function {create_toast_table, make_new_heap}
passed as list_concat(cookedDefaults,old_constraints) in DefineRelation
in DefineRelation we have function call:
MergeAttributes
heap_create_with_catalog
StoreConstraints
StoreConstraints second argument: cooked_constraints, some is comes from
DefineRelation->MergeAttributes old_constraints:
{
stmt->tableElts = MergeAttributes(stmt->tableElts, inheritOids,
stmt->relation->relpersistence, stmt->partbound != NULL, &old_constraints,
&old_notnulls);
}
My understanding from DefineRelation->MergeAttributes is that old_constraints
will only have CHECK constraints.
that means heap_create_with_catalog->StoreConstraints
StoreConstraints didn't actually handle CONSTR_NOTNULL.
heap_create_with_catalog comments also says:
* cooked_constraints: list of precooked check constraints and defaults
coverage https://coverage.postgresql.org/src/backend/catalog/heap.c.gcov.html
also shows StoreConstraints, CONSTR_NOTNULL never being called,
which is added by this thread.
my question is can we remove StoreConstraints, CONSTR_NOTNULL handling.
we have 3 functions {StoreConstraints, AddRelationNotNullConstraints,
AddRelationNewConstraints} that will call StoreRelNotNull to store the not-null
constraint. That means if we want to bullet proof that something is conflicting
with not-null, we need to add code to check all these 3 places.
removing StoreConstraints handling not-null seems helpful.
also comments in MergeAttributes:
* Output arguments:
* 'supconstr' receives a list of constraints belonging to the parents,
*updated as necessary to be valid for the child.
* 'supnotnulls' receives a list of CookedConstraints that corresponds to
*constraints coming from inheritance parents.
can we be explicit that "supconstr" is only about CHECK constraint,
"supnotnulls" is
only about NOT-NULL constraint.
Re: not null constraints, again
On 2024-Nov-09, Alvaro Herrera wrote: > I notice I cargo-culted a "free de-toasted copy", but I think it's > impossible to end up with a toasted datum here, because the column is > guaranteed to have only one element, so not a candidate for toasting. > But also, if we don't free it (in case somebody does an UPDATE to the > catalog with a large array), nothing happens, because memory is going to > be released soon anyway, by the error that results by conkey not being > one element long. I found out that my claim that it's impossible to have a detoasted datum was false: because the value is so small, we end up with a short varlena, which does use a separate palloc(). I decided to remove the pfree() anyway, because that makes it easier to return the value we want without having to first assign it away from the chunk we'd pfree. The DDL code mostly doesn't worry too much about memory leaks anyway, and this one is very small. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ “Cuando no hay humildad las personas se degradan” (A. Christie)
Re: not null constraints, again
On 2024-Nov-08, Tom Lane wrote: > Alvaro Herrera writes: > > But we'll see what else the buildfarm has to say now that I pushed it ... > > A lot of the buildfarm is saying > > adder | 2024-11-08 13:04:39 | > ../pgsql/src/backend/catalog/pg_constraint.c:708:37: warning: comparison is > always true due to limited range of data type [-Wtype-limits] > > which evidently is about this: > > Assert(colnum > 0 && colnum <= MaxAttrNumber); Hah. > The memcpy right before that doesn't seem like project style either. > Most other places that are doing similar things just cast the > ARR_DATA_PTR to the right pointer type and dereference it. Hmm, yeah, that's easily removed, diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index e953000c01d..043bf7c24dd 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -704,11 +704,7 @@ extractNotNullColumn(HeapTuple constrTup) ARR_DIMS(arr)[0] != 1) elog(ERROR, "conkey is not a 1-D smallint array"); - memcpy(&colnum, ARR_DATA_PTR(arr), sizeof(AttrNumber)); - Assert(colnum > 0 && colnum <= MaxAttrNumber); - - if ((Pointer) arr != DatumGetPointer(adatum)) - pfree(arr); /* free de-toasted copy, if any */ + colnum = ((AttrNumber *) ARR_DATA_PTR(arr))[0]; return colnum; } I notice I cargo-culted a "free de-toasted copy", but I think it's impossible to end up with a toasted datum here, because the column is guaranteed to have only one element, so not a candidate for toasting. But also, if we don't free it (in case somebody does an UPDATE to the catalog with a large array), nothing happens, because memory is going to be released soon anyway, by the error that results by conkey not being one element long. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Si quieres ser creativo, aprende el arte de perder el tiempo"
Re: not null constraints, again
Alvaro Herrera writes: > But we'll see what else the buildfarm has to say now that I pushed it ... A lot of the buildfarm is saying adder | 2024-11-08 13:04:39 | ../pgsql/src/backend/catalog/pg_constraint.c:708:37: warning: comparison is always true due to limited range of data type [-Wtype-limits] which evidently is about this: Assert(colnum > 0 && colnum <= MaxAttrNumber); The memcpy right before that doesn't seem like project style either. Most other places that are doing similar things just cast the ARR_DATA_PTR to the right pointer type and dereference it. regards, tom lane
Re: not null constraints, again
On 2024-Nov-08, jian he wrote: > > Here's v11, which I intended to commit today, but didn't get around to. > > CI is happy with it, so I'll probably do it tomorrow first thing. > > > v11 still has column_constraint versus table_constraint inconsistency. > > 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); > create table t8 (a serial, constraint foo1 not null a no inherit); > create table t8 (a serial not null no inherit, b int); > > i solved this issue at [1], Ah yeah, that stuff. Your commit message said it was a refactoring so I hadn't paid too much attention to it, but it's in fact not a refactoring at all. I included it with a large comment explaining why we do it that way and that we may want to remove it in the future. I also included these four sentences above in the tests, and pushed it after checking that the CI results are clean. Yesterday I verified that pg_upgrade works with the regression database from 12 onwards. I know the buildfarm uses a different way to do the pg_upgrade test, so there's no way to know if it'll work ahead of time. But we'll see what else the buildfarm has to say now that I pushed it ... -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: not null constraints, again
> Here's v11, which I intended to commit today, but didn't get around to. > CI is happy with it, so I'll probably do it tomorrow first thing. > v11 still has column_constraint versus table_constraint inconsistency. 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); create table t8 (a serial, constraint foo1 not null a no inherit); create table t8 (a serial not null no inherit, b int); i solved this issue at [1], that patch has one whitespace issue though. what do you think? [1] https://postgr.es/m/cacjufxhgbsjrhygj0eqzi9xv+zsozndcuj5sg-f5wk+dgcy...@mail.gmail.com
Re: not null constraints, again
On 2024-Nov-08, jian he wrote:
> > Here's v11, which I intended to commit today, but didn't get around to.
> > CI is happy with it, so I'll probably do it tomorrow first thing.
> >
>
> CREATE TABLE notnull_tbl2 (a INTEGER CONSTRAINT blah NOT NULL, b
> INTEGER CONSTRAINT blah NOT NULL);
>
> RelationGetNotNullConstraints, StoreRelNotNull
> will first create the constraint "blah", then iterate through the
> second "blah" error out,
> which is not great for error out cleaning, i believe.
I applaud your enthusiasm, but I don't like this change. We have plenty
of cases where we abort a command partway through after having created a
bunch of catalog rows (we even have comments about such behavior being
acceptable); if we wanted to get rid of them all, the code would become
far too complicated because it'd have to save state until the last
minute, just in case something else threw errors. Your proposed coding
seems complicated enough, in fact, given how fringe an error condition
it's protecting against. It's not like the user will try to run the
command thousands of times "to see if it works next time". One dead
catalog row every now and then won't hurt anything.
> while debugging, in RelationGetNotNullConstraints
> if (cooked)
> {
> CookedConstraint *cooked;
> cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
> cooked->contype = CONSTR_NOTNULL;
> cooked->name = pstrdup(NameStr(conForm->conname));
> cooked->attnum = colnum;
> .
> }
> We missed the assignment of cooked->conoid?
Eh, I can't see the OID would ever be useful for anything, but let's put
it there just in case some future caller wants it for some reason.
> MergeConstraintsIntoExisting
> /*
> * If the CHECK child constraint is "no inherit" then cannot
> * merge.
> */
> if (child_con->connoinherit)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> errmsg("constraint \"%s\" conflicts with
> non-inherited constraint on child table \"%s\"",
> NameStr(child_con->conname),
> RelationGetRelationName(child_rel;
>
> the above comment can also be hit by not-null constraint, so the
> comment is wrong?
Strange ... my copy is fixed already, and in fact I don't see the patch
touching this function at all. [ pokes around ] Ah, I changed it two
weeks ago:
https://github.com/alvherre/postgres/commit/efeed9416b8c7397d61446958d6835e23ec3f0b6
Thanks for looking once more!
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
"La grandeza es una experiencia transitoria. Nunca es consistente.
Depende en gran parte de la imaginación humana creadora de mitos"
(Irulan)
Re: not null constraints, again
>
> Here's v11, which I intended to commit today, but didn't get around to.
> CI is happy with it, so I'll probably do it tomorrow first thing.
>
CREATE TABLE notnull_tbl2 (a INTEGER CONSTRAINT blah NOT NULL, b
INTEGER CONSTRAINT blah NOT NULL);
RelationGetNotNullConstraints, StoreRelNotNull
will first create the constraint "blah", then iterate through the
second "blah" error out,
which is not great for error out cleaning, i believe.
so i change AddRelationNotNullConstraints
first loop "for (int outerpos = 0; outerpos <
list_length(constraints); outerpos++)"
we can first validate it through the loop, collect information
then do a loop to StoreRelNotNull.
while debugging, in RelationGetNotNullConstraints
if (cooked)
{
CookedConstraint *cooked;
cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
cooked->contype = CONSTR_NOTNULL;
cooked->name = pstrdup(NameStr(conForm->conname));
cooked->attnum = colnum;
.
}
We missed the assignment of cooked->conoid?
MergeConstraintsIntoExisting
/*
* If the CHECK child constraint is "no inherit" then cannot
* merge.
*/
if (child_con->connoinherit)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("constraint \"%s\" conflicts with
non-inherited constraint on child table \"%s\"",
NameStr(child_con->conname),
RelationGetRelationName(child_rel;
the above comment can also be hit by not-null constraint, so the
comment is wrong?
v11-0001-refactor-AddRelationNotNullConstraints.no-cfbot
Description: Binary data
Re: not null constraints, again
On 2024-Nov-07, jian he wrote: > RemoveInheritance > if (copy_con->coninhcount <= 0) /* shouldn't happen */ > elog(ERROR, "relation %u has non-inherited constraint \"%s\"", > RelationGetRelid(child_rel), NameStr(copy_con->conname)); > dropconstraint_internal > if (childcon->coninhcount <= 0) /* shouldn't happen */ > elog(ERROR, "relation %u has non-inherited constraint \"%s\"", > childrelid, NameStr(childcon->conname)); > > RemoveInheritance error triggered (see below), dropconstraint_internal may > also. > that means the error message should use RelationGetRelationName > rather than plain "relation %u"? > > drop table if exists inh_parent,inh_child1,inh_child2; > create table inh_parent(f1 int not null no inherit); > create table inh_child1(f1 int not null no inherit); > alter table inh_child1 inherit inh_parent; > alter table inh_child1 NO INHERIT inh_parent; > ERROR: relation 26387 has non-inherited constraint "inh_child1_f1_not_null" Hmm, no, this is just a code bug: in RemoveInheritance when scanning 'parent' for constraints, we must skip the ones that are NO INHERIT, but weren't. With the bug fixed, the sequence above results in a no-longer-child inh_child1 that still has inh_child1_f1_not_null, and no error is thrown. > sql-altertable.html > INHERIT parent_table > This form adds the target table as a new child of the specified > parent table. > Subsequently, queries against the parent will include records of the > target > table. To be added as a child, the target table must already contain > all the > same columns as the parent (it could have additional columns, too). > The columns > must have matching data types, and if they have NOT NULL constraints > in the > parent then they must also have NOT NULL constraints in the child. > > " > The columns must have matching data types, and if they have NOT NULL > constraints in the > parent then they must also have NOT NULL constraints in the child. > " > For the above sentence, we need to add some text to explain > NOT NULL constraints, NO INHERIT property > for the child table and parent table. True. I rewrote as follows, moving the whole explanation of constraints together to the same paragraph, rather than talking about some constraints in one paragraph and other constraints in another. The previous approach was better when NOT NULL markings were a property of the column, but now that they are constraints in their own right, this seems better. INHERIT parent_table This form adds the target table as a new child of the specified parent table. Subsequently, queries against the parent will include records of the target table. To be added as a child, the target table must already contain all the same columns as the parent (it could have additional columns, too). The columns must have matching data types. In addition, all CHECK and NOT NULL constraints on the parent must also exist on the child, except those marked non-inheritable (that is, created with ALTER TABLE ... ADD CONSTRAINT ... NO INHERIT), which are ignored. All child-table constraints matched must not be marked non-inheritable. Currently UNIQUE, PRIMARY KEY, and FOREIGN KEY constraints are not considered, but this might change in the future. > > drop table if exists inh_parent,inh_child1,inh_child2; > create table inh_parent(f1 int not null no inherit); > create table inh_child1(f1 int); > alter table inh_child1 inherit inh_parent; > alter table inh_child1 NO INHERIT inh_parent; > ERROR: 1 unmatched constraints while removing inheritance from "inh_child1" > to "inh_parent" > > now, we cannot "uninherit" inh_child1 from inh_parent? > not sure this is expected behavior. Yeah, this is the same bug as above. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "In fact, the basic problem with Perl 5's subroutines is that they're not crufty enough, so the cruft leaks out into user-defined code instead, by the Conservation of Cruft Principle." (Larry Wall, Apocalypse 6)
Re: not null constraints, again
RemoveInheritance if (copy_con->coninhcount <= 0) /* shouldn't happen */ elog(ERROR, "relation %u has non-inherited constraint \"%s\"", RelationGetRelid(child_rel), NameStr(copy_con->conname)); dropconstraint_internal if (childcon->coninhcount <= 0) /* shouldn't happen */ elog(ERROR, "relation %u has non-inherited constraint \"%s\"", childrelid, NameStr(childcon->conname)); RemoveInheritance error triggered (see below), dropconstraint_internal may also. that means the error message should use RelationGetRelationName rather than plain "relation %u"? drop table if exists inh_parent,inh_child1,inh_child2; create table inh_parent(f1 int not null no inherit); create table inh_child1(f1 int not null no inherit); alter table inh_child1 inherit inh_parent; alter table inh_child1 NO INHERIT inh_parent; ERROR: relation 26387 has non-inherited constraint "inh_child1_f1_not_null" sql-altertable.html INHERIT parent_table This form adds the target table as a new child of the specified parent table. Subsequently, queries against the parent will include records of the target table. To be added as a child, the target table must already contain all the same columns as the parent (it could have additional columns, too). The columns must have matching data types, and if they have NOT NULL constraints in the parent then they must also have NOT NULL constraints in the child. " The columns must have matching data types, and if they have NOT NULL constraints in the parent then they must also have NOT NULL constraints in the child. " For the above sentence, we need to add some text to explain NOT NULL constraints, NO INHERIT property for the child table and parent table. drop table if exists inh_parent,inh_child1,inh_child2; create table inh_parent(f1 int not null no inherit); create table inh_child1(f1 int); alter table inh_child1 inherit inh_parent; alter table inh_child1 NO INHERIT inh_parent; ERROR: 1 unmatched constraints while removing inheritance from "inh_child1" to "inh_parent" now, we cannot "uninherit" inh_child1 from inh_parent? not sure this is expected behavior.
Re: not null constraints, again
sql-altertable.html SET/DROP NOT NULL These forms change whether a column is marked to allow null values or to reject null values. If this table is a partition, one cannot perform DROP NOT NULL on a column if it is marked NOT NULL in the parent table. To drop the NOT NULL constraint from all the partitions, perform DROP NOT NULL on the parent table. Now this will be slightly inaccurate. drop table if exists part, part0 cascade; create table part (a int not null) partition by range (a); create table part0 (a int not null); alter table part attach partition part0 for values from (0) to (1000); alter table ONLY part0 add primary key(a); alter table part alter column a drop not null; as the example shows that part0 not-null constraint is still there. that means: perform DROP NOT NULL on the parent table will not drop the NOT NULL constraint from all partitions. so we need rephrase the following sentence: To drop the NOT NULL constraint from all the partitions, perform DROP NOT NULL on the parent table. to address this kind of corner case?
Re: not null constraints, again
tricky case: drop table if exists part, part0 cascade; create table part (a int not null) partition by range (a); create table part0 (a int primary key); alter table part attach partition part0 for values from (0) to (1000); alter table ONLY part add primary key(a); alter table ONLY part drop constraint part_a_not_null; -- alter table ONLY part alter column a drop not null; Now we are in a state where a partitioned table have a primary key but doesn't have a not-null constraint for it. select indisunique, indisprimary, indimmediate,indisvalid frompg_index where indexrelid = 'part_pkey'::regclass; shows this primary key index is invalid. but select conname,contype,convalidated from pg_constraint where conname = 'part_pkey'; shows this primary key constraint is valid.
Re: not null constraints, again
I did some refactoring on transformColumnDefinition
since transformColumnDefinition only deals with a single ColumnDef.
and serial/primary/identity cannot allow not-null no inherit.
We can preliminary iterate through ColumnDef->constraints to check
that ColumnDef can allow not-null no inherit or not.
if not allowed, then error out at CONSTR_NOTNULL.
please check attached.
in MergeConstraintsIntoExisting
we can
while (HeapTupleIsValid(child_tuple = systable_getnext(child_scan)))
{
Form_pg_constraint child_con = (Form_pg_constraint)
GETSTRUCT(child_tuple);
HeapTuplechild_copy;
if (child_con->contype != parent_con->contype)
continue;
if (child_con->connoinherit)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("constraint \"%s\" conflicts with
non-inherited constraint on child table \"%s\"",
NameStr(child_con->conname),
RelationGetRelationName(child_rel;
if (parent_con->convalidated && !child_con->convalidated)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("constraint \"%s\" conflicts with NOT
VALID constraint on child table \"%s\"",
NameStr(child_con->conname),
RelationGetRelationName(child_rel;
}
error out earlier, save some cache search cycle.
MergeConstraintsIntoExisting comment says
" * XXX See MergeWithExistingConstraint too if you change this code."
we actually did change the MergeConstraintsIntoExisting, not change
MergeWithExistingConstraint
but it seems MergeWithExistingConstraint does not deal with CONSTRAINT_NOTNULL.
So I guess the comments are fine.
previously, we mentioned adding some domain tests at [1].
but it seems v8, we don't have domain related regression tests.
[1]
https://www.postgresql.org/message-id/202409252014.74iepgsyuyws%40alvherre.pgsql
v8-0001-transformColumnDefinition-minor-refactor.no-cfbot
Description: Binary data
Re: not null constraints, again
On Fri, Oct 4, 2024 at 9:11 PM Alvaro Herrera wrote:
>
> Here's v8 of this patch.
in AdjustNotNullInheritance
if (count > 0)
{
conform->coninhcount += count;
changed = true;
}
if (is_local)
{
conform->conislocal = true;
changed = true;
}
change to
if (count > 0)
{
conform->coninhcount += count;
changed = true;
}
if (is_local && !conform->conislocal)
{
conform->conislocal = true;
changed = true;
}
then we can save some cycles.
---<<
MergeConstraintsIntoExisting
/*
* If the CHECK child constraint is "no inherit" then cannot
* merge.
*/
if (child_con->connoinherit)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("constraint \"%s\" conflicts with
non-inherited constraint on child table \"%s\"",
NameStr(child_con->conname),
RelationGetRelationName(child_rel;
the comments apply to not-null constraint aslo, so the comments need
to be refactored.
---<<
in ATExecSetNotNull
if (recursing)
{
conForm->coninhcount++;
changed = true;
}
grep "coninhcount++", I found out pattern:
constrForm->coninhcount++;
if (constrForm->coninhcount < 0)
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
here, maybe we can change to
if (recursing)
{
// conForm->coninhcount++;
if (pg_add_s16_overflow(conForm->coninhcount,1,
&conForm->coninhcount))
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
changed = true;
}
---<<
base on your reply at [1]
By contrast, a NOT NULL constraint that was created
as NO INHERIT will be changed to a normal inheriting
one during attach.
these text should removed from section:
<>
since currently v8, partition_name not-null no inherit constraint
cannot merge with the parent.
[1]
https://www.postgresql.org/message-id/202410021219.bvjmxzdspif2%40alvherre.pgsql
Re: not null constraints, again
On 2024-Oct-03, jian he wrote: > I thought SearchSysCacheCopyAttNum is expensive. > Relation->rd_att is enough for checking attnotnull. > > What do you think of the following refactoring of set_attnotnull? Eh, sure, why not. I mean, I expect that this is going to be barely noticeable performance-wise, but I don't see a reason not to do it this way. The new code in transformIndexConstraint() I added to verify NO INHERIT for columns in the PK[1] is likely to have a more noticeable impact: we have to scan the whole cxt->nnconstraints list for each column of the PK, and strcmp() the column names in order to find matches. I expect this this to be slow (and affect everybody) but I don't see any other reasonable way to do it. A possibility is to add a Constraint member to ColumnDef, and pre-process so that we attach the correct constraint definition to each column definition before invoking transformIndexConstraints in transformCreateStmt; we already do the match there, so it would be a good place for that. Alternatively, turn is_not_null into a tristate (yes, no, "yes but is no inherit"). [1] https://github.com/alvherre/postgres/commit/22e5820e241c744fb36cbc643a4d8d94162c562e -- Álvaro HerreraBreisgau, Deutschland — 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, http://www.paulgraham.com/opensource.html
Re: not null constraints, again
I thought SearchSysCacheCopyAttNum is expensive.
Relation->rd_att is enough for checking attnotnull.
What do you think of the following refactoring of set_attnotnull?
static void
set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum,
LOCKMODE lockmode)
{
Oidreloid = RelationGetRelid(rel);
HeapTupletuple;
Form_pg_attribute attForm;
Form_pg_attribute attr;
TupleDesctupleDesc;
CheckAlterTableIsSafe(rel);
tupleDesc = RelationGetDescr(rel);
attr = TupleDescAttr(tupleDesc, attnum - 1);
if (attr->attisdropped)
return;
if (!attr->attnotnull)
{
Relationattr_rel;
attr_rel = table_open(AttributeRelationId, RowExclusiveLock);
tuple = SearchSysCacheCopyAttNum(reloid, attnum);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for attribute %d of relation %u",
attnum, reloid);
attForm = (Form_pg_attribute) GETSTRUCT(tuple);
attForm->attnotnull = true;
CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
if (wqueue && !NotNullImpliedByRelConstraints(rel, attForm))
{
AlteredTableInfo *tab;
tab = ATGetQueueEntry(wqueue, rel);
tab->verify_new_notnull = true;
}
CommandCounterIncrement();
heap_freetuple(tuple);
table_close(attr_rel, RowExclusiveLock);
}
}
Re: not null constraints, again
On 2024-Oct-02, Alvaro Herrera wrote: > On 2024-Oct-02, jian he wrote: > > > On Tue, Oct 1, 2024 at 11:20 PM Alvaro Herrera > > wrote: > > after v7, still not bullet-proof. as before, pg_dump/restore will fail > > for the following: > > > > drop table if exists t2, t2_0 > > create table t2 (a int, b int, c int, constraint foo primary key(a), > > constraint foo1 not null a no inherit); > > create table t2_0 (a int constraint foo1 not null no inherit, b int, c > > int, constraint foo12 primary key(a)); > > Rats. Fixing :-) Hmm, I thought this was going to be a five-minute job: I figured I could add a check in DefineIndex() that reads all columns and ensure they're no-inherit. First complication: when creating a partition, we do DefineIndex to create the indexes that the parent table has, before we do AddRelationNotNullConstraints(), so the not-null constraint lookup fails. Easy enough to fix: just move the AddRelationNotNullConstraints call a few lines up. However, things are still not OK because ALTER TABLE ALTER COLUMN TYPE does want to recreate the PK before the not-nulls (per ATPostAlterTypeParse), because AT_PASS_OLD_INDEX comes before AT_PASS_OLD_CONSTR ... and obviously we cannot change that. Another possibility is to add something like AT_PASS_OLD_NOTNULL but that sounds far too ad-hoc. Maybe I need the restriction to appear somewhere else rather than on DefineIndex. Still looking ... -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Los dioses no protegen a los insensatos. Éstos reciben protección de otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
Re: not null constraints, again
On 2024-Oct-02, jian he wrote:
> On Tue, Oct 1, 2024 at 11:20 PM Alvaro Herrera
> wrote:
> >
> > 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.
> >
> after v7, still not bullet-proof. as before, pg_dump/restore will fail
> for the following:
>
> drop table if exists t2, t2_0
> create table t2 (a int, b int, c int, constraint foo primary key(a),
> constraint foo1 not null a no inherit);
> create table t2_0 (a int constraint foo1 not null no inherit, b int, c
> int, constraint foo12 primary key(a));
Rats. Fixing :-)
> 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);
>
> With V7, we basically cannot change the status of "NO INHERIT".
> now, we need to drop the not-null constraint foo,
> recreate a not-null constraint on idxpart0,
> then attach it to the partitioned table idxpart.
Yeah, that sucks. We'll need a new command
ALTER TABLE .. ALTER CONSTRAINT .. INHERIT
(exact syntax TBD) which allows you to turn a NO INHERIT constraint into
a normal one, to avoid this problem. I suggest we don't hold up this
patch for that.
> -
> drop table if exists inh_parent,inh_child1,inh_child2;
> create table inh_parent(f1 int);
> create table inh_child1(f1 int not null);
> alter table inh_child1 inherit inh_parent;
> alter table only inh_parent add constraint nn not null f1;
> alter table only inh_parent alter column f1 set not null;
>
> minor inconsistency, i guess.
> "alter table only inh_parent add constraint nn not null f1;"
> will fail.
> But
> "alter table only inh_parent alter column f1 set not null;"
> will not fail, but add a "NOT NULL f1 NO INHERIT" constraint.
> I thought they should behave the same.
>
>
> for partitioned table
> now both ALTER TABLE ONLY ADD CONSTRAINT NOT NULL,
> ALTER TABLE ONLY ALTER COLUMN SET NOT NULL
> will error out.
> I am fine with partitioned table behavior.
Yeah, this naughty relationship between ONLY and NO INHERIT is
bothersome and maybe we need to re-examine it.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
#error "Operator lives in the wrong universe"
("Use of cookies in real-time system development", M. Gleixner, M. Mc Guire)
Re: not null constraints, again
On Tue, Oct 1, 2024 at 11:20 PM Alvaro Herrera wrote: > > 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. > after v7, still not bullet-proof. as before, pg_dump/restore will fail for the following: drop table if exists t2, t2_0 create table t2 (a int, b int, c int, constraint foo primary key(a), constraint foo1 not null a no inherit); create table t2_0 (a int constraint foo1 not null no inherit, b int, c int, constraint foo12 primary key(a)); > > + By contrast, a NOT NULL constraint that was > > created > > + as NO INHERIT 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); > > 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? > > with your v7 change, you need remove: > > + By contrast, a NOT NULL constraint that was > > created > > + as NO INHERIT will be changed to a normal > > inheriting > > + one during attach. 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); With V7, we basically cannot change the status of "NO INHERIT". now, we need to drop the not-null constraint foo, recreate a not-null constraint on idxpart0, then attach it to the partitioned table idxpart. imagine a scenario where: At first we didn't know that the NO INHERIT not-null constraint would be attached to a partitioned table. If we want, then we hope attaching it to a partitioned table would be easier. As you can see, v7 will make idxpart0 attach to idxpart quite difficult. - drop table if exists inh_parent,inh_child1,inh_child2; create table inh_parent(f1 int); create table inh_child1(f1 int not null); alter table inh_child1 inherit inh_parent; alter table only inh_parent add constraint nn not null f1; alter table only inh_parent alter column f1 set not null; minor inconsistency, i guess. "alter table only inh_parent add constraint nn not null f1;" will fail. But "alter table only inh_parent alter column f1 set not null;" will not fail, but add a "NOT NULL f1 NO INHERIT" constraint. I thought they should behave the same. for partitioned table now both ALTER TABLE ONLY ADD CONSTRAINT NOT NULL, ALTER TABLE ONLY ALTER COLUMN SET NOT NULL will error out. I am fine with partitioned table behavior.
Re: not null constraints, again
CREATE TABLE a (aa TEXT);
CREATE TEMP TABLE z (b TEXT, UNIQUE(aa, b)) inherits (a);
\d+ z
Table "pg_temp_0.z"
Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
+--+---+--+-+--+-+--+-
aa | text | | not null | | extended |
| |
b | text | | | | extended |
| |
Indexes:
"z_aa_b_key" UNIQUE CONSTRAINT, btree (aa, b)
Not-null constraints:
"z_aa_not_null" NOT NULL "aa"
Inherits: a
Access method: heap
that means in transformIndexConstraint,
the following part only apply to CONSTR_PRIMARY
if (strcmp(key, inhname) == 0)
{
found = true;
typid = inhattr->atttypid;
cxt->nnconstraints =
lappend(cxt->nnconstraints,
makeNotNullConstraint(makeString(pstrdup(inhname;
break;
}
Re: not null constraints, again
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. 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. --- + By contrast, a NOT NULL constraint that was created + as NO INHERIT 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); - 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?
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 NOT NULL constraint that was created > + as NO INHERIT 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 (
Re: not null constraints, again
ATExecDropInherit /* * If the parent has a primary key, then we decrement counts for all NOT * NULL constraints */ ObjectAddressSet(address, RelationRelationId, RelationGetRelid(parent_rel)); only not-null constraint, with ALTER TABLE NO INHERIT we still decrement counts for not-null constraints. I feel the comment is in the wrong place? please check the attached function MergeConstraintsIntoExisting refactoring 1. make it error check more confined within CONSTRAINT_CHECK and CONSTRAINT_NOTNULL. 2. since get_attname will do system cache search, we can just use Relation->rd_att and TupleDescAttr MergeConstraintsIntoExisting.no-cfbot Description: Binary data
Re: not null constraints, again
On 2024-Sep-26, jian he wrote:
> +-- a PK in parent must have a not-null in child that it can mark inherited
> +create table inh_parent (a int primary key);
> +create table inh_child (a int primary key);
> +alter table inh_child inherit inh_parent; -- nope
> +alter table inh_child alter a set not null;
> +alter table inh_child inherit inh_parent; -- now it works
> +ERROR: relation "inh_parent" would be inherited from more than once
> in src/test/regress/sql/inherit.sql, the comments at the end of the
> command, seem to conflict with the output?
Outdated, useless -- removed.
> ---
>
> ALTER TABLE ALTER COLUMN SET NOT NULL
> implicitly means
> ALTER TABLE ALTER COLUMN SET NOT NULL NO INHERIT.
>
> So in ATExecSetNotNull
> if (conForm->connoinherit && recurse)
> ereport(ERROR,
> errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot change NO INHERIT status of NOT
> NULL constraint \"%s\" on relation \"%s\"",
>NameStr(conForm->conname),
>RelationGetRelationName(rel)));
> should be
> if (conForm->connoinherit)
> ereport(ERROR,
> errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("cannot change NO INHERIT status of NOT
> NULL constraint \"%s\" on relation \"%s\"",
>NameStr(conForm->conname),
>RelationGetRelationName(rel)));
>
> then we can avoid the weird case like below:
>
> drop table if exists pp1;
> create table pp1 (f1 int not null no inherit);
> ALTER TABLE pp1 ALTER f1 SET NOT NULL;
> ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;
Hmm, I don't understand why you say SET NOT NULL implicitly means SET
NOT NULL NO INHERIT. That's definitely not the intention. As I
explained earlier, the normal state is that a constraint is inheritable,
so if you do SET NOT NULL you want that constraint to be INHERIT.
Anyway, I don't see what you see as weird in the commands you list. To
me it reacts like this:
=# create table pp1 (f1 int not null no inherit);
CREATE TABLE
=# ALTER TABLE pp1 ALTER f1 SET NOT NULL;
ERROR: cannot change NO INHERIT status of NOT NULL constraint
"pp1_f1_not_null" on relation "pp1"
=# ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;
ALTER TABLE
=# \d+ pp1
Tabla «public.pp1»
Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión │ Almacenamiento │
Compresión │ Estadísticas │ Descripción
─┼─┼──┼──┼─┼┼┼──┼─
f1 │ integer │ │ not null │ │ plain │
│ │
Not-null constraints:
"pp1_f1_not_null" NOT NULL "f1" NO INHERIT
Método de acceso: heap
which seems to be exactly what we want.
> ---
>
> + else if (rel->rd_rel->relhassubclass &&
> + find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL)
> + {
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("not-null constraint on column \"%s\" must be removed in
> child tables too",
> + colName),
> + errhint("Do not specify the ONLY keyword."));
> + }
> this part in ATExecDropNotNull is not necessary?
>
> per alter_table.sql
> <<-->>
> -- make sure we can drop a constraint on the parent but it remains on the
> child
> CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL));
> CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent);
> ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT
> "test_drop_constr_parent_c_check";
> <<-->>
> by the same way, we can drop a not-null constraint ONLY on the parent,
> but it remains on the child.
> if we not remove the above part then
> ALTER TABLE ONLY DROP CONSTRAINT
> will behave differently from
> ALTER TABLE ONLY ALTER COLUMN DROP NOT NULL.
>
> example:
> drop table pp1,cc1, cc2;
> create table pp1 (f1 int not null);
> create table cc1 (f2 text, f3 int) inherits (pp1);
> create table cc2(f4 float) inherits(pp1,cc1);
>
> alter table only pp1 drop constraint pp1_f1_not_null; --works.
> alter table only pp1 alter column f1 drop not null; --- error, should also
> work.
> ---
Hmm. I'm not sure I like this behavior, but there is precedent in
CHECK, and since DROP CONSTRAINT also already works that way, I suppose
DROP NOT NULL should do that too. I'll get it changed.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I must say, I am absolutely impressed with what pgsql's implementation of
VALUES allows me to do. It's kind of ridiculous how much "work" goes away in
my code. Too bad I can't do this at work
Re: not null constraints, again
+-- a PK in parent must have a not-null in child that it can mark inherited
+create table inh_parent (a int primary key);
+create table inh_child (a int primary key);
+alter table inh_child inherit inh_parent; -- nope
+alter table inh_child alter a set not null;
+alter table inh_child inherit inh_parent; -- now it works
+ERROR: relation "inh_parent" would be inherited from more than once
in src/test/regress/sql/inherit.sql, the comments at the end of the
command, seem to conflict with the output?
---
ALTER TABLE ALTER COLUMN SET NOT NULL
implicitly means
ALTER TABLE ALTER COLUMN SET NOT NULL NO INHERIT.
So in ATExecSetNotNull
if (conForm->connoinherit && recurse)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change NO INHERIT status of NOT
NULL constraint \"%s\" on relation \"%s\"",
NameStr(conForm->conname),
RelationGetRelationName(rel)));
should be
if (conForm->connoinherit)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot change NO INHERIT status of NOT
NULL constraint \"%s\" on relation \"%s\"",
NameStr(conForm->conname),
RelationGetRelationName(rel)));
then we can avoid the weird case like below:
drop table if exists pp1;
create table pp1 (f1 int not null no inherit);
ALTER TABLE pp1 ALTER f1 SET NOT NULL;
ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;
---
+ else if (rel->rd_rel->relhassubclass &&
+ find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL)
+ {
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("not-null constraint on column \"%s\" must be removed in
child tables too",
+ colName),
+ errhint("Do not specify the ONLY keyword."));
+ }
this part in ATExecDropNotNull is not necessary?
per alter_table.sql
<<-->>
-- make sure we can drop a constraint on the parent but it remains on the child
CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL));
CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent);
ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT
"test_drop_constr_parent_c_check";
<<-->>
by the same way, we can drop a not-null constraint ONLY on the parent,
but it remains on the child.
if we not remove the above part then
ALTER TABLE ONLY DROP CONSTRAINT
will behave differently from
ALTER TABLE ONLY ALTER COLUMN DROP NOT NULL.
example:
drop table pp1,cc1, cc2;
create table pp1 (f1 int not null);
create table cc1 (f2 text, f3 int) inherits (pp1);
create table cc2(f4 float) inherits(pp1,cc1);
alter table only pp1 drop constraint pp1_f1_not_null; --works.
alter table only pp1 alter column f1 drop not null; --- error, should also work.
---
Re: not null constraints, again
Please check the attached minor doc changes.
make the create_foreign_table.sgml, alter_foreign_table.sgml
not-null description
consistent with normal tables.
change
doc/src/sgml/ref/create_table.sgml
Parameters section
from
NOT NULL
to
NOT NULL [ NO INHERIT ] .
in doc/src/sgml/ref/alter_table.sgml
Adding a constraint recurses only for CHECK constraints
that are not marked NO INHERIT.
This sentence needs to be rephrased to:
Adding a constraint recurses for CHECK and
NOT NULL constraints
that are not marked NO INHERIT.
diff --git a/doc/src/sgml/ref/alter_foreign_table.sgml b/doc/src/sgml/ref/alter_foreign_table.sgml
index 3cb6f08fcf..115ca5c3d7 100644
--- a/doc/src/sgml/ref/alter_foreign_table.sgml
+++ b/doc/src/sgml/ref/alter_foreign_table.sgml
@@ -173,7 +173,7 @@ ALTER FOREIGN TABLE [ IF EXISTS ] name
This form adds a new constraint to a foreign table, using the same
syntax as CREATE FOREIGN TABLE.
- Currently only CHECK constraints are supported.
+ Currently both CHECK and NOT NULL constraints are supported.
diff --git a/doc/src/sgml/ref/create_foreign_table.sgml b/doc/src/sgml/ref/create_foreign_table.sgml
index dc4b907599..a73b6212ff 100644
--- a/doc/src/sgml/ref/create_foreign_table.sgml
+++ b/doc/src/sgml/ref/create_foreign_table.sgml
@@ -43,7 +43,7 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] table_name
where column_constraint is:
[ CONSTRAINT constraint_name ]
-{ NOT NULL |
+{ NOT NULL [ NO INHERIT ] |
NULL |
CHECK ( expression ) [ NO INHERIT ] |
DEFAULT default_expr |
@@ -52,6 +52,7 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] table_name
and table_constraint is:
[ CONSTRAINT constraint_name ]
+ NOT NULL column_name [ NO INHERIT ] |
CHECK ( expression ) [ NO INHERIT ]
and partition_bound_spec is:
@@ -203,10 +204,10 @@ WITH ( MODULUS numeric_literal, REM
-NOT NULL
+NOT NULL [ NO INHERIT ]
- The column is not allowed to contain null values.
+ The column is not allowed to contain null values. NO INHERIT makes the not-null constraint not propagate to child tables.
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index f9a098e77a..3d3a84a405 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -61,7 +61,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
where column_constraint is:
[ CONSTRAINT constraint_name ]
-{ NOT NULL |
+{ NOT NULL [ NO INHERIT ] |
NULL |
CHECK ( expression ) [ NO INHERIT ] |
DEFAULT default_expr |
@@ -815,10 +815,11 @@ WITH ( MODULUS numeric_literal, REM
-NOT NULL
+NOT NULL [ NO INHERIT ]
The column is not allowed to contain null values.
+ NO INHERIT makes the not-null constraint not propagate to child tables.
Re: not null constraints, again
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.
>
> However, a column can have at most one explicit not-null constraint.
>
> 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 c
Re: not null constraints, again
in ATExecSetNotNull
/*
* If we find an appropriate constraint, we're almost done, but just
* need to change some properties on it: if we're recursing, increment
* coninhcount; if not, set conislocal if not already set.
*/
if (recursing)
{
conForm->coninhcount++;
changed = true;
}
else if (!conForm->conislocal)
{
conForm->conislocal = true;
changed = true;
elog(INFO, "constraint islocal attribute changed");
}
if (recursing && !conForm->conislocal)
elog(INFO, "should not happenX");
"should not happenX" appeared in regression.diff, but not
"constraint islocal attribute changed"
Does that mean the IF, ELSE IF logic is not right?
in doc/src/sgml/ref/create_table.sgml
[ NO INHERIT ]
can apply to
table_constraint
and
column_constraint
so we should change create_table.sgml
accordingly?
Re: not null constraints, again
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.
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?
However, a column can have at most one explicit not-null constraint.
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"
/*
* 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"
/*
* 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
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;
Re: not null constraints, again
On 2024-Sep-24, jian he wrote: > sql-createtable.html > SECTION: LIKE source_table [ like_option ... ] > INCLUDING CONSTRAINTS > CHECK constraints will be copied. No distinction is made between > column constraints and table constraints. Not-null constraints are > always copied to the new table. > > drop table if exists t, t_1,ssa; > create table t(a int, b int, not null a no inherit); > create table ssa (like t INCLUDING all); > > Here create table like won't include no inherit not-null constraint, > seems to conflict with the doc? Hmm, actually I think this is a bug, because if you have CHECK constraint with NO INHERIT, it will be copied: create table t (a int check (a > 0) no inherit); create table ssa (like t including constraints); 55490 18devel 141626=# \d+ ssa Tabla «public.ssa» Columna │ Tipo │ Ordenamiento │ Nulable │ Por omisión │ Almacenamiento> ─┼─┼──┼─┼─┼───> a │ integer │ │ │ │ plain > Restricciones CHECK: "t_a_check" CHECK (a > 0) NO INHERIT Método de acceso: heap It seems that NOT NULL constraint should behave the same as CHECK constraints in this regard, i.e., we should not heed NO INHERIT in this case. I have made these changes and added some tests, and will be posting a v5 shortly. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ really, I see PHP as like a strange amalgamation of C, Perl, Shell inflex: you know that "amalgam" means "mixture with mercury", more or less, right? i.e., "deadly poison"
Re: not null constraints, again
On 2024-Sep-24, jian he wrote:
> static void
> set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse,
>LOCKMODE lockmode)
> {
> What do you think of the above refactor?
> (I intentionally deleted empty new line)
Looks nicer ... but you know what? After spending some more time with
it, I realized that one caller is dead code (in
AttachPartitionEnsureIndexes) and another caller doesn't need to ask for
recursion, because it recurses itself (in ATAddCheckNNConstraint). So
that leaves us with a grand total of zero callers that need the
recursion here ... which means we can simplify it to the case that it
only examines a single relation and never recurses.
So I've stripped it down to its bare minimum:
/*
* Helper to set pg_attribute.attnotnull if it isn't set, and to tell phase 3
* to verify it.
*
* When called to alter an existing table, 'wqueue' must be given so that we
* can queue a check that existing tuples pass the constraint. When called
* from table creation, 'wqueue' should be passed as NULL.
*/
static void
set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum,
LOCKMODE lockmode)
{
Oid reloid = RelationGetRelid(rel);
HeapTuple tuple;
Form_pg_attribute attForm;
CheckAlterTableIsSafe(rel);
tuple = SearchSysCacheCopyAttNum(reloid, attnum);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for attribute %d of relation
%u",
attnum, reloid);
attForm = (Form_pg_attribute) GETSTRUCT(tuple);
if (!attForm->attnotnull)
{
Relationattr_rel;
attr_rel = table_open(AttributeRelationId, RowExclusiveLock);
attForm->attnotnull = true;
CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
if (wqueue && !NotNullImpliedByRelConstraints(rel, attForm))
{
AlteredTableInfo *tab;
tab = ATGetQueueEntry(wqueue, rel);
tab->verify_new_notnull = true;
}
CommandCounterIncrement();
table_close(attr_rel, RowExclusiveLock);
}
heap_freetuple(tuple);
}
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: not null constraints, again
On 2024-Sep-24, jian he wrote:
> static Oid
> StoreRelNotNull(Relation rel, const char *nnname, AttrNumber attnum,
> bool is_validated, bool is_local, int inhcount,
> bool is_no_inherit)
> {
> OidconstrOid;
> Assert(attnum > InvalidAttrNumber);
> constrOid =
> CreateConstraintEntry(nnname,
> RelationGetNamespace(rel),
> CONSTRAINT_NOTNULL,
> false,
> false,
> is_validated
>
> }
> is is_validated always true, can we add an Assert on it?
Sure. FWIW the reason it's a parameter at all, is that the obvious next
patch is to add support for NOT VALID constraints. I don't want to
introduce support for NOT VALID immediately with the first patch because
I'm sure some wrinkles will appear; but a followup patch will surely
follow shortly.
> in AddRelationNotNullConstraints
> for (int outerpos = 0; outerpos < list_length(old_notnulls); outerpos++)
> {
> }
> CookedConstraint struct already has "int inhcount;"
> can we rely on that, rather than using add_inhcount?
> we can also add an Assert: "Assert(!cooked->is_no_inherit);"
I'm not sure that works, because if your parent has two parents, you
don't want to add two -- you still have only one immediate parent.
I think the best way to check for correctness is to set up a scenario
where you would have that cooked->inhcount=2 (using whatever CREATE
TABLEs are necessary) and then see if ALTER TABLE NO INHERIT reach the
correct count (0) when all [immediate] parents are detached. But
anyway, keep in mind that inhcount keeps the number of _immediate_
parents, not the number of ancestors.
> /*
> * Remember primary key index, if any. We do this only if the index
> * is valid; but if the table is partitioned, then we do it even if
> * it's invalid.
> *
> * The reason for returning invalid primary keys for foreign tables is
> * because of pg_dump of NOT NULL constraints, and the fact that PKs
> * remain marked invalid until the partitions' PKs are attached to it.
> * If we make rd_pkindex invalid, then the attnotnull flag is reset
> * after the PK is created, which causes the ALTER INDEX ATTACH
> * PARTITION to fail with 'column ... is not marked NOT NULL'. With
> * this, dropconstraint_internal() will believe that the columns must
> * not have attnotnull reset, so the PKs-on-partitions can be attached
> * correctly, until finally the PK-on-parent is marked valid.
> *
> * Also, this doesn't harm anything, because rd_pkindex is not a
> * "real" index anyway, but a RELKIND_PARTITIONED_INDEX.
> */
> if (index->indisprimary &&
> (index->indisvalid ||
> relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
> {
> pkeyIndex = index->indexrelid;
> pkdeferrable = !index->indimmediate;
> }
> The comment (especially paragraph "The reason for returning invalid
> primary keys") is overwhelming.
> Can you also add some sql examples into the comments.
> I guess some sql examples, people can understand it more easily?
Ooh, thanks for catching this -- this comment is a leftover from
previous idea that you could have PKs without NOT NULL. I think it
mostly needs to be removed, and maybe the whole "if" clause put back to
its original form.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)
Re: not null constraints, again
static Oid
StoreRelNotNull(Relation rel, const char *nnname, AttrNumber attnum,
bool is_validated, bool is_local, int inhcount,
bool is_no_inherit)
{
OidconstrOid;
Assert(attnum > InvalidAttrNumber);
constrOid =
CreateConstraintEntry(nnname,
RelationGetNamespace(rel),
CONSTRAINT_NOTNULL,
false,
false,
is_validated
}
is is_validated always true, can we add an Assert on it?
in AddRelationNotNullConstraints
for (int outerpos = 0; outerpos < list_length(old_notnulls); outerpos++)
{
}
CookedConstraint struct already has "int inhcount;"
can we rely on that, rather than using add_inhcount?
we can also add an Assert: "Assert(!cooked->is_no_inherit);"
I've put these points into a patch,
please check the attached.
/*
* Remember primary key index, if any. We do this only if the index
* is valid; but if the table is partitioned, then we do it even if
* it's invalid.
*
* The reason for returning invalid primary keys for foreign tables is
* because of pg_dump of NOT NULL constraints, and the fact that PKs
* remain marked invalid until the partitions' PKs are attached to it.
* If we make rd_pkindex invalid, then the attnotnull flag is reset
* after the PK is created, which causes the ALTER INDEX ATTACH
* PARTITION to fail with 'column ... is not marked NOT NULL'. With
* this, dropconstraint_internal() will believe that the columns must
* not have attnotnull reset, so the PKs-on-partitions can be attached
* correctly, until finally the PK-on-parent is marked valid.
*
* Also, this doesn't harm anything, because rd_pkindex is not a
* "real" index anyway, but a RELKIND_PARTITIONED_INDEX.
*/
if (index->indisprimary &&
(index->indisvalid ||
relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
{
pkeyIndex = index->indexrelid;
pkdeferrable = !index->indimmediate;
}
The comment (especially paragraph "The reason for returning invalid
primary keys") is overwhelming.
Can you also add some sql examples into the comments.
I guess some sql examples, people can understand it more easily?
AddRelationNotNullConstraints.no-cfbot
Description: Binary data
Re: not null constraints, again
On Fri, Sep 20, 2024 at 8:08 PM Alvaro Herrera wrote:
>
> On 2024-Sep-20, jian he wrote:
>
> > about set_attnotnull.
> >
> > we can make set_attnotnull look less recursive.
> > instead of calling find_inheritance_children,
> > let's just one pass, directly call find_all_inheritors
> > overall, I think it would be more intuitive.
> >
> > please check the attached refactored set_attnotnull.
> > regress test passed, i only test regress.
>
> Hmm, what do we gain from doing this change? It's longer in number of
> lines of code, and it's not clear to me that it is simpler.
>
static void
set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse,
LOCKMODE lockmode)
{
HeapTupletuple;
Form_pg_attribute attForm;
boolchanged = false;
List *all_oids;
Relationthisrel;
AttrNumberchildattno;
const char *attrname;
CheckAlterTableIsSafe(rel);
attrname = get_attname(RelationGetRelid(rel), attnum, false);
if (recurse)
all_oids = find_all_inheritors(RelationGetRelid(rel), lockmode,
NULL);
else
all_oids = list_make1_int(RelationGetRelid(rel));
foreach_oid(reloid, all_oids)
{
thisrel = table_open(reloid, NoLock);
if (reloid != RelationGetRelid(rel))
CheckAlterTableIsSafe(thisrel);
childattno = get_attnum(reloid, attrname);
tuple = SearchSysCacheCopyAttNum(reloid, childattno);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for attribute %d of relation %s",
attnum, RelationGetRelationName(thisrel));
attForm = (Form_pg_attribute) GETSTRUCT(tuple);
if (!attForm->attnotnull)
{
Relationattr_rel;
attr_rel = table_open(AttributeRelationId, RowExclusiveLock);
attForm->attnotnull = true;
CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
table_close(attr_rel, RowExclusiveLock);
if (wqueue && !NotNullImpliedByRelConstraints(thisrel, attForm))
{
AlteredTableInfo *tab;
tab = ATGetQueueEntry(wqueue, thisrel);
tab->verify_new_notnull = true;
}
changed = true;
}
if (changed)
CommandCounterIncrement();
changed = false;
table_close(thisrel, NoLock);
}
}
What do you think of the above refactor?
(I intentionally deleted empty new line)
Re: not null constraints, again
On Sat, Sep 21, 2024 at 5:15 AM Alvaro Herrera wrote:
>
> Okay, so here is v4 with these problems fixed, including correct
> propagation of constraint names to children tables, which I had
> inadvertently broken earlier. This one does pass the pg_upgrade tests
> and as far as I can see pg_dump does all the correct things also. I
> cleaned up the tests to remove everything that's unneeded, redundant, or
> testing behavior that no longer exists.
>
in findNotNullConstraintAttnum
if (con->contype != CONSTRAINT_NOTNULL)
continue;
if (!con->convalidated)
continue;
if con->convalidated is false, then we have a bigger problem?
maybe we can change to ERROR to expose/capture potential problems.
like:
if (con->contype != CONSTRAINT_NOTNULL)
continue;
if (!con->convalidated)
elog(ERROR, "not-null constraint is not validated");
----
HeapTuple
findNotNullConstraint(Oid relid, const char *colname)
{
AttrNumberattnum = get_attnum(relid, colname);
return findNotNullConstraintAttnum(relid, attnum);
}
we can change to
HeapTuple
findNotNullConstraint(Oid relid, const char *colname)
{
AttrNumberattnum = get_attnum(relid, colname);
if (attnum <= InvalidAttrNumber)
return NULL;
return findNotNullConstraintAttnum(relid, attnum);
}
----
sql-createtable.html
SECTION: LIKE source_table [ like_option ... ]
INCLUDING CONSTRAINTS
CHECK constraints will be copied. No distinction is made between
column constraints and table constraints. Not-null constraints are
always copied to the new table.
drop table if exists t, t_1,ssa;
create table t(a int, b int, not null a no inherit);
create table ssa (like t INCLUDING all);
Here create table like won't include no inherit not-null constraint,
seems to conflict with the doc?
----
drop table if exists t, t_1;
create table t(a int primary key, b int, not null a no inherit);
create table t_1 () inherits (t);
t_1 will inherit the not-null constraint from t,
so the syntax "not null a no inherit" information is ignored.
other cases:
create table t(a int not null, b int, not null a no inherit);
create table t(a int not null no inherit, b int, not null a);
seems currently, column constraint have not-null constraint, then use
it and table constraint (not-null)
are ignored.
but if column constraint don't have not-null then according to table constraint.
Re: not null constraints, again
Alvaro Herrera 于2024年9月21日周六 05:15写道： > On 2024-Sep-20, Alvaro Herrera wrote: > > > Yeah, there's a bunch of conflicts in current master. I rebased > > yesterday but I'm still composing the email for v4. Coming soon. > > Okay, so here is v4 with these problems fixed, including correct > propagation of constraint names to children tables, which I had > inadvertently broken earlier. This one does pass the pg_upgrade tests > and as far as I can see pg_dump does all the correct things also. I > cleaned up the tests to remove everything that's unneeded, redundant, or > testing behavior that no longer exists. > > I changed the behavior of ALTER TABLE ONLY ADD PRIMARY KEY, so > that it throws error in case a child does not have a NOT NULL constraint > on one of the columns, rather than silently creating such a constraint. > (This is how `master` currently behaves). I think this is better > behavior, because it lets the user decide whether they want to scan the > table to create that constraint or not. It's a bit crude at present, > because (1) a child could have a NO INHERIT constraint and have further > children, which would foil the check (I think changing > find_inheritance_children to find_all_inheritors would be sufficient to > fix this, but that's only needed in legacy inheritance not > partitioning); (2) the error message doesn't have an errcode, and the > wording might need work. > The indexing test case in regress failed with v4 patch. alter table only idxpart add primary key (a); -- fail, no not-null constraint -ERROR: column a of table idxpart0 is not marked not null +ERROR: column "a" of table "idxpart0" is not marked NOT NULL It seemed the error message forgot to change. -- Thanks, Tender Wang https://www.openpie.com/
Re: not null constraints, again
On 2024-Sep-20, jian he wrote: > about set_attnotnull. > > we can make set_attnotnull look less recursive. > instead of calling find_inheritance_children, > let's just one pass, directly call find_all_inheritors > overall, I think it would be more intuitive. > > please check the attached refactored set_attnotnull. > regress test passed, i only test regress. Hmm, what do we gain from doing this change? It's longer in number of lines of code, and it's not clear to me that it is simpler. > I am also beginning to wonder if ATExecSetNotNull inside can also call > find_all_inheritors. The point of descending levels one by one in ATExecSetNotNull is that we can stop for any child on which a constraint already exists. We don't need to scan any children thereof, which saves work. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Use it up, wear it out, make it do, or do without"
Re: not null constraints, again
> > We only have this Synopsis
> > ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL
>
> Yeah, this syntax is intended to add a "normal" not-null constraint,
> i.e. one that inherits.
>
> > --tests from src/test/regress/sql/inherit.sql
> > CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT);
> > ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
> > current fail at ATExecSetNotNull
> > ERROR: cannot change NO INHERIT status of NOT NULL constraint
> > "inh_nn_parent_a_not_null" on relation "inh_nn_parent"
>
> This is correct, because here you want a normal not-null constraint but
> the table already has the weird ones that don't inherit.
>
i found a case,that in a sense kind of support to make it a no-op.
no-op means, if this attribute is already not-null, ALTER column SET NOT NULL;
won't have any effect.
or maybe there is a bug somewhere.
drop table if exists pp1;
create table pp1 (f1 int not null no inherit);
ALTER TABLE pp1 ALTER f1 SET NOT NULL;
ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;
There is no child table, no partition, just a single regular table.
so, in this case, with or without ONLY should behave the same?
now "ALTER TABLE ONLY" works, "ALTER TABLE" error out.
per sql-altertable.html:
name
The name (optionally schema-qualified) of an existing table to alter.
If ONLY is specified before the table name, only that table is
altered. If ONLY is not specified, the table and all its descendant
tables (if any) are altered.
diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index 93b3f664f2..57c4ecd93a 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -77,6 +77,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } |
UNLOGGED ] TABLE [ IF NOT EXI
[ CONSTRAINT constraint_name ]
{ CHECK ( expression ) [
NO INHERIT ] |
+ NOT NULL column_name [
NO INHERIT ] |
UNIQUE [ NULLS [ NOT ] DISTINCT ] ( column_name [, ... ] ) index_parameters |
PRIMARY KEY ( column_name [, ... ] ) index_parameters |
EXCLUDE [ USING index_method ] ( exclude_element WITH operator [, ... ] ) index_parameters [ WHERE (
predicate ) ] |
we can
create table pp1 (f1 int not null no inherit);
create table pp1 (f1 int, constraint nn not null f1 no inherit);
"NO INHERIT" should be applied for column_constraint and table_constraint?
Re: not null constraints, again
about set_attnotnull.
we can make set_attnotnull look less recursive.
instead of calling find_inheritance_children,
let's just one pass, directly call find_all_inheritors
overall, I think it would be more intuitive.
please check the attached refactored set_attnotnull.
regress test passed, i only test regress.
I am also beginning to wonder if ATExecSetNotNull inside can also call
find_all_inheritors.
static void
set_attnotnull(List **wqueue, Relation rel, AttrNumber attnum, bool recurse,
LOCKMODE lockmode)
{
HeapTuple tuple;
Form_pg_attribute attForm;
bool changed = false;
tuple = SearchSysCacheCopyAttNum(RelationGetRelid(rel), attnum);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for attribute %d of relation %u",
attnum, RelationGetRelid(rel));
attForm = (Form_pg_attribute) GETSTRUCT(tuple);
if (!attForm->attnotnull)
{
Relation attr_rel;
attr_rel = table_open(AttributeRelationId, RowExclusiveLock);
attForm->attnotnull = true;
CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
table_close(attr_rel, RowExclusiveLock);
/*
* And set up for existing values to be checked, unless another
* constraint already proves this.
*/
if (wqueue && !NotNullImpliedByRelConstraints(rel, attForm))
{
AlteredTableInfo *tab;
tab = ATGetQueueEntry(wqueue, rel);
tab->verify_new_notnull = true;
}
changed = true;
}
if (recurse)
{
List *child_oids;
Relation childrel;
AttrNumber childattno;
HeapTuple rel_tuple;
Form_pg_attribute child_attForm;
const char *attrname;
attrname = get_attname(RelationGetRelid(rel), attnum, false);
child_oids = find_all_inheritors(RelationGetRelid(rel), lockmode,
NULL);
if (changed)
CommandCounterIncrement();
foreach_oid(childrelid, child_oids)
{
/* we alerady dealt with parent rel in above */
if (childrelid == RelationGetRelid(rel))
continue;
childrel = table_open(childrelid, NoLock);
CheckAlterTableIsSafe(childrel);
childattno = get_attnum(childrelid, attrname);
rel_tuple = SearchSysCacheCopyAttNum(childrelid, childattno);
if (!HeapTupleIsValid(rel_tuple))
elog(ERROR, "cache lookup failed for attribute %d of relation %s",
attnum, RelationGetRelationName(childrel));
child_attForm = (Form_pg_attribute) GETSTRUCT(rel_tuple);
if (!child_attForm->attnotnull)
{
Relation attr_rel;
attr_rel = table_open(AttributeRelationId, RowExclusiveLock);
child_attForm->attnotnull = true;
CatalogTupleUpdate(attr_rel, &rel_tuple->t_self, rel_tuple);
table_close(attr_rel, RowExclusiveLock);
if (wqueue && !NotNullImpliedByRelConstraints(childrel, child_attForm))
{
AlteredTableInfo *tab;
tab = ATGetQueueEntry(wqueue, childrel);
tab->verify_new_notnull = true;
}
changed = true;
}
changed = false;
table_close(childrel, NoLock);
}
}
}
Re: not null constraints, again
On 2024-Sep-20, Tender Wang wrote: > By the way, the v3 failed applying on Head(d35e293878) > git am v3-0001-Catalog-not-null-constraints.patch > Applying: Catalog not-null constraints > error: patch failed: doc/src/sgml/ref/create_table.sgml:77 Yeah, there's a bunch of conflicts in current master. I rebased yesterday but I'm still composing the email for v4. Coming soon. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "En las profundidades de nuestro inconsciente hay una obsesiva necesidad de un universo lógico y coherente. Pero el universo real se halla siempre un paso más allá de la lógica" (Irulan)
Re: not null constraints, again
On 2024-Sep-20, Tender Wang wrote:
> jian he 于2024年9月20日周五 11:34写道：
>
> > another bug.
> > I will dig later, just want to share it first.
> >
> > minimum producer:
> > drop table if exists pp1,cc1, cc2,cc3;
> > create table pp1 (f1 int );
> > create table cc1 () inherits (pp1);
> > create table cc2() inherits(pp1,cc1);
> > create table cc3() inherits(pp1,cc1,cc2);
> >
> > alter table pp1 alter f1 set not null;
> > ERROR: tuple already updated by self
>
> I guess some place needs call CommandCounterIncrement().
Yeah ... this fixes it:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 579b8075b5..3f66e43b9a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7877,12 +7877,6 @@ ATExecSetNotNull(List **wqueue, Relation rel, char
*conName, char *colName,
{
List *children;
- /*
-* Make previous addition visible, in case we process the same
-* relation again while chasing down multiple inheritance trees.
-*/
- CommandCounterIncrement();
-
children = find_inheritance_children(RelationGetRelid(rel),
lockmode);
@@ -7890,6 +7884,8 @@ ATExecSetNotNull(List **wqueue, Relation rel, char
*conName, char *colName,
{
Relationchildrel = table_open(childoid, NoLock);
+ CommandCounterIncrement();
+
ATExecSetNotNull(wqueue, childrel, conName, colName,
recurse, true,
lockmode);
table_close(childrel, NoLock);
I was trying to save on the number of CCIs that we perform, but it's
likely not a wise expenditure of time given that this isn't a very
common scenario anyway. (Nobody with thousands of millions of children
tables will try to run thousands of commands in a single transaction
anyway ... so saving a few increments doesn't make any actual
difference. If such people exist, they can show us their use case and
we can investigate and fix it then.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"This is what I like so much about PostgreSQL. Most of the surprises
are of the "oh wow! That's cool" Not the "oh shit!" kind. :)"
Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php
Re: not null constraints, again
By the way, the v3 failed applying on Head(d35e293878) git am v3-0001-Catalog-not-null-constraints.patch Applying: Catalog not-null constraints error: patch failed: doc/src/sgml/ref/create_table.sgml:77 error: doc/src/sgml/ref/create_table.sgml: patch does not apply error: patch failed: src/backend/commands/tablecmds.c:4834 error: src/backend/commands/tablecmds.c: patch does not apply error: patch failed: src/backend/parser/gram.y:4141 error: src/backend/parser/gram.y: patch does not apply error: patch failed: src/backend/parser/parse_utilcmd.c:2385 error: src/backend/parser/parse_utilcmd.c: patch does not apply Patch failed at 0001 Catalog not-null constraints Alvaro Herrera 于2024年9月17日周二 01:47写道： > Sadly, there were some other time-wasting events that I failed to > consider, but here's now v3 which has fixed (AFAICS) all the problems > you reported. > > On 2024-Sep-11, jian he wrote: > > > after applying your changes. > > > > in ATExecAddConstraint, ATAddCheckNNConstraint. > > ATAddCheckNNConstraint(wqueue, tab, rel, > > newConstraint, recurse, false, is_readd, > > lockmode); > > if passed to ATAddCheckNNConstraint rel is a partitioned table. > > ATAddCheckNNConstraint itself can recurse to create not-null > pg_constraint > > for itself and it's partitions (children table). > > This is fine as long as we only call ATExecAddConstraint once. > > > > but ATExecAddConstraint itself will recurse, it will call > > the partitioned table and each of the partitions. > > Yeah, this is because ATPrepAddPrimaryKey was queueing SetNotNull nodes > for each column on each children, which is repetitive and causes the > problem you see. That was a leftover from the previous way we handled > PKs; we no longer need it to work that way. I have changed it so that > it queues one constraint addition per column, on the same table that > receives the PK. It now works correctly as far as I can tell. > > Sadly, there's one more pg_dump issue, which causes the pg_upgrade tests > to fail. The problem is that if you have this sequence (taken from > constraints.sql): > > CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED); > CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS > (notnull_tbl4); > > this is dumped by pg_dump in this other way: > > CREATE TABLE public.notnull_tbl4 (a integer NOT NULL); > CREATE TABLE public.notnull_tbl4_cld2 () INHERITS (public.notnull_tbl4); > ALTER TABLE ONLY public.notnull_tbl4_cld2 ADD CONSTRAINT > notnull_tbl4_cld2_pkey PRIMARY KEY (a) DEFERRABLE; > ALTER TABLE ONLY public.notnull_tbl4 ADD CONSTRAINT notnull_tbl4_pkey > PRIMARY KEY (a) DEFERRABLE INITIALLY DEFERRED; > > This is almost exactly the same, except that the PK for > notnull_tbl4_cld2 is created in a separate command ... and IIUC this > causes the not-null constraint to obtain a different name, or a > different inheritance characteristic, and then from the > restored-by-pg_upgrade database, it's dumped by pg_dump separately. > This is what causes the pg_upgrade test to fail. > > Anyway, this made me realize that there is a more general problem, to > wit, that pg_dump is not dumping not-null constraint names correctly -- > sometimes they just not dumped, which is Not Good. I'll have to look > into that once more. > > > (Also: there are still a few additional test stanzas in regress/ that > ought to be removed; also, I haven't re-tested sepgsql, so it's probably > broken ATM.) > > -- > Álvaro Herrera PostgreSQL Developer — > https://www.EnterpriseDB.com/ > -- Thanks, Tender Wang
Re: not null constraints, again
jian he 于2024年9月20日周五 11:34写道： > another bug. > I will dig later, just want to share it first. > > minimum producer: > drop table if exists pp1,cc1, cc2,cc3; > create table pp1 (f1 int ); > create table cc1 () inherits (pp1); > create table cc2() inherits(pp1,cc1); > create table cc3() inherits(pp1,cc1,cc2); > > alter table pp1 alter f1 set not null; > ERROR: tuple already updated by self > I guess some place needs call CommandCounterIncrement(). -- Thanks, Tender Wang
Re: not null constraints, again
another bug. I will dig later, just want to share it first. minimum producer: drop table if exists pp1,cc1, cc2,cc3; create table pp1 (f1 int ); create table cc1 () inherits (pp1); create table cc2() inherits(pp1,cc1); create table cc3() inherits(pp1,cc1,cc2); alter table pp1 alter f1 set not null; ERROR: tuple already updated by self
Re: not null constraints, again
On Thu, Sep 19, 2024 at 4:26 PM Alvaro Herrera wrote:
>
>
> > drop table if exists idxpart, idxpart0, idxpart1 cascade;
> > create table idxpart (a int) partition by range (a);
> > create table idxpart0 (a int primary key);
> > alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> > alter table idxpart alter column a set not null;
> > alter table idxpart0 alter column a drop not null;
> > alter table idxpart0 drop constraint idxpart0_a_not_null;
> >
> > "alter table idxpart0 alter column a drop not null;"
> > is logically equivalent to
> > "alter table idxpart0 drop constraint idxpart0_a_not_null;"
> >
> > the first one (alter column) ERROR out,
> > the second success.
> > the second "drop constraint" should also ERROR out?
> > since it violates the sentence in ddl-partitioning.html
> > "You cannot drop a NOT NULL constraint on a partition's column if the
> > same constraint is present in the parent table."
>
> Yeah, I modified this code already a few days ago, and now it does error
> out like this
>
> ERROR: cannot drop inherited constraint "idxpart0_a_not_null" of relation
> "idxpart0"
>
> Anyway, as I mentioned back then, the DROP CONSTRAINT didn't _actually_
> remove the constraint; it only marked the constraint as no longer
> locally defined (conislocal=false), which had no practical effect other
> than changing the representation during pg_dump. Even detaching the
> partition after having "dropped" the constraint would make the not-null
> constraint appear again as coninhcount=0,conislocal=true rather than
> drop it.
>
funny.
as the previously sql example, if you execute
"alter table idxpart0 drop constraint idxpart0_a_not_null;"
again
then
ERROR: cannot drop inherited constraint "idxpart0_a_not_null" of
relation "idxpart0"
I am not sure if that's logically OK or if the user can deduce the
logic from the manual.
like, the first time you use "alter table drop constraint"
to drop a constraint, the constraint is not totally dropped,
the second time you execute it again the constraint cannot be dropped directly.
i think the issue is the changes we did in dropconstraint_internal
in dropconstraint_internal, we have:
---
if (con->contype == CONSTRAINT_NOTNULL &&
con->conislocal && con->coninhcount > 0)
{
HeapTuplecopytup;
copytup = heap_copytuple(constraintTup);
con = (Form_pg_constraint) GETSTRUCT(copytup);
con->conislocal = false;
CatalogTupleUpdate(conrel, ©tup->t_self, copytup);
ObjectAddressSet(conobj, ConstraintRelationId, con->oid);
CommandCounterIncrement();
table_close(conrel, RowExclusiveLock);
return conobj;
}
/* Don't allow drop of inherited constraints */
if (con->coninhcount > 0 && !recursing)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("cannot drop inherited constraint \"%s\" of
relation \"%s\"",
constrName, RelationGetRelationName(rel;
---
comments in dropconstraint_internal
"* Reset pg_constraint.attnotnull, if this is a not-null constraint."
should be
"pg_attribute.attnotnull"
also, we don't have tests for not-null constraint similar to check
constraint tests on
src/test/regress/sql/alter_table.sql (line 2067 to line 2073)
Re: not null constraints, again
On 2024-Sep-19, jian he wrote:
> still based on v3.
> in src/sgml/html/ddl-partitioning.html
> << Both CHECK and NOT NULL constraints of a partitioned table are always
> inherited by all its partitions.
> CHECK constraints that are marked NO INHERIT are not allowed to be
> created on partitioned tables.
> You cannot drop a NOT NULL constraint on a partition's column if the
> same constraint is present in the parent table.
> << we can change
> "CHECK constraints that are marked NO INHERIT are not allowed to be
> created on partitioned tables."
> to
> "CHECK and NOT NULL constraints that are marked NO INHERIT are not
> allowed to be created on partitioned tables."
Right. Your proposed text is correct but sounds a bit repetitive with
the phrase just prior, and also the next one about inability to drop a
NOT NULL applies equally to CHECK constraints; so I modified the whole
paragraph to this:
Both CHECK and NOT NULL
constraints of a partitioned table are always inherited by all its
partitions; it is not allowed to create NO INHERIT
constraints of those types.
You cannot drop a constraint of those types if the same constraint
is present in the parent table.
> in sql-altertable.html we have:
> << ATTACH PARTITION partition_name { FOR VALUES partition_bound_spec | DEFAULT }
> If any of the CHECK constraints of the table being attached are marked
> NO INHERIT, the command will fail; such constraints must be recreated
> without the NO INHERIT clause.
> <<
> create table idxpart (a int constraint nn not null) partition by range (a);
> create table idxpart0 (a int constraint nn not null no inherit);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
>
> In the above sql query case,
> we changed a constraint ("nn" on idxpart0) connoinherit attribute
> after ATTACH PARTITION.
> (connoinherit from true to false)
> Do we need extra sentences to explain it?
> here not-null constraint behavior seems to divert from CHECK constraint.
Ah, yeah, the docs are misleading: we do allow these constraints to
mutate from NO INHERIT to INHERIT. There's no danger in this, because
such a table cannot have children: no inheritance children (because
inheritance-parent tables cannot be partitions) and no partitions
either, because partitioned tables are not allowed to have NOT NULL NO INHERIT
constraints. So this can only happen on a standalone table, and thus
changing the existing not-null constraint from NO INHERIT to normal does
no harm.
I think we could make CHECK behave the same way on this point; but in the
meantime, I propose this text:
If any of the CHECK constraints of the table being
attached are marked NO INHERIT, the command will fail;
such constraints must be recreated without the
NO INHERIT clause.
By contrast, a NOT NULL constraint that was created
as NO INHERIT will be changed to a normal inheriting
one during attach.
> drop table if exists idxpart, idxpart0, idxpart1 cascade;
> create table idxpart (a int) partition by range (a);
> create table idxpart0 (a int primary key);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> alter table idxpart alter column a set not null;
> alter table idxpart0 alter column a drop not null;
> alter table idxpart0 drop constraint idxpart0_a_not_null;
>
> "alter table idxpart0 alter column a drop not null;"
> is logically equivalent to
> "alter table idxpart0 drop constraint idxpart0_a_not_null;"
>
> the first one (alter column) ERROR out,
> the second success.
> the second "drop constraint" should also ERROR out?
> since it violates the sentence in ddl-partitioning.html
> "You cannot drop a NOT NULL constraint on a partition's column if the
> same constraint is present in the parent table."
Yeah, I modified this code already a few days ago, and now it does error
out like this
ERROR: cannot drop inherited constraint "idxpart0_a_not_null" of relation
"idxpart0"
Anyway, as I mentioned back then, the DROP CONSTRAINT didn't _actually_
remove the constraint; it only marked the constraint as no longer
locally defined (conislocal=false), which had no practical effect other
than changing the representation during pg_dump. Even detaching the
partition after having "dropped" the constraint would make the not-null
constraint appear again as coninhcount=0,conislocal=true rather than
drop it.
Speaking of pg_dump, I'm still on the nightmarish trip to get it to
behave correctly for all cases (esp. for pg_upgrade). It seems I
tripped up on my own code from the previous round, having
under-commented and misunderstood it :-(
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)
Re: not null constraints, again
still based on v3. in src/sgml/html/ddl-partitioning.html <<
Re: not null constraints, again
On Tue, Sep 17, 2024 at 1:47 AM Alvaro Herrera wrote:
>
still digging inheritance related issues.
drop table if exists pp1,cc1, cc2;
create table pp1 (f1 int, constraint nn check (f1 > 1));
create table cc1 (f2 text, f3 int ) inherits (pp1);
create table cc2(f4 float, constraint nn check (f1 > 1)) inherits(pp1,cc1);
execute constr_meta('{pp1,cc1, cc2}');
alter table only cc2 drop constraint nn;
ERROR: cannot drop inherited constraint "nn" of relation "cc2"
So:
drop table if exists pp1,cc1, cc2;
create table pp1 (f1 int not null);
create table cc1 (f2 text, f3 int not null no inherit) inherits (pp1);
create table cc2(f4 float, f1 int not null) inherits(pp1,cc1);
execute constr_meta('{pp1,cc1, cc2}');
alter table only cc2 drop constraint cc2_f1_not_null;
Last "alter table only cc2" should fail?
because it violates catalog-pg-constraint coninhcount description:
"The number of direct inheritance ancestors this constraint has. A
constraint with a nonzero number of ancestors cannot be dropped nor
renamed."
also
alter table only cc2 drop constraint cc2_f1_not_null;
success executed.
some pg_constraint attribute info may change.
but constraint cc2_f1_not_null still exists.
Re: not null constraints, again
On 2024-Sep-12, jian he wrote:
> ---exampleA
> drop table pp1,cc1, cc2;
> create table pp1 (f1 int);
> create table cc1 (f2 text, f3 int) inherits (pp1);
> create table cc2(f4 float) inherits(pp1,cc1);
> alter table pp1 alter column f1 set not null;
> execute constr_meta('{pp1,cc1, cc2}');
>
> ---exampleB
> drop table pp1,cc1, cc2;
> create table pp1 (f1 int not null);
> create table cc1 (f2 text, f3 int) inherits (pp1);
> create table cc2(f4 float) inherits(pp1,cc1);
> execute constr_meta('{pp1,cc1, cc2}');
>
> Should exampleA and exampleB
> return same pg_constraint->coninhcount
> for not-null constraint "cc2_f1_not_null"
> ?
Yes, they should be identical. In this case example A is in the wrong,
the constraint in cc2 should have inhcount=2 (which example B has) and
it has inhcount=1. This becomes obvious if you do ALTER TABLE NO
INHERIT of both parents -- in example A, it fails the second one with
ERROR: relation 43823 has non-inherited constraint "cc2_f1_not_null"
because the inhcount was set wrong by SET NOT NULL. Will fix. (I think
the culprit is the "readyRels" stuff I had added -- I should nuke that
and add a CommandCounterIncrement in the right spot ...)
> We only have this Synopsis
> ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL
Yeah, this syntax is intended to add a "normal" not-null constraint,
i.e. one that inherits.
> --tests from src/test/regress/sql/inherit.sql
> CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT);
> ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
> current fail at ATExecSetNotNull
> ERROR: cannot change NO INHERIT status of NOT NULL constraint
> "inh_nn_parent_a_not_null" on relation "inh_nn_parent"
This is correct, because here you want a normal not-null constraint but
the table already has the weird ones that don't inherit.
> seems we translate
> ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
> to
> ALTER TABLE inh_nn_parent ALTER a SET NOT NULL INHERIT
Well, we don't "translate" it as such. It's just what's normal.
> but we cannot (syntax error)
> ALTER TABLE inh_nn_parent ALTER a SET NOT NULL NO INHERIT;
I don't feel a need to support this syntax. You can do with with the
ADD CONSTRAINT syntax if you need it.
> /*
> - * Return the address of the modified column. If the column was already NOT
> - * NULL, InvalidObjectAddress is returned.
> + * ALTER TABLE ALTER COLUMN SET NOT NULL
> + *
> + * Add a not-null constraint to a single table and its children. Returns
> + * the address of the constraint added to the parent relation, if one gets
> + * added, or InvalidObjectAddress otherwise.
> + *
> + * We must recurse to child tables during execution, rather than using
> + * ALTER TABLE's normal prep-time recursion.
> */
> static ObjectAddress
> -ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
> - const char *colName, LOCKMODE lockmode)
> +ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName,
> + bool recurse, bool recursing, List **readyRels,
> + LOCKMODE lockmode)
>
> you introduced two boolean "recurse", "recursing", don't have enough
> explanation.
> That is too much to comprehend.
Apologies. I think it's a well-established pattern in tablecmds.c.
"bool recurse" is for the caller (ATRewriteCatalogs) to request
recursion. "bool recursing" is for the function informing itself that
it is calling itself recursively, i.e. "I'm already recursing". This is
mostly (?) used to skip things like permission checks.
> " * We must recurse to child tables during execution, rather than using
> " * ALTER TABLE's normal prep-time recursion.
> What does this sentence mean for these two boolean "recurse", "recursing"?
Here "recurse during execution" means ALTER TABLE's phase 2, that is,
ATRewriteCatalogs (which means some ATExecFoo function needs to
implement recursion internally), and "normal prep-time recursion" means
the recursion set up by phase 1 (ATPrepCmd), which creates separate
AlterTableCmd nodes for each child table. See the comments for
AlterTable and the code for ATController.
> Finally, I did some cosmetic changes, also improved error message
> in MergeConstraintsIntoExisting
Thanks, will incorporate.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL. This is by far the most pleasant management experience of
any database I've worked on." (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
Re: not null constraints, again
> > On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera
> > wrote:
> > >
> > > Hello, here's a v2 of this patch. I have fixed --I think-- all the
> > > issues you and Tender Wang reported (unless I declined a fix in some
> > > previous email).
> > >
PREPARE constr_meta (name[]) AS
with cte as
(
select con.oid as conoid, conrelid::regclass, pc.relkind as relkind,
con.coninhcount as inhcnt
,con.conname, con.contype, con.connoinherit as noinherit
,con.conislocal as islocal, pa.attname, pa.attnum
from pg_constraint con join pg_class pc on pc.oid = con.conrelid
join pg_attribute pa on pa.attrelid = pc.oid
and pa.attnum = any(conkey)
where con.contype in ('n', 'p', 'c') and
pc.relname = ANY ($1)
)
select pg_get_constraintdef(conoid), * from cte
order by contype, inhcnt, islocal, attnum;
The above constr_meta is used to query meta info, nothing fancy.
---exampleA
drop table pp1,cc1, cc2;
create table pp1 (f1 int);
create table cc1 (f2 text, f3 int) inherits (pp1);
create table cc2(f4 float) inherits(pp1,cc1);
alter table pp1 alter column f1 set not null;
execute constr_meta('{pp1,cc1, cc2}');
---exampleB
drop table pp1,cc1, cc2;
create table pp1 (f1 int not null);
create table cc1 (f2 text, f3 int) inherits (pp1);
create table cc2(f4 float) inherits(pp1,cc1);
execute constr_meta('{pp1,cc1, cc2}');
Should exampleA and exampleB
return same pg_constraint->coninhcount
for not-null constraint "cc2_f1_not_null"
?
We only have this Synopsis
ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL
--tests from src/test/regress/sql/inherit.sql
CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT);
ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
current fail at ATExecSetNotNull
ERROR: cannot change NO INHERIT status of NOT NULL constraint
"inh_nn_parent_a_not_null" on relation "inh_nn_parent"
seems we translate
ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
to
ALTER TABLE inh_nn_parent ALTER a SET NOT NULL INHERIT
but we cannot (syntax error)
ALTER TABLE inh_nn_parent ALTER a SET NOT NULL NO INHERIT;
In this case, why not make it no-op, this column "a" already NOT NULL.
so ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL
will change not-null information, no need to consider other not-null
related information.
/*
- * Return the address of the modified column. If the column was already NOT
- * NULL, InvalidObjectAddress is returned.
+ * ALTER TABLE ALTER COLUMN SET NOT NULL
+ *
+ * Add a not-null constraint to a single table and its children. Returns
+ * the address of the constraint added to the parent relation, if one gets
+ * added, or InvalidObjectAddress otherwise.
+ *
+ * We must recurse to child tables during execution, rather than using
+ * ALTER TABLE's normal prep-time recursion.
*/
static ObjectAddress
-ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
- const char *colName, LOCKMODE lockmode)
+ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName,
+ bool recurse, bool recursing, List **readyRels,
+ LOCKMODE lockmode)
you introduced two boolean "recurse", "recursing", don't have enough
explanation.
That is too much to comprehend.
" * We must recurse to child tables during execution, rather than using
" * ALTER TABLE's normal prep-time recursion.
What does this sentence mean for these two boolean "recurse", "recursing"?
Finally, I did some cosmetic changes, also improved error message
in MergeConstraintsIntoExisting
v2-0001-minor-coesmetic-change-for-not-null-patch-v2.no-cfbot
Description: Binary data
v2-0002-imporve-error-message-in-MergeConstraintsIntoE.no-cfbot
Description: Binary data
Re: not null constraints, again
On 2024-Sep-11, jian he wrote:
> On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera
> wrote:
> >
> > Hello, here's a v2 of this patch. I have fixed --I think-- all the
> > issues you and Tender Wang reported (unless I declined a fix in some
> > previous email).
> >
>
> + /*
> + * The constraint must appear as inherited in children, so create a
> + * modified constraint object to use.
> + */
> + constr = copyObject(constr);
> + constr->inhcount = 1;
>
> in ATAddCheckNNConstraint, we don't need the above copyObject call.
> because at the beginning of ATAddCheckNNConstraint, we do
> newcons = AddRelationNewConstraints(rel, NIL,
> list_make1(copyObject(constr)),
> recursing || is_readd,/*
> allow_merge */
> !recursing, /* is_local */
> is_readd,/* is_internal */
> NULL);/* queryString not available
> * here */
I'm disinclined to change this. The purpose of creating a copy at this
point is to avoid modifying an object that doesn't belong to us. It
doesn't really matter much that we have an additional copy anyway; this
isn't in any way performance-critical or memory-intensive.
> create table idxpart (a int) partition by range (a);
> create table idxpart0 (like idxpart);
> alter table idxpart0 add primary key (a);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> alter table idxpart add primary key (a);
>
> alter table idxpart0 DROP CONSTRAINT idxpart0_pkey;
> alter table idxpart0 DROP CONSTRAINT idxpart0_a_not_null;
>
> First DROP CONSTRAINT failed as the doc said,
> but the second success.
> but the second DROP CONSTRAINT should fail?
> Even if you drop success, idxpart0_a_not_null still exists.
> it also conflicts with the pg_constraint I've quoted above.
Hmm, this is because we allow a DROP CONSTRAINT to set conislocal from
true to false. So the constraint isn't *actually* dropped. If you try
the DROP CONSTRAINT a second time, you'll get an error then. Maybe I
should change the order of checks here, so that we forbid doing the
conislocal change; that would be more consistent with what we document.
I'm undecided about this TBH -- maybe I should reword the documentation
you cite in a different way.
> transformTableLikeClause, expandTableLikeClause
> can be further simplified when the relation don't have not-null as all like:
>
> /*
> * Reproduce not-null constraints by copying them. This doesn't require
> * any option to have been given.
> */
> if (tupleDesc->constr && tupleDesc->constr->has_not_null)
> {
> lst = RelationGetNotNullConstraints(RelationGetRelid(relation),
> false);
> cxt->nnconstraints = list_concat(cxt->nnconstraints, lst);
> }
True.
> Also, seems AdjustNotNullInheritance never being called/used?
Oh, right, I removed the last callsite recently. I'll remove the
function, and rename AdjustNotNullInheritance1 to
AdjustNotNullInheritance now that that name is free.
Thanks for reviewing! I'll handle your other comment tomorrow.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: not null constraints, again
On Wed, Sep 11, 2024 at 9:11 AM jian he wrote: > > On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera > wrote: > > > > Hello, here's a v2 of this patch. I have fixed --I think-- all the > > issues you and Tender Wang reported (unless I declined a fix in some > > previous email). > > after applying your changes. in ATExecAddConstraint, ATAddCheckNNConstraint. ATAddCheckNNConstraint(wqueue, tab, rel, newConstraint, recurse, false, is_readd, lockmode); if passed to ATAddCheckNNConstraint rel is a partitioned table. ATAddCheckNNConstraint itself can recurse to create not-null pg_constraint for itself and it's partitions (children table). This is fine as long as we only call ATExecAddConstraint once. but ATExecAddConstraint itself will recurse, it will call the partitioned table and each of the partitions. The first time ATExecAddConstraint with a partitioned table with the following calling chain ATAddCheckNNConstraint-> AddRelationNewConstraints -> AdjustNotNullInheritance1 works fine. the second time ATExecAddConstraint with the partitions ATAddCheckNNConstraint-> AddRelationNewConstraints -> AdjustNotNullInheritance1 AdjustNotNullInheritance1 will make the partitions pg_constraint->coninhcount bigger than 1. for example: drop table if exists idxpart, idxpart0, idxpart1 cascade; create table idxpart (a int) partition by range (a); create table idxpart0 (a int primary key); alter table idxpart attach partition idxpart0 for values from (0) to (1000); alter table idxpart add primary key (a); After the above query pg_constraint->coninhcount of idxpart0_a_not_null becomes 2. but it should be 1 ?
Re: not null constraints, again
On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera wrote: > > Hello, here's a v2 of this patch. I have fixed --I think-- all the > issues you and Tender Wang reported (unless I declined a fix in some > previous email). > + /* + * The constraint must appear as inherited in children, so create a + * modified constraint object to use. + */ + constr = copyObject(constr); + constr->inhcount = 1; in ATAddCheckNNConstraint, we don't need the above copyObject call. because at the beginning of ATAddCheckNNConstraint, we do newcons = AddRelationNewConstraints(rel, NIL, list_make1(copyObject(constr)), recursing || is_readd,/* allow_merge */ !recursing, /* is_local */ is_readd,/* is_internal */ NULL);/* queryString not available * here */ pg_constraint manual <<constr->has_not_null) { lst = RelationGetNotNullConstraints(RelationGetRelid(relation), false); cxt->nnconstraints = list_concat(cxt->nnconstraints, lst); } we can do: create table parent (a text, b int); create table child () inherits (parent); alter table child no inherit parent; so comments in AdjustNotNullInheritance * AdjustNotNullInheritance *Adjust not-null constraints' inhcount/islocal for *ALTER TABLE [NO] INHERITS "ALTER TABLE [NO] INHERITS" should be "ALTER TABLE ALTER COLUMN [NO] INHERITS" ? Also, seems AdjustNotNullInheritance never being called/used?
Re: not null constraints, again
On 2024-Sep-02, Tender Wang wrote: > The attached patch adds List *nnconstraints, which store the not-null > definition, in struct CreateStmt. This makes me a little confused > about List *constraints in struct CreateStmt. Actually, the List > constraints store ckeck constraint, and it will be better if the > comments can reflect that. Re-naming it to List *ckconstraints seems > more reasonable. But a lot of codes that use stmt->constraints will be > changed. Well, if you look at the comment about CreateStmt, there's this: /* -- * Create Table Statement * * NOTE: in the raw gram.y output, ColumnDef and Constraint nodes are * intermixed in tableElts, and constraints and nnconstraints are NIL. After * parse analysis, tableElts contains just ColumnDefs, nnconstraints contains * Constraint nodes of CONSTR_NOTNULL type from various sources, and * constraints contains just CONSTR_CHECK Constraint nodes. * -- */ So if we were to rename 'constraints' to 'ckconstraints', it would no longer reflect the fact that not-null ones can be in the former list initially. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "No hay ausente sin culpa ni presente sin disculpa" (Prov. francés)
Re: not null constraints, again
On 2024-Sep-09, jian he wrote:
> bold idea. print out the constraint name: violates not-null constraint \"%s\"
> for the following code:
> ereport(ERROR,
> (errcode(ERRCODE_NOT_NULL_VIOLATION),
> errmsg("null value in column \"%s\" of
> relation \"%s\" violates not-null constraint",
> NameStr(att->attname),
> RelationGetRelationName(orig_rel)),
> val_desc ? errdetail("Failing row contains
> %s.", val_desc) : 0,
> errtablecol(orig_rel, attrChk)));
I gave this a quick run, but I'm not sure it actually improves things
much. Here's one change from the regression tests. What do you think?
INSERT INTO reloptions_test VALUES (1, NULL), (NULL, NULL);
-ERROR: null value in column "i" of relation "reloptions_test" violates
not-null constraint
+ERROR: null value in column "i" of relation "reloptions_test" violates
not-null constraint "reloptions_test_i_not_null"
What do I get from having the constraint name? It's not like I'm going
to drop the constraint and retry the insert.
Here's the POC-quality patch for this. I changes a lot of regression
tests, which I don't patch here. (But that's not the reason for me
thinking that this isn't worth it.)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 29e186fa73d..d84137f4f43 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1907,6 +1907,7 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
* have been converted from the original input tuple after tuple routing.
* 'resultRelInfo' is the final result relation, after tuple routing.
*/
+#include "catalog/pg_constraint.h"
void
ExecConstraints(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
@@ -1932,6 +1933,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
char *val_desc;
Relationorig_rel = rel;
TupleDesc orig_tupdesc =
RelationGetDescr(rel);
+ char *conname;
/*
* If the tuple has been routed, it's been
converted to the
@@ -1970,14 +1972,24 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
tupdesc,
modifiedCols,
64);
+ {
+ HeapTuple tuple;
+ Form_pg_constraint conForm;
+
+ tuple =
findNotNullConstraintAttnum(RelationGetRelid(orig_rel),
+
att->attnum);
+ conForm = (Form_pg_constraint)
GETSTRUCT(tuple);
+ conname =
pstrdup(NameStr(conForm->conname));
+ }
ereport(ERROR,
-
(errcode(ERRCODE_NOT_NULL_VIOLATION),
-errmsg("null value in column
\"%s\" of relation \"%s\" violates not-null constraint",
-
NameStr(att->attname),
-
RelationGetRelationName(orig_rel)),
-val_desc ? errdetail("Failing
row contains %s.", val_desc) : 0,
-errtablecol(orig_rel,
attrChk)));
+
errcode(ERRCODE_NOT_NULL_VIOLATION),
+ errmsg("null value in column
\"%s\" of relation \"%s\" violates not-null constraint \"%s\"",
+
NameStr(att->attname),
+
RelationGetRelationName(orig_rel),
+ conname),
+ val_desc ? errdetail("Failing
row contains %s.", val_desc) : 0,
+ errtablecol(orig_rel, attrChk));
}
}
}
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: not null constraints, again
jian he 于2024年9月9日周一 16:31写道： > On Mon, Sep 2, 2024 at 6:33 PM Tender Wang wrote: > > > > > > > > The attached patch adds List *nnconstraints, which store the not-null > definition, in struct CreateStmt. > > This makes me a little confused about List *constraints in struct > CreateStmt. Actually, the List constraints > > store ckeck constraint, and it will be better if the comments can > reflect that. Re-naming it to List *ckconstraints > > seems more reasonable. But a lot of codes that use stmt->constraints > will be changed. > > > hi. > seems you forgot to attach the patch? > I also noticed this minor issue. > I have no preference for Renaming it to List *ckconstraints. > +1 for better comments. maybe reword to > >>> > List *constraints;/* CHECK constraints (list of Constraint > nodes) */ > >>> > I just gave advice; whether it is accepted or not, it's up to Alvaro. If Alvaro agrees with the advice, he will patch a new one. We can continue to review the new patch. If Alvaro disagrees, he doesn't need to change the current patch. I think this way will be more straightforward for others who will review this feature. > > On Tue, Sep 3, 2024 at 3:17 PM Tender Wang wrote: > > > > The test case in constraints.sql, as below: > > CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL); > > > ^^ > > There are two not-null definitions, and is the second one redundant? > > > > hi. > i think this is ok. please see > function transformColumnDefinition and variable saw_nullable. > Yeah, it is ok. > we need to make sure this: > CREATE TABLE notnull_tbl3 (a INTEGER NULL NOT NULL); > fails. > > > of course, it's also OK do this: > CREATE TABLE notnull_tbl3 (a INTEGER NULL NULL); > -- Thanks, Tender Wang
Re: not null constraints, again
On Mon, Sep 2, 2024 at 6:33 PM Tender Wang wrote: > > > > The attached patch adds List *nnconstraints, which store the not-null > definition, in struct CreateStmt. > This makes me a little confused about List *constraints in struct CreateStmt. > Actually, the List constraints > store ckeck constraint, and it will be better if the comments can reflect > that. Re-naming it to List *ckconstraints > seems more reasonable. But a lot of codes that use stmt->constraints will be > changed. > hi. seems you forgot to attach the patch? I also noticed this minor issue. I have no preference for Renaming it to List *ckconstraints. +1 for better comments. maybe reword to >>> List *constraints;/* CHECK constraints (list of Constraint nodes) */ >>> On Tue, Sep 3, 2024 at 3:17 PM Tender Wang wrote: > > The test case in constraints.sql, as below: > CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL); > > ^^ > There are two not-null definitions, and is the second one redundant? > hi. i think this is ok. please see function transformColumnDefinition and variable saw_nullable. we need to make sure this: CREATE TABLE notnull_tbl3 (a INTEGER NULL NOT NULL); fails. of course, it's also OK do this: CREATE TABLE notnull_tbl3 (a INTEGER NULL NULL);
Re: not null constraints, again
On Sat, Aug 31, 2024 at 11:59 AM Alvaro Herrera wrote:
>
> Hello
>
> Here I present another attempt at making not-null constraints be
> catalogued. This is largely based on the code reverted at 9ce04b50e120,
> except that we now have a not-null constraint automatically created for
> every column of a primary key, and such constraint cannot be removed
> while the PK exists. Thanks to this, a lot of rather ugly code is gone,
> both in pg_dump and in backend -- in particular the handling of NO
> INHERIT, which was needed for pg_dump.
>
> Noteworthy psql difference: because there are now even more not-null
> constraints than before, the \d+ display would be far too noisy if we
> just let it grow. So here I've made it omit any constraints that
> underlie the primary key. This should be OK since you can't do much
> with those constraints while the PK is still there. If you drop the PK,
> the next \d+ will show those constraints.
>
hi.
my brief review.
create table t1(a int, b int, c int not null, primary key(a, b));
\d+ t1
ERROR: operator is not unique: smallint[] <@ smallint[]
LINE 8: coalesce(NOT ARRAY[at.attnum] <@ (SELECT conkey FROM pg_cata...
^
HINT: Could not choose a best candidate operator. You might need to
add explicit type casts.
the regression test still passed, i have no idea why.
anyway, the following changes make the above ERROR disappear.
also seems more lean.
printfPQExpBuffer(&buf,
/* FIXME the coalesce trick looks silly. What's a better way? */
"SELECT co.conname, at.attname, co.connoinherit, co.conislocal,\n"
"co.coninhcount <> 0\n"
"FROM pg_catalog.pg_constraint co JOIN\n"
"pg_catalog.pg_attribute at ON\n"
"(at.attrelid = co.conrelid AND at.attnum = co.conkey[1])\n"
"WHERE co.contype = 'n' AND\n"
"co.conrelid = '%s'::pg_catalog.regclass AND\n"
"coalesce(NOT ARRAY[at.attnum] <@ (SELECT conkey FROM
pg_catalog.pg_constraint\n"
" WHERE contype = 'p' AND conrelid = '%s'::regclass), true)\n"
"ORDER BY at.attnum",
oid,
oid);
change to
printfPQExpBuffer(&buf,
"SELECT co.conname, at.attname,
co.connoinherit, co.conislocal,\n"
"co.coninhcount <> 0\n"
"FROM pg_catalog.pg_constraint co JOIN\n"
"pg_catalog.pg_attribute at ON\n"
"(at.attrelid = co.conrelid AND
at.attnum = co.conkey[1])\n"
"WHERE co.contype = 'n' AND\n"
"co.conrelid = '%s'::pg_catalog.regclass AND\n"
"NOT EXISTS (SELECT 1 FROM
pg_catalog.pg_constraint co1 where co1.contype = 'p'\n"
"AND at.attnum = any(co1.conkey) AND
co1.conrelid = '%s'::pg_catalog.regclass)\n"
"ORDER BY at.attnum",
oid,
oid);
steal idea from https://stackoverflow.com/a/75614278/15603477
create type comptype as (r float8, i float8);
create domain dcomptype1 as comptype not null no inherit;
with cte as (
SELECT oid, conrelid::regclass, conname FROM pg_catalog.pg_constraint
where contypid in ('dcomptype1'::regtype))
select pg_get_constraintdef(oid) from cte;
current output is
NOT NULL
but it's not the same as
CREATE TABLE ATACC1 (a int, not null a no inherit);
with cte as ( SELECT oid, conrelid::regclass, conname FROM
pg_catalog.pg_constraint
where conrelid in ('ATACC1'::regclass))
select pg_get_constraintdef(oid) from cte;
NOT NULL a NO INHERIT
i don't really sure the meaning of "on inherit" in
"create domain dcomptype1 as comptype not null no inherit;"
bold idea. print out the constraint name: violates not-null constraint \"%s\"
for the following code:
ereport(ERROR,
(errcode(ERRCODE_NOT_NULL_VIOLATION),
errmsg("null value in column \"%s\" of
relation \"%s\" violates not-null constraint",
NameStr(att->attname),
RelationGetRelationName(orig_rel)),
val_desc ? errdetail("Failing row contains
%s.", val_desc) : 0,
errtablecol(orig_rel, attrChk)));
in extractNotNullColumn
we can Assert(colnum > 0);
create table t3(a int , b int , c int ,not null a, not null c, not
null b, not null tableoid);
this should not be allowed?
foreach(lc,
RelationGetNotNullConstraints(RelationGetRelid(relation), false))
{
AlterTableCmd *atsubcmd;
atsubcmd = makeNode(AlterTableCmd);
atsubcmd->subtype = AT_AddConstraint;
atsubcmd->def = (Node *) lfirst_node(Constraint, lc);
atsubcmds = lappend(atsubcmds, atsubcmd);
}
forgive me for being hypocritical.
I
Re: not null constraints, again
Alvaro Herrera 于2024年8月31日周六 11:59写道： > Hello > > Here I present another attempt at making not-null constraints be > catalogued. This is largely based on the code reverted at 9ce04b50e120, > except that we now have a not-null constraint automatically created for > every column of a primary key, and such constraint cannot be removed > while the PK exists. Thanks to this, a lot of rather ugly code is gone, > both in pg_dump and in backend -- in particular the handling of NO > INHERIT, which was needed for pg_dump. > > Noteworthy psql difference: because there are now even more not-null > constraints than before, the \d+ display would be far too noisy if we > just let it grow. So here I've made it omit any constraints that > underlie the primary key. This should be OK since you can't do much > with those constraints while the PK is still there. If you drop the PK, > the next \d+ will show those constraints. > > One thing that regretfully I haven't yet had time for, is paring down > the original test code: a lot of it is verifying the old semantics, > particularly for NO INHERIT constraints, which had grown ugly special > cases. It now mostly raises errors; or the tests are simply redundant. > I'm going to remove that stuff as soon as I'm back on my normal work > timezone. > > sepgsql is untested. > > I'm adding this to the September commitfest. > The test case in constraints.sql, as below: CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL); ^^ There are two not-null definitions, and is the second one redundant? When we drop the column not-null constraint, we will enter ATExecDropNotNull(). Then, it calls findNotNullConstraint() to get the constraint tuple. We already have attnum before the call findNotNullConstraint(). Can we directly call findNotNullConstraintAttnum()? In RemoveConstraintById(), I see below comments: "For not-null and primary key constraints, obtain the list of columns affected, so that we can reset their attnotnull flags below." However, there are no related codes that reflect the above comments. -- Thanks, Tender Wang
Re: not null constraints, again
Alvaro Herrera 于2024年8月31日周六 11:59写道： > Hello > > Here I present another attempt at making not-null constraints be > catalogued. This is largely based on the code reverted at 9ce04b50e120, > except that we now have a not-null constraint automatically created for > every column of a primary key, and such constraint cannot be removed > while the PK exists. Thanks to this, a lot of rather ugly code is gone, > both in pg_dump and in backend -- in particular the handling of NO > INHERIT, which was needed for pg_dump. > > Noteworthy psql difference: because there are now even more not-null > constraints than before, the \d+ display would be far too noisy if we > just let it grow. So here I've made it omit any constraints that > underlie the primary key. This should be OK since you can't do much > with those constraints while the PK is still there. If you drop the PK, > the next \d+ will show those constraints. > > One thing that regretfully I haven't yet had time for, is paring down > the original test code: a lot of it is verifying the old semantics, > particularly for NO INHERIT constraints, which had grown ugly special > cases. It now mostly raises errors; or the tests are simply redundant. > I'm going to remove that stuff as soon as I'm back on my normal work > timezone. > > sepgsql is untested. > > I'm adding this to the September commitfest. > The attached patch adds List *nnconstraints, which store the not-null definition, in struct CreateStmt. This makes me a little confused about List *constraints in struct CreateStmt. Actually, the List constraints store ckeck constraint, and it will be better if the comments can reflect that. Re-naming it to List *ckconstraints seems more reasonable. But a lot of codes that use stmt->constraints will be changed. Since AddRelationNewConstraints() can now add not-null column constraint, the comments about AddRelationNewConstraints() should tweak a little. "All entries in newColDefaults will be processed. Entries in newConstraints will be processed only if they are CONSTR_CHECK type." Now, the type of new constraints may be not-null constraints. If the column has already had one not-null constraint, and we add same not-null constraint again. Then the code will call AdjustNotNullInheritance1() in AddRelationNewConstraints(). The comments before entering AdjustNotNullInheritance1() in AddRelationNewConstraints() look confusing to me. Because constraint is not inherited. -- Tender Wang
Re: not null constraints, again
Alvaro Herrera 于2024年8月31日周六 11:59写道： > Hello > > Here I present another attempt at making not-null constraints be > catalogued. This is largely based on the code reverted at 9ce04b50e120, > except that we now have a not-null constraint automatically created for > every column of a primary key, and such constraint cannot be removed > while the PK exists. Thanks to this, a lot of rather ugly code is gone, > both in pg_dump and in backend -- in particular the handling of NO > INHERIT, which was needed for pg_dump. > > Noteworthy psql difference: because there are now even more not-null > constraints than before, the \d+ display would be far too noisy if we > just let it grow. So here I've made it omit any constraints that > underlie the primary key. This should be OK since you can't do much > with those constraints while the PK is still there. If you drop the PK, > the next \d+ will show those constraints. > > One thing that regretfully I haven't yet had time for, is paring down > the original test code: a lot of it is verifying the old semantics, > particularly for NO INHERIT constraints, which had grown ugly special > cases. It now mostly raises errors; or the tests are simply redundant. > I'm going to remove that stuff as soon as I'm back on my normal work > timezone. > > sepgsql is untested. > > I'm adding this to the September commitfest. Thanks for working on this again. AT_PASS_OLD_COL_ATTRS, I didn't see any other code that used it. I don't think it's necessary. I will take the time to look over the attached patch. -- Tender Wang
