On Wed, Nov 13, 2024 at 10:49 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > On 2024-Nov-13, Amit Langote wrote: > > > I rebased my patch over 14e87ffa5c54 and noticed that NOT NULL > > constraints can also (or not) be marked NO INHERIT. I think we should > > allow NO INHERIT NOT NULL constraints on leaf partitions just like > > CHECK constraints, so changed AddRelationNotNullConstraints() that way > > and added some tests. > > OK, looks good.
Thanks. I'll hold off pushing until we have some clarity on whether
the behavior should be identical for CHECK and NOT NULL constraints.
For now, I have removed the changes in my patch pertaining to NOT NULL
constraints.
> > However, I noticed that there is no clean way to do that for the following
> > case:
> >
> > ALTER TABLE leaf_partition ADD CONSTRAINT col_nn_ni NOT NULL col NO INHERIT;
>
> Sorry, I didn't understand what's the initial state. Does the
> constraint already exist here or not?
Sorry, here's the full example. Note I'd changed both
AddRelationNotNullConstraints() and AdjustNotNullInheritance() to not
throw an error *if* the table is a leaf partition when the NO INHERIT
of an existing constraint doesn't match that of the new constraint.
create table p (a int not null) partition by list (a);
create table p1 partition of p for values in (1);
alter table p1 add constraint a_nn_ni not null a no inherit;
\d+ p1
Table "public.p1"
Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
a | integer | | not null | | plain |
| |
Partition of: p FOR VALUES IN (1)
Partition constraint: ((a IS NOT NULL) AND (a = 1))
Not-null constraints:
"p_a_not_null" NOT NULL "a" (local, inherited)
Access method: heap
-- noop
alter table p1 add constraint a_nn_ni not null a no inherit;
alter table p1 add constraint a_nn_ni not null a no inherit;
\d+ p1
Table "public.p1"
Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
a | integer | | not null | | plain |
| |
Partition of: p FOR VALUES IN (1)
Partition constraint: ((a IS NOT NULL) AND (a = 1))
Not-null constraints:
"p_a_not_null" NOT NULL "a" (local, inherited)
Access method: heap
The state of the existing inherited constraint is not valid and the
a_nn_ni is never created.
So, it seems that allowing NO INHERIT NOT NULL constraints of leaf
partitions to be merged with the inherited one is not as
straightforward as it is for CHECK constraints.
> > If I change the new function AdjustNotNullInheritance() added by your
> > commit to not throw an error for leaf partitions, the above constraint
> > (col_nn_ni) is not stored at all, because the function returns true in
> > that case, which means the caller does nothing with the constraint. I
> > am not sure what the right thing to do would be. If we return false,
> > the caller will store the constraint the first time around, but
> > repeating the command will again do nothing, not even prevent the
> > addition of a duplicate constraint.
>
> Do you mean if we return false, it allows two not-null constraints for
> the same column? That would absolutely be a bug.
I don't think there is any such bug at the moment, because if
AdjustNotNullInheritance() finds an existing constraint, it always
returns true provided the new constraint does not cause the error due
to its NO INHERIT property not matching the existing one's.
> I think:
> * if a leaf partition already has an inherited not-null constraint
> from its parent and we want to add another one, we should:
> - if the one being added is NO INHERIT, then throw an error, because
> we cannot merge them
Hmm, we'll be doing something else for CHECK constraints if we apply my patch:
create table p (a int not null, constraint check_a check (a > 0))
partition by list (a);
create table p1 partition of p (constraint check_a check (a > 0) no
inherit) for values in (1);
NOTICE: merging constraint "check_a" with inherited definition
\d p1
Table "public.p1"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | not null |
Partition of: p FOR VALUES IN (1)
Check constraints:
"check_a" CHECK (a > 0) NO INHERIT
I thought we were fine with allowing merging of such child
constraints, because leaf partitions can't have children to pass them
onto, so the NO INHERIT clause is essentially pointless.
> - if the one being added is not NO INHERIT, then both constraints
> would have the same state and so we silently do nothing.
Maybe we should emit some kind of NOTICE message in such cases?
> * if a leaf partition has a locally defined not-null marked NO INHERIT
> - if we add another NO INHERIT, silently do nothing
> - if we add an INHERIT one, throw an error because cannot merge.
So IIUC, there cannot be multiple *named* NOT NULL constraints for the
same column?
> If you want, you could leave the not-null constraint case alone and I
> can have a look later. It wasn't my intention to burden you with that.
No worries. I want to ensure that the behaviors for NOT NULL and
CHECK constraints are as consistent as possible.
Anyway, for now, I've fixed my patch to only consider CHECK
constraints -- leaf partitions can have inherited CHECK constraints
that are marked NO INHERIT.
--
Thanks, Amit Langote
v3-0001-Allow-inherited-constraints-to-be-marked-NO-INHER.patch
Description: Binary data
