Feedback-ID: ia2694551:Fastmail
Date: Thu, 7 Nov 2024 17:48:24 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: jian he <jian.universality@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>, 
	Tender Wang <tndrwang@gmail.com>
Subject: Re: not null constraints, again
Message-ID: <202411071648.b7eixytkwzbe@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACJufxEYG7qD9t=qq=28ZLagA43dSE5c1FD7me+cto7QxZNcuQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202411071648.b7eixytkwzbe%40alvherre.pgsql>
Precedence: bulk

On 2024-Nov-07, jian he wrote:

> RemoveInheritance
>             if (copy_con->coninhcount <= 0) /* shouldn't happen */
>                 elog(ERROR, "relation %u has non-inherited constraint \"%s\"",
>                      RelationGetRelid(child_rel), NameStr(copy_con->conname));
> dropconstraint_internal
>         if (childcon->coninhcount <= 0) /* shouldn't happen */
>             elog(ERROR, "relation %u has non-inherited constraint \"%s\"",
>                  childrelid, NameStr(childcon->conname));
> 
> RemoveInheritance error triggered (see below), dropconstraint_internal may also.
> that means the error message should use RelationGetRelationName
> rather than plain "relation %u"?
> 
> drop table if exists inh_parent,inh_child1,inh_child2;
> create table inh_parent(f1 int not null no inherit);
> create table inh_child1(f1 int not null no inherit);
> alter table inh_child1 inherit inh_parent;
> alter table inh_child1 NO INHERIT inh_parent;
> ERROR:  relation 26387 has non-inherited constraint "inh_child1_f1_not_null"

Hmm, no, this is just a code bug: in RemoveInheritance when scanning
'parent' for constraints, we must skip the ones that are NO INHERIT, but
weren't.  With the bug fixed, the sequence above results in a
no-longer-child inh_child1 that still has inh_child1_f1_not_null, and no
error is thrown.

> sql-altertable.html
> INHERIT parent_table
>         This form adds the target table as a new child of the specified parent table.
>         Subsequently, queries against the parent will include records of the target
>         table.  To be added as a child, the target table must already contain all the
>         same columns as the parent (it could have additional columns, too).  The columns
>         must have matching data types, and if they have NOT NULL constraints in the
>         parent then they must also have NOT NULL constraints in the child.
> 
> "
> The columns must have matching data types, and if they have NOT NULL
> constraints in the
>  parent then they must also have NOT NULL constraints in the child.
> "
> For the above sentence, we need to add some text to explain
> NOT NULL constraints, NO INHERIT property
> for the child table and parent table.

True.  I rewrote as follows, moving the whole explanation of constraints
together to the same paragraph, rather than talking about some
constraints in one paragraph and other constraints in another.  The
previous approach was better when NOT NULL markings were a property of
the column, but now that they are constraints in their own right, this
seems better.

    <term><literal>INHERIT <replaceable class="parameter">parent_table</replaceable></literal></term>
    <listitem>
     <para>
      This form adds the target table as a new child of the specified parent
      table.  Subsequently, queries against the parent will include records
      of the target table.  To be added as a child, the target table must
      already contain all the same columns as the parent (it could have
      additional columns, too).  The columns must have matching data types.
     </para>
      
     <para>
      In addition, all <literal>CHECK</literal> and <literal>NOT NULL</literal>
      constraints on the parent must also exist on the child, except those
      marked non-inheritable (that is, created with
      <literal>ALTER TABLE ... ADD CONSTRAINT ... NO INHERIT</literal>), which
      are ignored.  All child-table constraints matched must not be marked
      non-inheritable.  Currently
      <literal>UNIQUE</literal>, <literal>PRIMARY KEY</literal>, and
      <literal>FOREIGN KEY</literal> constraints are not considered, but
      this might change in the future.
     </para>
    </listitem>


> ------------------------------------------------
> drop table if exists inh_parent,inh_child1,inh_child2;
> create table inh_parent(f1 int not null no inherit);
> create table inh_child1(f1 int);
> alter table inh_child1 inherit inh_parent;
> alter table inh_child1 NO INHERIT inh_parent;
> ERROR:  1 unmatched constraints while removing inheritance from "inh_child1" to "inh_parent"
> 
> now, we cannot "uninherit" inh_child1 from inh_parent?
> not sure this is expected behavior.

Yeah, this is the same bug as above.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"In fact, the basic problem with Perl 5's subroutines is that they're not
crufty enough, so the cruft leaks out into user-defined code instead, by
the Conservation of Cruft Principle."  (Larry Wall, Apocalypse 6)