Feedback-ID: ia2694551:Fastmail
Date: Wed, 6 Nov 2024 13:34:19 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>, 
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: doc fail about ALTER TABLE ATTACH re. NO INHERIT
Message-ID: <202411061234.3jticfqxgwgd@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+HiwqET6+uBzdw7Pz+H1pz84t28WSO_7pnGLMuXoKYnkmsqUw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202411061234.3jticfqxgwgd%40alvherre.pgsql>
Precedence: bulk

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