Re: doc fail about ALTER TABLE ATTACH re. NO INHERIT

Hi Alvaro,

On Tue, Nov 5, 2024 at 9:01 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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(at)alvh(dot)no-ip(dot)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