Feedback-ID: ia2694551:Fastmail
Date: Thu, 26 Sep 2024 18:53:01 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: jian he <jian.universality@gmail.com>
Cc: Tender Wang <tndrwang@gmail.com>, 
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: not null constraints, again
Message-ID: <202409261653.zrz32zjsaqh4@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACJufxEiGZxm1Nh-0vUKCRVWLLdOT_Vh=_Lrk56V3Kj6J76eaA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202409261653.zrz32zjsaqh4%40alvherre.pgsql>
Precedence: bulk

On 2024-Sep-26, jian he wrote:

> +-- a PK in parent must have a not-null in child that it can mark inherited
> +create table inh_parent (a int primary key);
> +create table inh_child (a int primary key);
> +alter table inh_child inherit inh_parent; -- nope
> +alter table inh_child alter a set not null;
> +alter table inh_child inherit inh_parent; -- now it works
> +ERROR:  relation "inh_parent" would be inherited from more than once
> in src/test/regress/sql/inherit.sql, the comments at the end of the
> command, seem to conflict with the output?

Outdated, useless -- removed.

> -------------------------------------------------------------------------------
> 
> ALTER TABLE ALTER COLUMN SET NOT NULL
> implicitly means
> ALTER TABLE ALTER COLUMN SET NOT NULL NO INHERIT.
> 
> So in ATExecSetNotNull
>         if (conForm->connoinherit && recurse)
>             ereport(ERROR,
>                     errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                     errmsg("cannot change NO INHERIT status of NOT
> NULL constraint \"%s\" on relation \"%s\"",
>                            NameStr(conForm->conname),
>                            RelationGetRelationName(rel)));
> should be
>         if (conForm->connoinherit)
>             ereport(ERROR,
>                     errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                     errmsg("cannot change NO INHERIT status of NOT
> NULL constraint \"%s\" on relation \"%s\"",
>                            NameStr(conForm->conname),
>                            RelationGetRelationName(rel)));
> 
> then we can avoid the weird case like below:
> 
> drop table if exists pp1;
> create table pp1 (f1 int not null no inherit);
> ALTER TABLE pp1 ALTER f1 SET NOT NULL;
> ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;

Hmm, I don't understand why you say SET NOT NULL implicitly means SET
NOT NULL NO INHERIT.  That's definitely not the intention.  As I
explained earlier, the normal state is that a constraint is inheritable,
so if you do SET NOT NULL you want that constraint to be INHERIT.

Anyway, I don't see what you see as weird in the commands you list.  To
me it reacts like this:

=# create table pp1 (f1 int not null no inherit);
CREATE TABLE
=# ALTER TABLE pp1 ALTER f1 SET NOT NULL;
ERROR:  cannot change NO INHERIT status of NOT NULL constraint "pp1_f1_not_null" on relation "pp1"
=# ALTER TABLE ONLY pp1 ALTER f1 SET NOT NULL;
ALTER TABLE
=# \d+ pp1
                                                  Tabla «public.pp1»
 Columna │  Tipo   │ Ordenamiento │ Nulable  │ Por omisión │ Almacenamiento │ Compresión │ Estadísticas │ Descripción 
─────────┼─────────┼──────────────┼──────────┼─────────────┼────────────────┼────────────┼──────────────┼─────────────
 f1      │ integer │              │ not null │             │ plain          │            │              │ 
Not-null constraints:
    "pp1_f1_not_null" NOT NULL "f1" NO INHERIT
Método de acceso: heap

which seems to be exactly what we want.


> -------------------------------------------------------------------------------
> 
> + else if (rel->rd_rel->relhassubclass &&
> + find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL)
> + {
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> + errmsg("not-null constraint on column \"%s\" must be removed in
> child tables too",
> +   colName),
> + errhint("Do not specify the ONLY keyword."));
> + }
> this part in ATExecDropNotNull is not necessary?
> 
> per alter_table.sql
> <<<<<<---------->>>>>>
> -- make sure we can drop a constraint on the parent but it remains on the child
> CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL));
> CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent);
> ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT
> "test_drop_constr_parent_c_check";
> <<<<<<---------->>>>>>
> by the same way, we can drop a not-null constraint ONLY on the parent,
> but it remains on the child.
> if we not remove the above part then
> ALTER TABLE ONLY DROP CONSTRAINT
> will behave differently from
> ALTER TABLE ONLY ALTER COLUMN DROP NOT NULL.
> 
> example:
> drop table pp1,cc1, cc2;
> create table pp1 (f1 int not null);
> create table cc1 (f2 text, f3 int) inherits (pp1);
> create table cc2(f4 float) inherits(pp1,cc1);
> 
> alter table only pp1 drop constraint pp1_f1_not_null; --works.
> alter table only pp1 alter column f1 drop not null; --- error, should also work.
> -------------------------------------------------------------------------------

Hmm.  I'm not sure I like this behavior, but there is precedent in
CHECK, and since DROP CONSTRAINT also already works that way, I suppose
DROP NOT NULL should do that too.  I'll get it changed.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I must say, I am absolutely impressed with what pgsql's implementation of
VALUES allows me to do. It's kind of ridiculous how much "work" goes away in
my code.  Too bad I can't do this at work (Oracle 8/9)."       (Tom Allison)
           http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php