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

From: Alvaro Herrera <[email-masked]>
Date: Tue, 5 Nov 2024 13:01:12 +0100

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."

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 5 Nov 2024 13:02:55 +0100

> (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)
>From 718c3e17a76f4adf219f4e1d1aaf79766efaeb02 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email-masked]>
Date: Tue, 5 Nov 2024 12:52:47 +0100
Subject: [PATCH] doc: Remove incorrect assertion about ALTER TABLE failing

---
 doc/src/sgml/ref/alter_table.sgml | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 36770c012a6..c054530e723 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -1026,10 +1026,6 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
       <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>
 
      <para>
-- 
2.39.5

---

From: Amit Langote <[email-masked]>
Date: Wed, 6 Nov 2024 11:43:22 +0900

Hi Alvaro,

>  Column │  Type   │ Collation │ Nullable │ Default │ Storage │ Stats target │ Description

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 <[email-masked]>
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

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 6 Nov 2024 13:34:19 +0100

> 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.

> exists in the parent table; such constraints must be recreated without

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