doc fail about ALTER TABLE ATTACH re. NO INHERIT

On 2024-Nov-05, Alvaro Herrera wrote:

All in all, I think this text serves no purpose and should be removed
(from all live branches), as in the attached patch.

Attached here.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Los cuentos de hadas no dan al niño su primera idea sobre los monstruos.
Lo que le dan es su primera idea de la posible derrota del monstruo."
(G. K. Chesterton)

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 Wed, Nov 6, 2024 at 9:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

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.

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.

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>

+1

Though if we decide to apply the attached, does the note "not marked
NO INHERIT" become unnecessary?

--
Thanks, Amit Langote

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 Tue, Nov 12, 2024 at 5:46 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

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.

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.

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;

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.

--
Thanks, Amit Langote

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 Wed, Nov 13, 2024 at 10:49 PM Alvaro Herrera <alvherre@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

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