Re: doc fail about ALTER TABLE ATTACH re. NO INHERIT - Mailing list pgsql-hackers

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