MIME-Version: 1.0
References: <202411051201.zody6mld7vkw@alvherre.pgsql>
In-Reply-To: <202411051201.zody6mld7vkw@alvherre.pgsql>
From: Amit Langote <amitlangote09@gmail.com>
Date: Wed, 6 Nov 2024 11:43:22 +0900
Message-ID: <CA+HiwqET6+uBzdw7Pz+H1pz84t28WSO_7pnGLMuXoKYnkmsqUw@mail.gmail.com>
Subject: Re: doc fail about ALTER TABLE ATTACH re. NO INHERIT
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>, Robert Haas <robertmhaas@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BHiwqET6%2BuBzdw7Pz%2BH1pz84t28WSO_7pnGLMuXoKYnkmsqUw%40mail.gmail.com>
Precedence: bulk

Hi Alvaro,

On Tue, Nov 5, 2024 at 9:01=E2=80=AFPM 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 marke=
d NO
>     INHERIT, the command will fail; such constraints must be recreated wi=
thout 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 =E2=94=82  Type   =E2=94=82 Collation =E2=94=82 Nullable =E2=94=
=82 Default =E2=94=82 Storage =E2=94=82 Stats target =E2=94=82 Description
> =E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=
=E2=94=BC=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=BC=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=BC=E2=94=80=E2=94=80=
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=BC=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=BC=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=
=E2=94=80=E2=94=80=E2=94=80=E2=94=BC=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=
=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=
=80=E2=94=80=E2=94=BC=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=
=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80=E2=94=80
>  a      =E2=94=82 integer =E2=94=82           =E2=94=82          =E2=94=
=82         =E2=94=82 plain   =E2=94=82              =E2=94=82
> Partition of: parted FOR VALUES IN (1)
> Partition constraint: ((a IS NOT NULL) AND (a =3D 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.

--=20
Thanks, Amit Langote