Feedback-ID: ia2694551:Fastmail
Date: Thu, 19 Sep 2024 10:26:00 +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: <202409190826.qty7bw7d6uce@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CACJufxE5HO4LZMNOX6igVJmixj2er9vJcyFmLsM9OQB8+k8kLw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202409190826.qty7bw7d6uce%40alvherre.pgsql>
Precedence: bulk

On 2024-Sep-19, jian he wrote:

> still based on v3.
> in src/sgml/html/ddl-partitioning.html
> <<<QUOTE<<
> Both CHECK and NOT NULL constraints of a partitioned table are always
> inherited by all its partitions.
> CHECK constraints that are marked NO INHERIT are not allowed to be
> created on partitioned tables.
> You cannot drop a NOT NULL constraint on a partition's column if the
> same constraint is present in the parent table.
> <<<QUOTE<<
> we can change
> "CHECK constraints that are marked NO INHERIT are not allowed to be
> created on partitioned tables."
> to
> "CHECK and NOT NULL constraints that are marked NO INHERIT are not
> allowed to be created on partitioned tables."

Right.  Your proposed text is correct but sounds a bit repetitive with
the phrase just prior, and also the next one about inability to drop a
NOT NULL applies equally to CHECK constraints; so I modified the whole
paragraph to this:

        Both <literal>CHECK</literal> and <literal>NOT NULL</literal>
        constraints of a partitioned table are always inherited by all its
        partitions; it is not allowed to create <literal>NO INHERIT<literal>
        constraints of those types.
        You cannot drop a constraint of those types if the same constraint
        is present in the parent table.


> in sql-altertable.html we have:
> <<<QUOTE<<
> ATTACH PARTITION partition_name { FOR VALUES partition_bound_spec | DEFAULT }
> 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.
> <<<QUOTE<<
> 
> create table idxpart (a int constraint nn not null) partition by range (a);
> create table idxpart0 (a int constraint nn not null no inherit);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> 
> In the above sql query case,
> we changed a constraint ("nn" on idxpart0) connoinherit attribute
> after ATTACH PARTITION.
> (connoinherit from true to false)
> Do we need extra sentences to explain it?
> here not-null constraint behavior seems to divert from CHECK constraint.

Ah, yeah, the docs are misleading: we do allow these constraints to
mutate from NO INHERIT to INHERIT.  There's no danger in this, because
such a table cannot have children: no inheritance children (because
inheritance-parent tables cannot be partitions) and no partitions
either, because partitioned tables are not allowed to have NOT NULL NO INHERIT 
constraints.  So this can only happen on a standalone table, and thus
changing the existing not-null constraint from NO INHERIT to normal does
no harm.

I think we could make CHECK behave the same way on this point; but in the
meantime, I propose this text:

      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.
      By contrast, a <literal>NOT NULL</literal> constraint that was created
      as <literal>NO INHERIT</literal> will be changed to a normal inheriting
      one during attach.


> drop table if exists idxpart, idxpart0, idxpart1 cascade;
> create table idxpart (a int) partition by range (a);
> create table idxpart0 (a int primary key);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> alter table idxpart alter column a set not null;
> alter table idxpart0 alter column a drop not null;
> alter table idxpart0 drop constraint idxpart0_a_not_null;
> 
> "alter table idxpart0 alter column a drop not null;"
> is logically equivalent to
> "alter table idxpart0 drop constraint idxpart0_a_not_null;"
> 
> the first one (alter column) ERROR out,
> the second success.
> the second "drop constraint" should also ERROR out?
> since it violates the sentence in ddl-partitioning.html
> "You cannot drop a NOT NULL constraint on a partition's column if the
> same constraint is present in the parent table."

Yeah, I modified this code already a few days ago, and now it does error
out like this

ERROR:  cannot drop inherited constraint "idxpart0_a_not_null" of relation "idxpart0"

Anyway, as I mentioned back then, the DROP CONSTRAINT didn't _actually_
remove the constraint; it only marked the constraint as no longer
locally defined (conislocal=false), which had no practical effect other
than changing the representation during pg_dump.  Even detaching the
partition after having "dropped" the constraint would make the not-null
constraint appear again as coninhcount=0,conislocal=true rather than
drop it.


Speaking of pg_dump, I'm still on the nightmarish trip to get it to
behave correctly for all cases (esp. for pg_upgrade).  It seems I
tripped up on my own code from the previous round, having
under-commented and misunderstood it :-(

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)