Thread: Re: not null constraints, again
On Fri, Oct 4, 2024 at 9:11 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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 <literal>NOT NULL</literal> constraint that was created
as <literal>NO INHERIT</literal> will be changed to a normal inheriting
one during attach.
these text should removed from section:
<<ATTACH PARTITION partition_name { FOR VALUES partition_bound_spec |
DEFAULT }>>
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
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 from pg_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.
sql-altertable.html
<varlistentry id="sql-altertable-desc-set-drop-not-null">
<term><literal>SET</literal>/<literal>DROP NOT NULL</literal></term>
<listitem>
<para>
These forms change whether a column is marked to allow null
values or to reject null values.
</para>
<para>
If this table is a partition, one cannot perform <literal>DROP
NOT NULL</literal>
on a column if it is marked <literal>NOT NULL</literal> in the parent
table. To drop the <literal>NOT NULL</literal> constraint from all the
partitions, perform <literal>DROP NOT NULL</literal> on the parent
table.
</para>
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 <literal>DROP NOT NULL</literal> on the parent table
will not drop the <literal>NOT NULL</literal> constraint from all partitions.
so we need rephrase the following sentence:
To drop the <literal>NOT NULL</literal> constraint from all the
partitions, perform <literal>DROP NOT NULL</literal> on the parent
table.
to address this kind of corner case?
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.
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.
<term><literal>INHERIT <replaceable class="parameter">parent_table</replaceable></literal></term>
<listitem>
<para>
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.
</para>
<para>
In addition, all <literal>CHECK</literal> and <literal>NOT NULL</literal>
constraints on the parent must also exist on the child, except those
marked non-inheritable (that is, created with
<literal>ALTER TABLE ... ADD CONSTRAINT ... NO INHERIT</literal>), which
are ignored. All child-table constraints matched must not be marked
non-inheritable. Currently
<literal>UNIQUE</literal>, <literal>PRIMARY KEY</literal>, and
<literal>FOREIGN KEY</literal> constraints are not considered, but
this might change in the future.
</para>
</listitem>
> ------------------------------------------------
> 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 Herrera Breisgau, 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)
> 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+dGCYZMg@mail.gmail.com
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 Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> 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
alwaystrue 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
On 2024-Nov-08, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> 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
alwaystrue 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"
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)
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.