Feedback-ID: ia2694551:Fastmail
Date: Thu, 12 Sep 2024 12:41:49 +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: <202409121041.aqwna5onctkq@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CACJufxFXTYip772prUZ-UFopEHhprj96K44yoj5pJO6P3fmXjQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202409121041.aqwna5onctkq%40alvherre.pgsql>
Precedence: bulk

On 2024-Sep-12, jian he wrote:

> ---exampleA
> drop table pp1,cc1, cc2;
> create table pp1 (f1 int);
> create table cc1 (f2 text, f3 int) inherits (pp1);
> create table cc2(f4 float) inherits(pp1,cc1);
> alter table pp1 alter column f1 set not null;
> execute constr_meta('{pp1,cc1, cc2}');
> 
> ---exampleB
> 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);
> execute constr_meta('{pp1,cc1, cc2}');
> 
> Should exampleA and exampleB
> return  same pg_constraint->coninhcount
> for not-null constraint "cc2_f1_not_null"
> ?

Yes, they should be identical.  In this case example A is in the wrong,
the constraint in cc2 should have inhcount=2 (which example B has) and
it has inhcount=1.  This becomes obvious if you do ALTER TABLE NO
INHERIT of both parents -- in example A, it fails the second one with
 ERROR:  relation 43823 has non-inherited constraint "cc2_f1_not_null"
because the inhcount was set wrong by SET NOT NULL.  Will fix.  (I think
the culprit is the "readyRels" stuff I had added -- I should nuke that
and add a CommandCounterIncrement in the right spot ...)


> We only have this Synopsis
> ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL

Yeah, this syntax is intended to add a "normal" not-null constraint,
i.e. one that inherits.

> --tests from src/test/regress/sql/inherit.sql
> CREATE TABLE inh_nn_parent (a int, NOT NULL a NO INHERIT);
> ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
> current fail at ATExecSetNotNull
> ERROR:  cannot change NO INHERIT status of NOT NULL constraint
> "inh_nn_parent_a_not_null" on relation "inh_nn_parent"

This is correct, because here you want a normal not-null constraint but
the table already has the weird ones that don't inherit.

> seems we translate
> ALTER TABLE inh_nn_parent ALTER a SET NOT NULL;
> to
> ALTER TABLE inh_nn_parent ALTER a SET NOT NULL INHERIT

Well, we don't "translate" it as such.  It's just what's normal.

> but we cannot (syntax error)
> ALTER TABLE inh_nn_parent ALTER a SET NOT NULL NO INHERIT;

I don't feel a need to support this syntax.  You can do with with the
ADD CONSTRAINT syntax if you need it.


>  /*
> - * Return the address of the modified column.  If the column was already NOT
> - * NULL, InvalidObjectAddress is returned.
> + * ALTER TABLE ALTER COLUMN SET NOT NULL
> + *
> + * Add a not-null constraint to a single table and its children.  Returns
> + * the address of the constraint added to the parent relation, if one gets
> + * added, or InvalidObjectAddress otherwise.
> + *
> + * We must recurse to child tables during execution, rather than using
> + * ALTER TABLE's normal prep-time recursion.
>   */
>  static ObjectAddress
> -ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
> - const char *colName, LOCKMODE lockmode)
> +ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName,
> + bool recurse, bool recursing, List **readyRels,
> + LOCKMODE lockmode)
> 
> you introduced two boolean "recurse", "recursing", don't have enough
> explanation.
> That is too much to comprehend.

Apologies.  I think it's a well-established pattern in tablecmds.c.
"bool recurse" is for the caller (ATRewriteCatalogs) to request
recursion.  "bool recursing" is for the function informing itself that
it is calling itself recursively, i.e. "I'm already recursing".  This is
mostly (?) used to skip things like permission checks.


> " * We must recurse to child tables during execution, rather than using
> " * ALTER TABLE's normal prep-time recursion.
> What does this sentence mean for these two boolean "recurse", "recursing"?

Here "recurse during execution" means ALTER TABLE's phase 2, that is,
ATRewriteCatalogs (which means some ATExecFoo function needs to
implement recursion internally), and "normal prep-time recursion" means
the recursion set up by phase 1 (ATPrepCmd), which creates separate
AlterTableCmd nodes for each child table.  See the comments for
AlterTable and the code for ATController.

> Finally, I did some cosmetic changes, also improved error message
> in MergeConstraintsIntoExisting

Thanks, will incorporate.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL.  This is by far the most pleasant management experience of
any database I've worked on."                             (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php