Thread: doc fail about ALTER TABLE ATTACH re. NO INHERIT
Hello,
While doing final review for not-null constraints, I noticed that the
ALTER TABLE ATTACH PARTITION have this phrase:
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.
However, this is not true and apparently has never been true. I tried
this in both master and pg10:
create table parted (a int) partition by list (a);
create table part1 (a int , check (a > 0) no inherit);
alter table parted attach partition part1 for values in (1);
In both versions (and I imagine all intermediate ones) that sequence
works fine and results in this table:
Table "public.part1"
Column │ Type │ Collation │ Nullable │ Default │ Storage │ Stats target │ Description
────────┼─────────┼───────────┼──────────┼─────────┼─────────┼──────────────┼─────────────
a │ integer │ │ │ │ plain │ │
Partition of: parted FOR VALUES IN (1)
Partition constraint: ((a IS NOT NULL) AND (a = 1))
Check constraints:
"part1_a_check" CHECK (a > 0) NO INHERIT
On the other hand, if we were to throw an error in the ALTER TABLE as
the docs say, it would serve no purpose: the partition cannot have any
more descendants, so the fact that the CHECK constraint is NO INHERIT
makes no difference. So I think the code is fine and we should just fix
the docs.
Note that if you interpret it the other way around, i.e., that the
"table being attached" is the parent table, then the first CREATE
already fails:
create table parted2 (a int check (a > 0) no inherit) partition by list (a);
ERROR: cannot add NO INHERIT constraint to partitioned table "parted2"
This says that we don't need to worry about this condition in the parent
table either.
All in all, I think this text serves no purpose and should be removed
(from all live branches), as in the attached patch.
This text came in with the original partitioning commit f0e44751d717.
CCing Robert and Amit.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."
Hi Alvaro,
On Tue, Nov 5, 2024 at 9:01 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hello,
>
> While doing final review for not-null constraints, I noticed that the
> ALTER TABLE ATTACH PARTITION have this phrase:
>
> 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.
>
> However, this is not true and apparently has never been true. I tried
> this in both master and pg10:
>
> create table parted (a int) partition by list (a);
> create table part1 (a int , check (a > 0) no inherit);
> alter table parted attach partition part1 for values in (1);
>
> In both versions (and I imagine all intermediate ones) that sequence
> works fine and results in this table:
>
> Table "public.part1"
> Column │ Type │ Collation │ Nullable │ Default │ Storage │ Stats target │ Description
> ────────┼─────────┼───────────┼──────────┼─────────┼─────────┼──────────────┼─────────────
> a │ integer │ │ │ │ plain │ │
> Partition of: parted FOR VALUES IN (1)
> Partition constraint: ((a IS NOT NULL) AND (a = 1))
> Check constraints:
> "part1_a_check" CHECK (a > 0) NO INHERIT
>
> On the other hand, if we were to throw an error in the ALTER TABLE as
> the docs say, it would serve no purpose: the partition cannot have any
> more descendants, so the fact that the CHECK constraint is NO INHERIT
> makes no difference. So I think the code is fine and we should just fix
> the docs.
>
>
> Note that if you interpret it the other way around, i.e., that the
> "table being attached" is the parent table, then the first CREATE
> already fails:
>
> create table parted2 (a int check (a > 0) no inherit) partition by list (a);
> ERROR: cannot add NO INHERIT constraint to partitioned table "parted2"
>
> This says that we don't need to worry about this condition in the parent
> table either.
>
> All in all, I think this text serves no purpose and should be removed
> (from all live branches), as in the attached patch.
I think that text might be talking about this case:
create table parent (a int, constraint check_a check (a > 0))
partition by list (a);
create table part1 (a int, constraint check_a check (a > 0) no inherit);
alter table parent attach partition part1 for values in (1);
ERROR: constraint "check_a" conflicts with non-inherited constraint
on child table "part1"
which is due to this code in MergeConstraintsIntoExisting():
/* If the 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))));
that came in with the following commit:
commit 3b11247aadf857bbcbfc765191273973d9ca9dd7
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon Jan 16 19:19:42 2012 -0300
Disallow merging ONLY constraints in children tables
Perhaps the ATTACH PARTITION text should be changed to make clear
which case it's talking about, say, like:
If any of the CHECK constraints of the table being attached are marked
NO INHERIT, the command will fail if a constraint with the same name
exists in the parent table; such constraints must be recreated without
the NO INHERIT clause.
--
Thanks, Amit Langote
On 2024-Nov-06, Amit Langote wrote:
> On Tue, Nov 5, 2024 at 9:01 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > While doing final review for not-null constraints, I noticed that the
> > ALTER TABLE ATTACH PARTITION have this phrase:
> >
> > 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.
> I think that text might be talking about this case:
>
> create table parent (a int, constraint check_a check (a > 0))
> partition by list (a);
> create table part1 (a int, constraint check_a check (a > 0) no inherit);
> alter table parent attach partition part1 for values in (1);
> ERROR: constraint "check_a" conflicts with non-inherited constraint on child table "part1"
Oh, hmm, that makes sense I guess. Still, while this restriction makes
sense for inheritance, it doesn't IMO for partitioned tables. I would
even suggest that we drop enforcement of this restriction during ATTACH.
> Perhaps the ATTACH PARTITION text should be changed to make clear
> which case it's talking about, say, like:
>
> If any of the CHECK constraints of the table being attached are marked
> NO INHERIT, the command will fail if a constraint with the same name
> exists in the parent table; such constraints must be recreated without
> the NO INHERIT clause.
Hmm, not sure about it; I think we're giving too much prominence to a
detail that's not so important that it warrants four extra lines, when
it could be a parenthical note next to the other mention of those
constraints earlier in that paragraph. I suggest something like this:
<para>
A partition using <literal>FOR VALUES</literal> uses same syntax for
<replaceable class="parameter">partition_bound_spec</replaceable> as
<link linkend="sql-createtable"><command>CREATE TABLE</command></link>. The partition bound specification
must correspond to the partitioning strategy and partition key of the
target table. The table to be attached must have all the same columns
as the target table and no more; moreover, the column types must also
match. Also, it must have all the <literal>NOT NULL</literal> and
<literal>CHECK</literal> constraints of the target table. Currently
<literal>FOREIGN KEY</literal> constraints are not considered.
<literal>UNIQUE</literal> and <literal>PRIMARY KEY</literal> constraints
from the parent table will be created in the partition, if they don't
already exist.
If any of the <literal>CHECK</literal> constraints of the table being
attached are marked <literal>NO INHERIT</literal>, the command will fail;
such constraints must be recreated without the
<literal>NO INHERIT</literal> clause.
</para>
I suggest we change it to
<para>
A partition using <literal>FOR VALUES</literal> uses same syntax for
<replaceable class="parameter">partition_bound_spec</replaceable> as
<link linkend="sql-createtable"><command>CREATE TABLE</command></link>. The partition bound specification
must correspond to the partitioning strategy and partition key of the
target table. The table to be attached must have all the same columns
as the target table and no more; moreover, the column types must also
match. Also, it must have all the <literal>NOT NULL</literal> and
<literal>CHECK</literal> constraints of the target table, not marked
<literal>NO INHERIT</literal>. Currently
<literal>FOREIGN KEY</literal> constraints are not considered.
<literal>UNIQUE</literal> and <literal>PRIMARY KEY</literal> constraints
from the parent table will be created in the partition, if they don't
already exist.
</para>
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"You don't solve a bad join with SELECT DISTINCT" #CupsOfFail
https://twitter.com/connor_mc_d/status/1431240081726115845
On 2024-Nov-07, Amit Langote wrote: > On Wed, Nov 6, 2024 at 9:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Oh, hmm, that makes sense I guess. Still, while this restriction makes > > sense for inheritance, it doesn't IMO for partitioned tables. I would > > even suggest that we drop enforcement of this restriction during ATTACH. > > I agree. Since leaf partitions have no children to propagate > constraints to, the NO INHERIT mark shouldn't matter. And partitioned > partitions already disallow NO INHERIT constraints as you mentioned. > > Do you think we should apply something like the attached at least in > the master? I found that a similar restriction exists in the CREATE > TABLE PARTITION OF path too. Yeah, that sounds reasonable. I didn't look at the code in detail, but I'm not sure I understand why you'd change CREATE TABLE PARTITION OF, since the point is that this restriction would apply when you attach a table that already exists, not when you create a new table. Maybe I misunderstand what you're saying though. > +1 Thanks, pushed. > Though if we decide to apply the attached, does the note "not marked > NO INHERIT" become unnecessary? Yes -- I think your patch would have to remove it again. A short-lived note for sure, but I thought it was better to have all branches in the same state, and now you can modify master. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Fri, Nov 8, 2024 at 2:54 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2024-Nov-07, Amit Langote wrote: > > > On Wed, Nov 6, 2024 at 9:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > > Oh, hmm, that makes sense I guess. Still, while this restriction makes > > > sense for inheritance, it doesn't IMO for partitioned tables. I would > > > even suggest that we drop enforcement of this restriction during ATTACH. > > > > I agree. Since leaf partitions have no children to propagate > > constraints to, the NO INHERIT mark shouldn't matter. And partitioned > > partitions already disallow NO INHERIT constraints as you mentioned. > > > > Do you think we should apply something like the attached at least in > > the master? I found that a similar restriction exists in the CREATE > > TABLE PARTITION OF path too. > > Yeah, that sounds reasonable. I didn't look at the code in detail, but > I'm not sure I understand why you'd change CREATE TABLE PARTITION OF, > since the point is that this restriction would apply when you attach a > table that already exists, not when you create a new table. Maybe I > misunderstand what you're saying though. The restriction also exists in the CREATE TABLE PARTITION OF path: create table parted_parent (a int, constraint check_a check (a > 0)) partition by list (a); -- leaf partition create table parted_parent_part1 partition of parted_parent (constraint check_a check(a > 0) no inherit) for values in (1); ERROR: constraint "check_a" conflicts with inherited constraint on relation "parted_parent_part1" -- non-leaf partition postgres=# create table parted_parent_part1 partition of parted_parent (constraint check_a check(a > 0) no inherit) for values in (1) partition by list (a); ERROR: constraint "check_a" conflicts with inherited constraint on relation "parted_parent_part1" I think we can remove the restriction at least for the leaf partition case just like in the ATTACH PARTITION path. > > Though if we decide to apply the attached, does the note "not marked > > NO INHERIT" become unnecessary? > > Yes -- I think your patch would have to remove it again. A short-lived > note for sure, but I thought it was better to have all branches in the > same state, and now you can modify master. Ok, got it. -- Thanks, Amit Langote
On 2024-Nov-11, Amit Langote wrote: > The restriction also exists in the CREATE TABLE PARTITION OF path: > > [...] > > I think we can remove the restriction at least for the leaf partition > case just like in the ATTACH PARTITION path. Sure, WFM. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
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.
> 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?
> 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 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
- if the one being added is not NO INHERIT, then both constraints
would have the same state and so we silently do nothing.
* 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.
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.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.
On 2024-Nov-14, Amit Langote wrote: > 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; Yeah, I think this behavior is bogus, because the user wants to have something (a constraint that will not inherit) but they cannot have it, because there is already a constraint that will inherit. The current behavior of throwing an error seems correct to me. With your patch, what this does is mark the constraint as "local" in addition to inherited, but that'd be wrong, because the constraint the user wanted is not of the same state. > On Wed, Nov 13, 2024 at 10:49 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > 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. I'm beginning to have second thoughts about this whole thing TBH, as it feels inconsistent -- and unnecessary, if we get the patch to flip the INHERIT/NO INHERIT flag of constraints. > > - 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? Hmm, I'm not sure. It's not terribly useful, is it? I mean, if the user wants to have a constraint, then whether the constraint is there or not, the end result is the same and we needn't say anything about it. It's only if the constraint is not what they want (because of mismatching INHERIT flag) that we throw some message. (I wonder if we'd throw an error if the proposed constraint has a different _name_ from the existing constraint. If a DDL script is expecting that the constraint will be named a certain way, then by failing to throw an error we might be giving confusing expectations.) > > * 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? That's correct. What I mean is that if you have a constraint, and you try to add another, then the reaction is to compare the desired constraint with the existing one; if the comparison yields okay, then we silently do nothing; if the comparison says both things are incompatible, we throw an error. In no case would we add a second constraint. > > 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. Sounds good. > 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. I agree that both types of constraints should behave as similarly as possible in as many ways as possible. Behavioral differences are unlikely to be cherished by users. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Tue, Nov 19, 2024 at 6:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2024-Nov-14, Amit Langote wrote:
>
> > 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;
>
> Yeah, I think this behavior is bogus, because the user wants to have
> something (a constraint that will not inherit) but they cannot have it,
> because there is already a constraint that will inherit. The current
> behavior of throwing an error seems correct to me. With your patch,
> what this does is mark the constraint as "local" in addition to
> inherited, but that'd be wrong, because the constraint the user wanted
> is not of the same state.
Yeah, just not throwing the error, as my patch did, is not enough.
The patch didn't do anything to ensure that a separate constraint with
the properties that the user entered will exist alongside the
inherited one, but that's not possible given that the design only
allows one NOT NULL constraint for a column as you've written below.
> > On Wed, Nov 13, 2024 at 10:49 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > > 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.
>
> I'm beginning to have second thoughts about this whole thing TBH, as it
> feels inconsistent -- and unnecessary, if we get the patch to flip the
> INHERIT/NO INHERIT flag of constraints.
Ah, ok, I haven't looked at that patch, but I am happy to leave this alone.
> > > - 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?
>
> Hmm, I'm not sure. It's not terribly useful, is it? I mean, if the
> user wants to have a constraint, then whether the constraint is there or
> not, the end result is the same and we needn't say anything about it.
>
> It's only if the constraint is not what they want (because of
> mismatching INHERIT flag) that we throw some message.
>
> (I wonder if we'd throw an error if the proposed constraint has a
> different _name_ from the existing constraint. If a DDL script is
> expecting that the constraint will be named a certain way, then by
> failing to throw an error we might be giving confusing expectations.)
Here's an example that I think matches the above description, which,
ISTM, leads to a state similar to what one would encounter with my
patch as described earlier:
create table parent (a int not null);
create table child (a int, constraint a_not_null_child not null a)
inherits (parent);
NOTICE: merging column "a" with inherited definition
\d+ child
Table "public.child"
Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
a | integer | | not null | | plain |
| |
Not-null constraints:
"a_not_null_child" NOT NULL "a" (local, inherited)
Inherits: parent
Access method: heap
I think the inherited constraint should be named parent_a_not_null,
but the constraint that gets stored is the user-specified constraint
in `create table child`. Actually, even the automatically generated
constraint name using the child table's name won't match the name of
the inherited constraint:
create table child (a int not null) inherits (parent);
NOTICE: merging column "a" with inherited definition
\d+ child
Table "public.child"
Column | Type | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
a | integer | | not null | | plain |
| |
Not-null constraints:
"child_a_not_null" NOT NULL "a" (local, inherited)
Inherits: parent
Access method: heap
Not sure, but perhaps the following piece of code of
AddRelationNotNullConstraints() should also check that the names
match?
/*
* Search in the list of inherited constraints for any entries on the
* same column; determine an inheritance count from that. Also, if at
* least one parent has a constraint for this column, then we must not
* accept a user specification for a NO INHERIT one. Any constraint
* from parents that we process here is deleted from the list: we no
* longer need to process it in the loop below.
*/
foreach_ptr(CookedConstraint, old, old_notnulls)
{
if (old->attnum == attnum)
{
/*
* If we get a constraint from the parent, having a local NO
* INHERIT one doesn't work.
*/
if (constr->is_no_inherit)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("cannot define not-null constraint
on column \"%s\" with NO INHERIT",
strVal(linitial(constr->keys))),
errdetail("The column has an inherited
not-null constraint.")));
inhcount++;
old_notnulls = foreach_delete_current(old_notnulls, old);
}
}
Unless the inherited NOT NULL constraints are not required to have the
same name.
> > > * 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?
>
> That's correct. What I mean is that if you have a constraint, and you
> try to add another, then the reaction is to compare the desired
> constraint with the existing one; if the comparison yields okay, then we
> silently do nothing; if the comparison says both things are
> incompatible, we throw an error. In no case would we add a second
> constraint.
>
> > > 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.
>
> Sounds good.
>
> > 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.
>
> I agree that both types of constraints should behave as similarly as
> possible in as many ways as possible. Behavioral differences are
> unlikely to be cherished by users.
To be clear, I suppose we agree on continuing to throw an error when
trying to define a NO INHERIT CHECK constraint on a leaf partition.
That is to match the behavior we currently (as of 14e87ffa5) have for
NOT NULL constraints.
--
Thanks, Amit Langote
On 2024-Nov-20, Amit Langote wrote: > On Tue, Nov 19, 2024 at 6:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Here's an example that I think matches the above description, which, > ISTM, leads to a state similar to what one would encounter with my > patch as described earlier: > > create table parent (a int not null); > create table child (a int, constraint a_not_null_child not null a) > inherits (parent); > NOTICE: merging column "a" with inherited definition > > \d+ child > Table "public.child" > Column | Type | Collation | Nullable | Default | Storage | > Compression | Stats target | Description > --------+---------+-----------+----------+---------+---------+-------------+--------------+------------- > a | integer | | not null | | plain | > | | > Not-null constraints: > "a_not_null_child" NOT NULL "a" (local, inherited) > Inherits: parent > Access method: heap > > I think the inherited constraint should be named parent_a_not_null, > but the constraint that gets stored is the user-specified constraint > in `create table child`. Yeah, the user-specified name taking precedence over using a name from inheritance was an explicit decision. I think this is working as intended. An important point is that pg_dump should preserve any constraint name; that's for example why we disallow ALTER TABLE DROP CONSTRAINT of an inherited constraint: previously I made that command reset the 'islocal' flag of the constraint and otherwise do nothing; but the problem is that if we allow that, then the constraint gets the wrong name after dump/restore. > Not sure, but perhaps the following piece of code of > AddRelationNotNullConstraints() should also check that the names > match? > > /* > * Search in the list of inherited constraints for any entries on the > * same column; determine an inheritance count from that. Also, if at > * least one parent has a constraint for this column, then we must not > * accept a user specification for a NO INHERIT one. Any constraint > * from parents that we process here is deleted from the list: we no > * longer need to process it in the loop below. > */ I was of two minds about checking that the constraint names match, but in the end I decided it wasn't useful and limiting, because you cannot have a particular name in the children if you want to. One thing that distinguishes not-null constraints from check ones is that when walking down inheritance trees we match by the column that the apply to, rather than by name as check constraints do. So the fact that the names don't match doesn't harm. That would not fly for check constraints. > Unless the inherited NOT NULL constraints are not required to have the > same name. Yep. > > I agree that both types of constraints should behave as similarly as > > possible in as many ways as possible. Behavioral differences are > > unlikely to be cherished by users. > > To be clear, I suppose we agree on continuing to throw an error when > trying to define a NO INHERIT CHECK constraint on a leaf partition. > That is to match the behavior we currently (as of 14e87ffa5) have for > NOT NULL constraints. Yeah. I think we should only worry to the extent that these things trouble users over what they can and cannot do with tables. Adding a NO INHERIT constraint to a partition, just so that they can continue to have the constraint after they detach and that the constraint doesn't affect any tables that are added as children ... doesn't seem a very important use case. _But_, for anybody out there that does care about such a thing, we might have an ALTER TABLE command to change a constraint from NO INHERIT to INHERIT, and perhaps vice-versa. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "No es bueno caminar con un hombre muerto"
On Wed, Nov 27, 2024 at 12:53 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2024-Nov-20, Amit Langote wrote: > > On Tue, Nov 19, 2024 at 6:41 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > Here's an example that I think matches the above description, which, > > ISTM, leads to a state similar to what one would encounter with my > > patch as described earlier: > > > > create table parent (a int not null); > > create table child (a int, constraint a_not_null_child not null a) > > inherits (parent); > > NOTICE: merging column "a" with inherited definition > > > > \d+ child > > Table "public.child" > > Column | Type | Collation | Nullable | Default | Storage | > > Compression | Stats target | Description > > --------+---------+-----------+----------+---------+---------+-------------+--------------+------------- > > a | integer | | not null | | plain | > > | | > > Not-null constraints: > > "a_not_null_child" NOT NULL "a" (local, inherited) > > Inherits: parent > > Access method: heap > > > > I think the inherited constraint should be named parent_a_not_null, > > but the constraint that gets stored is the user-specified constraint > > in `create table child`. > > Yeah, the user-specified name taking precedence over using a name from > inheritance was an explicit decision. I think this is working as > intended. An important point is that pg_dump should preserve any > constraint name; that's for example why we disallow ALTER TABLE DROP > CONSTRAINT of an inherited constraint: previously I made that command > reset the 'islocal' flag of the constraint and otherwise do nothing; but > the problem is that if we allow that, then the constraint gets the wrong > name after dump/restore. Ok, I see. I didn't think about the pg_dump / restore aspect of this. > > Not sure, but perhaps the following piece of code of > > AddRelationNotNullConstraints() should also check that the names > > match? > > > > /* > > * Search in the list of inherited constraints for any entries on the > > * same column; determine an inheritance count from that. Also, if at > > * least one parent has a constraint for this column, then we must not > > * accept a user specification for a NO INHERIT one. Any constraint > > * from parents that we process here is deleted from the list: we no > > * longer need to process it in the loop below. > > */ > > I was of two minds about checking that the constraint names match, but > in the end I decided it wasn't useful and limiting, because you cannot > have a particular name in the children if you want to. > > One thing that distinguishes not-null constraints from check ones is > that when walking down inheritance trees we match by the column that the > apply to, rather than by name as check constraints do. So the fact that > the names don't match doesn't harm. That would not fly for check > constraints. Yeah, that makes sense. > > Unless the inherited NOT NULL constraints are not required to have the > > same name. > > Yep. > > > > I agree that both types of constraints should behave as similarly as > > > possible in as many ways as possible. Behavioral differences are > > > unlikely to be cherished by users. > > > > To be clear, I suppose we agree on continuing to throw an error when > > trying to define a NO INHERIT CHECK constraint on a leaf partition. > > That is to match the behavior we currently (as of 14e87ffa5) have for > > NOT NULL constraints. > > Yeah. I think we should only worry to the extent that these things > trouble users over what they can and cannot do with tables. Adding a NO > INHERIT constraint to a partition, just so that they can continue to > have the constraint after they detach and that the constraint doesn't > affect any tables that are added as children ... doesn't seem a very > important use case. _But_, for anybody out there that does care about > such a thing, we might have an ALTER TABLE command to change a > constraint from NO INHERIT to INHERIT, and perhaps vice-versa. +1 -- Thanks, Amit Langote