Thread: Re: not null constraints, again

On Sat, Aug 31, 2024 at 11:59 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hello
>
> Here I present another attempt at making not-null constraints be
> catalogued.  This is largely based on the code reverted at 9ce04b50e120,
> except that we now have a not-null constraint automatically created for
> every column of a primary key, and such constraint cannot be removed
> while the PK exists.  Thanks to this, a lot of rather ugly code is gone,
> both in pg_dump and in backend -- in particular the handling of NO
> INHERIT, which was needed for pg_dump.
>
> Noteworthy psql difference: because there are now even more not-null
> constraints than before, the \d+ display would be far too noisy if we
> just let it grow.  So here I've made it omit any constraints that
> underlie the primary key.  This should be OK since you can't do much
> with those constraints while the PK is still there.  If you drop the PK,
> the next \d+ will show those constraints.
>

hi.
my brief review.

create table t1(a int, b int, c int not null, primary key(a, b));
\d+ t1
ERROR:  operator is not unique: smallint[] <@ smallint[]
LINE 8: coalesce(NOT ARRAY[at.attnum] <@ (SELECT conkey FROM pg_cata...
                                      ^
HINT:  Could not choose a best candidate operator. You might need to
add explicit type casts.


the regression test still passed, i have no idea why.
anyway, the following changes make the above ERROR disappear.
also seems more lean.

printfPQExpBuffer(&buf,
          /* FIXME the coalesce trick looks silly. What's a better way? */
          "SELECT co.conname, at.attname, co.connoinherit, co.conislocal,\n"
          "co.coninhcount <> 0\n"
          "FROM pg_catalog.pg_constraint co JOIN\n"
          "pg_catalog.pg_attribute at ON\n"
          "(at.attrelid = co.conrelid AND at.attnum = co.conkey[1])\n"
          "WHERE co.contype = 'n' AND\n"
          "co.conrelid = '%s'::pg_catalog.regclass AND\n"
          "coalesce(NOT ARRAY[at.attnum] <@ (SELECT conkey FROM
pg_catalog.pg_constraint\n"
          "  WHERE contype = 'p' AND conrelid = '%s'::regclass), true)\n"
          "ORDER BY at.attnum",
          oid,
          oid);

change to

            printfPQExpBuffer(&buf,
                              "SELECT co.conname, at.attname,
co.connoinherit, co.conislocal,\n"
                              "co.coninhcount <> 0\n"
                              "FROM pg_catalog.pg_constraint co JOIN\n"
                              "pg_catalog.pg_attribute at ON\n"
                              "(at.attrelid = co.conrelid AND
at.attnum = co.conkey[1])\n"
                              "WHERE co.contype = 'n' AND\n"
                              "co.conrelid = '%s'::pg_catalog.regclass AND\n"
                              "NOT EXISTS (SELECT 1 FROM
pg_catalog.pg_constraint co1 where co1.contype = 'p'\n"
                              "AND at.attnum = any(co1.conkey) AND
co1.conrelid = '%s'::pg_catalog.regclass)\n"
                              "ORDER BY at.attnum",
                               oid,
                               oid);

steal idea from https://stackoverflow.com/a/75614278/15603477
============
create type comptype as (r float8, i float8);
create domain dcomptype1 as comptype not null no inherit;
with cte as (
  SELECT oid, conrelid::regclass, conname FROM  pg_catalog.pg_constraint
  where contypid in ('dcomptype1'::regtype))
select pg_get_constraintdef(oid) from cte;
current output is
NOT NULL

but it's not the same as
CREATE TABLE ATACC1 (a int, not null a no inherit);
with cte as ( SELECT oid, conrelid::regclass, conname FROM
pg_catalog.pg_constraint
  where conrelid in ('ATACC1'::regclass))
select pg_get_constraintdef(oid) from cte;
NOT NULL a NO INHERIT

i don't really sure the meaning of "on inherit" in
"create domain dcomptype1 as comptype not null no inherit;"

====================
bold idea. print out the constraint name: violates not-null constraint \"%s\"
for the following code:
                ereport(ERROR,
                        (errcode(ERRCODE_NOT_NULL_VIOLATION),
                         errmsg("null value in column \"%s\" of
relation \"%s\" violates not-null constraint",
                                NameStr(att->attname),
                                RelationGetRelationName(orig_rel)),
                         val_desc ? errdetail("Failing row contains
%s.", val_desc) : 0,
                         errtablecol(orig_rel, attrChk)));



====================
in extractNotNullColumn
we can Assert(colnum > 0);



create table t3(a int  , b int , c int ,not null a, not null c, not
null b, not null tableoid);
this should not be allowed?



    foreach(lc,
RelationGetNotNullConstraints(RelationGetRelid(relation), false))
    {
        AlterTableCmd *atsubcmd;

        atsubcmd = makeNode(AlterTableCmd);
        atsubcmd->subtype = AT_AddConstraint;
        atsubcmd->def = (Node *) lfirst_node(Constraint, lc);
        atsubcmds = lappend(atsubcmds, atsubcmd);
    }
forgive me for being hypocritical.
I guess this is not a good coding pattern.
one reason would be: if you do:
=
list *a = RelationGetNotNullConstraints(RelationGetRelid(relation), false);
foreach(lc, a)
=
then you can call pprint(a).


+ /*
+ * If INCLUDING INDEXES is not given and a primary key exists, we need to
+ * add not-null constraints to the columns covered by the PK (except those
+ * that already have one.)  This is required for backwards compatibility.
+ */
+ if ((table_like_clause->options & CREATE_TABLE_LIKE_INDEXES) == 0)
+ {
+ Bitmapset  *pkcols;
+ int x = -1;
+ Bitmapset  *donecols = NULL;
+ ListCell   *lc;
+
+ /*
+ * Obtain a bitmapset of columns on which we'll add not-null
+ * constraints in expandTableLikeClause, so that we skip this for
+ * those.
+ */
+ foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), true))
+ {
+ CookedConstraint *cooked = (CookedConstraint *) lfirst(lc);
+
+ donecols = bms_add_member(donecols, cooked->attnum);
+ }
+
+ pkcols = RelationGetIndexAttrBitmap(relation,
+ INDEX_ATTR_BITMAP_PRIMARY_KEY);
+ while ((x = bms_next_member(pkcols, x)) >= 0)
+ {
+ Constraint *notnull;
+ AttrNumber attnum = x + FirstLowInvalidHeapAttributeNumber;
+ String   *colname;
+ Form_pg_attribute attForm;
+
+ /* ignore if we already have one for this column */
+ if (bms_is_member(attnum, donecols))
+ continue;
+
+ attForm = TupleDescAttr(tupleDesc, attnum - 1);
+ colname = makeString(pstrdup(NameStr(attForm->attname)));
+ notnull = makeNotNullConstraint(colname);
+
+ cxt->nnconstraints = lappend(cxt->nnconstraints, notnull);
+ }
+ }
this part (" if (bms_is_member(attnum, donecols))" will always be true?
donecols is all not-null attnums, pkcols is pk not-null attnums.
so pkcols info will always be included in donecols.
i placed a "elog(INFO, "%s we are in", __func__);"
above
"attForm = TupleDescAttr(tupleDesc, attnum - 1);"
all regression tests still passed.

On 2024-Sep-09, jian he wrote:

> bold idea. print out the constraint name: violates not-null constraint \"%s\"
> for the following code:
>                 ereport(ERROR,
>                         (errcode(ERRCODE_NOT_NULL_VIOLATION),
>                          errmsg("null value in column \"%s\" of
> relation \"%s\" violates not-null constraint",
>                                 NameStr(att->attname),
>                                 RelationGetRelationName(orig_rel)),
>                          val_desc ? errdetail("Failing row contains
> %s.", val_desc) : 0,
>                          errtablecol(orig_rel, attrChk)));

I gave this a quick run, but I'm not sure it actually improves things
much.  Here's one change from the regression tests.  What do you think?

 INSERT INTO reloptions_test VALUES (1, NULL), (NULL, NULL);
 -ERROR:  null value in column "i" of relation "reloptions_test" violates not-null constraint
 +ERROR:  null value in column "i" of relation "reloptions_test" violates not-null constraint
"reloptions_test_i_not_null"

What do I get from having the constraint name?  It's not like I'm going
to drop the constraint and retry the insert.

Here's the POC-quality patch for this.  I changes a lot of regression
tests, which I don't patch here.  (But that's not the reason for me
thinking that this isn't worth it.)

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 29e186fa73d..d84137f4f43 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1907,6 +1907,7 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
  * have been converted from the original input tuple after tuple routing.
  * 'resultRelInfo' is the final result relation, after tuple routing.
  */
+#include "catalog/pg_constraint.h"
 void
 ExecConstraints(ResultRelInfo *resultRelInfo,
                 TupleTableSlot *slot, EState *estate)
@@ -1932,6 +1933,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
                 char       *val_desc;
                 Relation    orig_rel = rel;
                 TupleDesc    orig_tupdesc = RelationGetDescr(rel);
+                char       *conname;
 
                 /*
                  * If the tuple has been routed, it's been converted to the
@@ -1970,14 +1972,24 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
                                                          tupdesc,
                                                          modifiedCols,
                                                          64);
+                {
+                    HeapTuple    tuple;
+                    Form_pg_constraint conForm;
+
+                    tuple = findNotNullConstraintAttnum(RelationGetRelid(orig_rel),
+                                                        att->attnum);
+                    conForm = (Form_pg_constraint) GETSTRUCT(tuple);
+                    conname = pstrdup(NameStr(conForm->conname));
+                }
 
                 ereport(ERROR,
-                        (errcode(ERRCODE_NOT_NULL_VIOLATION),
-                         errmsg("null value in column \"%s\" of relation \"%s\" violates not-null constraint",
-                                NameStr(att->attname),
-                                RelationGetRelationName(orig_rel)),
-                         val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
-                         errtablecol(orig_rel, attrChk)));
+                        errcode(ERRCODE_NOT_NULL_VIOLATION),
+                        errmsg("null value in column \"%s\" of relation \"%s\" violates not-null constraint \"%s\"",
+                               NameStr(att->attname),
+                               RelationGetRelationName(orig_rel),
+                               conname),
+                        val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
+                        errtablecol(orig_rel, attrChk));
             }
         }
     }
-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hello, here's a v2 of this patch.  I have fixed --I think-- all the
> issues you and Tender Wang reported (unless I declined a fix in some
> previous email).
>

+ /*
+ * The constraint must appear as inherited in children, so create a
+ * modified constraint object to use.
+ */
+ constr = copyObject(constr);
+ constr->inhcount = 1;

in ATAddCheckNNConstraint, we don't need the above copyObject call.
because at the beginning of ATAddCheckNNConstraint, we do
    newcons = AddRelationNewConstraints(rel, NIL,
                                        list_make1(copyObject(constr)),
                                        recursing || is_readd,    /*
allow_merge */
                                        !recursing, /* is_local */
                                        is_readd,    /* is_internal */
                                        NULL);    /* queryString not available
                                                 * here */


pg_constraint manual <<<<QUOTE<<<
conislocal bool
This constraint is defined locally for the relation. Note that a
constraint can be locally defined and inherited simultaneously.
coninhcount int2
The number of direct inheritance ancestors this constraint has. A
constraint with a nonzero number of ancestors cannot be dropped nor
renamed.
<<<<END OF QUOTE

drop table idxpart cascade;
create table idxpart (a int) partition by range (a);
create table idxpart0 (like idxpart);
alter table idxpart0 add primary key (a);
alter table idxpart attach partition idxpart0 for values from (0) to (1000);
alter table idxpart add primary key (a);

alter table idxpart0 DROP CONSTRAINT idxpart0_pkey;
alter table idxpart0 DROP CONSTRAINT idxpart0_a_not_null;

First DROP CONSTRAINT failed as the doc said,
but the second success.
but the second DROP CONSTRAINT should fail?
Even if you drop success, idxpart0_a_not_null still exists.
it also conflicts with the pg_constraint I've quoted above.


transformTableLikeClause, expandTableLikeClause
can be further simplified when the relation don't have not-null as all like:

    /*
     * Reproduce not-null constraints by copying them.  This doesn't require
     * any option to have been given.
     */
    if (tupleDesc->constr && tupleDesc->constr->has_not_null)
    {
        lst = RelationGetNotNullConstraints(RelationGetRelid(relation), false);
        cxt->nnconstraints = list_concat(cxt->nnconstraints, lst);
    }




we can do:
create table parent (a text, b int);
create table child () inherits (parent);
alter table child no inherit parent;

so comments in AdjustNotNullInheritance
 * AdjustNotNullInheritance
 *        Adjust not-null constraints' inhcount/islocal for
 *        ALTER TABLE [NO] INHERITS

"ALTER TABLE [NO] INHERITS"
should be
"ALTER TABLE ALTER COLUMN [NO] INHERITS"
?

Also, seems AdjustNotNullInheritance never being called/used?

On Wed, Sep 11, 2024 at 9:11 AM jian he <jian.universality@gmail.com> wrote:
>
> On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > Hello, here's a v2 of this patch.  I have fixed --I think-- all the
> > issues you and Tender Wang reported (unless I declined a fix in some
> > previous email).
> >

after applying your changes.

in ATExecAddConstraint, ATAddCheckNNConstraint.
ATAddCheckNNConstraint(wqueue, tab, rel,
            newConstraint, recurse, false, is_readd,
            lockmode);
if passed to ATAddCheckNNConstraint rel is a partitioned table.
ATAddCheckNNConstraint itself can recurse to create not-null pg_constraint
for itself and it's partitions (children table).
This is fine as long as we only call ATExecAddConstraint once.

but ATExecAddConstraint itself will recurse, it will call
the partitioned table and each of the partitions.

The first time ATExecAddConstraint with a partitioned table with the
following calling chain
ATAddCheckNNConstraint-> AddRelationNewConstraints -> AdjustNotNullInheritance1
works fine.

the second time ATExecAddConstraint with the partitions
ATAddCheckNNConstraint-> AddRelationNewConstraints -> AdjustNotNullInheritance1
AdjustNotNullInheritance1 will make the partitions
pg_constraint->coninhcount bigger than 1.


for example:
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 add primary key (a);

After the above query
pg_constraint->coninhcount of idxpart0_a_not_null  becomes 2.
but it should be 1
?

On 2024-Sep-11, jian he wrote:

> On Wed, Sep 11, 2024 at 2:18 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > Hello, here's a v2 of this patch.  I have fixed --I think-- all the
> > issues you and Tender Wang reported (unless I declined a fix in some
> > previous email).
> >
> 
> + /*
> + * The constraint must appear as inherited in children, so create a
> + * modified constraint object to use.
> + */
> + constr = copyObject(constr);
> + constr->inhcount = 1;
> 
> in ATAddCheckNNConstraint, we don't need the above copyObject call.
> because at the beginning of ATAddCheckNNConstraint, we do
>     newcons = AddRelationNewConstraints(rel, NIL,
>                                         list_make1(copyObject(constr)),
>                                         recursing || is_readd,    /*
> allow_merge */
>                                         !recursing, /* is_local */
>                                         is_readd,    /* is_internal */
>                                         NULL);    /* queryString not available
>                                                  * here */

I'm disinclined to change this.  The purpose of creating a copy at this
point is to avoid modifying an object that doesn't belong to us.  It
doesn't really matter much that we have an additional copy anyway; this
isn't in any way performance-critical or memory-intensive.

> create table idxpart (a int) partition by range (a);
> create table idxpart0 (like idxpart);
> alter table idxpart0 add primary key (a);
> alter table idxpart attach partition idxpart0 for values from (0) to (1000);
> alter table idxpart add primary key (a);
> 
> alter table idxpart0 DROP CONSTRAINT idxpart0_pkey;
> alter table idxpart0 DROP CONSTRAINT idxpart0_a_not_null;
> 
> First DROP CONSTRAINT failed as the doc said,
> but the second success.
> but the second DROP CONSTRAINT should fail?
> Even if you drop success, idxpart0_a_not_null still exists.
> it also conflicts with the pg_constraint I've quoted above.

Hmm, this is because we allow a DROP CONSTRAINT to set conislocal from
true to false.  So the constraint isn't *actually* dropped.  If you try
the DROP CONSTRAINT a second time, you'll get an error then.  Maybe I
should change the order of checks here, so that we forbid doing the
conislocal change; that would be more consistent with what we document.
I'm undecided about this TBH -- maybe I should reword the documentation
you cite in a different way.

> transformTableLikeClause, expandTableLikeClause
> can be further simplified when the relation don't have not-null as all like:
> 
>     /*
>      * Reproduce not-null constraints by copying them.  This doesn't require
>      * any option to have been given.
>      */
>     if (tupleDesc->constr && tupleDesc->constr->has_not_null)
>     {
>         lst = RelationGetNotNullConstraints(RelationGetRelid(relation), false);
>         cxt->nnconstraints = list_concat(cxt->nnconstraints, lst);
>     }

True.

> Also, seems AdjustNotNullInheritance never being called/used?

Oh, right, I removed the last callsite recently.  I'll remove the
function, and rename AdjustNotNullInheritance1 to
AdjustNotNullInheritance now that that name is free.

Thanks for reviewing!  I'll handle your other comment tomorrow.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On Tue, Sep 17, 2024 at 1:47 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>


still digging inheritance related issues.

drop table if exists pp1,cc1, cc2;
create table pp1 (f1 int, constraint nn check (f1 > 1));
create table cc1 (f2 text, f3 int ) inherits (pp1);
create table cc2(f4 float, constraint nn check (f1 > 1)) inherits(pp1,cc1);
execute constr_meta('{pp1,cc1, cc2}');
alter table only cc2 drop constraint nn;
ERROR:  cannot drop inherited constraint "nn" of relation "cc2"

So:

drop table if exists pp1,cc1, cc2;
create table pp1 (f1 int not null);
create table cc1 (f2 text, f3 int not null no inherit) inherits (pp1);
create table cc2(f4 float, f1 int not null) inherits(pp1,cc1);
execute constr_meta('{pp1,cc1, cc2}');
alter table only cc2 drop constraint cc2_f1_not_null;

Last "alter table only cc2" should fail?
because it violates catalog-pg-constraint coninhcount description:
"The number of direct inheritance ancestors this constraint has. A
constraint with a nonzero number of ancestors cannot be dropped nor
renamed."

also
alter table only cc2 drop constraint cc2_f1_not_null;
success executed.
some pg_constraint attribute info may change.
but constraint cc2_f1_not_null still exists.

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."



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.



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."

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)

On Thu, Sep 19, 2024 at 4:26 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
>
> > 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.
>

funny.
as the previously sql example, if you execute
"alter table idxpart0 drop constraint idxpart0_a_not_null;"
again

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

I am not sure if that's logically OK or if the user can deduce the
logic from the manual.
like, the first time you use "alter table drop constraint"
to drop a constraint, the constraint is not totally dropped,
the second time you execute it again the constraint cannot be dropped directly.


i think the issue is the changes we did in dropconstraint_internal
in dropconstraint_internal, we have:
-----------
    if (con->contype == CONSTRAINT_NOTNULL &&
        con->conislocal && con->coninhcount > 0)
    {
        HeapTuple    copytup;
        copytup = heap_copytuple(constraintTup);
        con = (Form_pg_constraint) GETSTRUCT(copytup);
        con->conislocal = false;
        CatalogTupleUpdate(conrel, ©tup->t_self, copytup);
        ObjectAddressSet(conobj, ConstraintRelationId, con->oid);
        CommandCounterIncrement();
        table_close(conrel, RowExclusiveLock);
        return conobj;
    }
    /* Don't allow drop of inherited constraints */
    if (con->coninhcount > 0 && !recursing)
        ereport(ERROR,
                (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
                 errmsg("cannot drop inherited constraint \"%s\" of
relation \"%s\"",
                        constrName, RelationGetRelationName(rel))));
-----------



comments in dropconstraint_internal
"* Reset pg_constraint.attnotnull, if this is a not-null constraint."
should be
"pg_attribute.attnotnull"



also, we don't have tests for not-null constraint similar to check
constraint tests on
src/test/regress/sql/alter_table.sql (line 2067 to line 2073)

another bug.
I will dig later, just want to share it first.

minimum producer:
drop table if exists pp1,cc1, cc2,cc3;
create table pp1 (f1 int );
create table cc1 () inherits (pp1);
create table cc2() inherits(pp1,cc1);
create table cc3() inherits(pp1,cc1,cc2);

alter table pp1 alter f1 set not null;
ERROR:  tuple already updated by self

On 2024-Sep-20, Tender Wang wrote:

> jian he <jian.universality@gmail.com> 于2024年9月20日周五 11:34写道：
> 
> > another bug.
> > I will dig later, just want to share it first.
> >
> > minimum producer:
> > drop table if exists pp1,cc1, cc2,cc3;
> > create table pp1 (f1 int );
> > create table cc1 () inherits (pp1);
> > create table cc2() inherits(pp1,cc1);
> > create table cc3() inherits(pp1,cc1,cc2);
> >
> > alter table pp1 alter f1 set not null;
> > ERROR:  tuple already updated by self
> 
> I guess some place needs call CommandCounterIncrement().

Yeah ... this fixes it:

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 579b8075b5..3f66e43b9a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7877,12 +7877,6 @@ ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName,
     {
         List       *children;
 
-        /*
-         * Make previous addition visible, in case we process the same
-         * relation again while chasing down multiple inheritance trees.
-         */
-        CommandCounterIncrement();
-
         children = find_inheritance_children(RelationGetRelid(rel),
                                              lockmode);
 
@@ -7890,6 +7884,8 @@ ATExecSetNotNull(List **wqueue, Relation rel, char *conName, char *colName,
         {
             Relation    childrel = table_open(childoid, NoLock);
 
+            CommandCounterIncrement();
+
             ATExecSetNotNull(wqueue, childrel, conName, colName,
                              recurse, true, lockmode);
             table_close(childrel, NoLock);


I was trying to save on the number of CCIs that we perform, but it's
likely not a wise expenditure of time given that this isn't a very
common scenario anyway.  (Nobody with thousands of millions of children
tables will try to run thousands of commands in a single transaction
anyway ... so saving a few increments doesn't make any actual
difference.  If such people exist, they can show us their use case and
we can investigate and fix it then.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"This is what I like so much about PostgreSQL.  Most of the surprises
are of the "oh wow!  That's cool" Not the "oh shit!" kind.  :)"
Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php

On 2024-Sep-20, Tender Wang wrote:

> By the way, the v3  failed applying on Head(d35e293878)
> git am v3-0001-Catalog-not-null-constraints.patch
> Applying: Catalog not-null constraints
> error: patch failed: doc/src/sgml/ref/create_table.sgml:77

Yeah, there's a bunch of conflicts in current master.  I rebased
yesterday but I'm still composing the email for v4.  Coming soon.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

On Sat, Sep 21, 2024 at 5:15 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Okay, so here is v4 with these problems fixed, including correct
> propagation of constraint names to children tables, which I had
> inadvertently broken earlier.  This one does pass the pg_upgrade tests
> and as far as I can see pg_dump does all the correct things also.  I
> cleaned up the tests to remove everything that's unneeded, redundant, or
> testing behavior that no longer exists.
>

in findNotNullConstraintAttnum
        if (con->contype != CONSTRAINT_NOTNULL)
            continue;
        if (!con->convalidated)
            continue;

if con->convalidated is false, then we have a bigger problem?
maybe we can change to ERROR to expose/capture potential problems.
like:
        if (con->contype != CONSTRAINT_NOTNULL)
            continue;
        if (!con->convalidated)
            elog(ERROR, "not-null constraint is not validated");

------<<<<<<<<------------------
HeapTuple
findNotNullConstraint(Oid relid, const char *colname)
{
    AttrNumber    attnum = get_attnum(relid, colname);
    return findNotNullConstraintAttnum(relid, attnum);
}

we can change to

HeapTuple
findNotNullConstraint(Oid relid, const char *colname)
{
    AttrNumber    attnum = get_attnum(relid, colname);
    if (attnum <= InvalidAttrNumber)
        return NULL;
    return findNotNullConstraintAttnum(relid, attnum);
}
------<<<<<<<<------------------

sql-createtable.html
SECTION: LIKE source_table [ like_option ... ]
INCLUDING CONSTRAINTS
CHECK constraints will be copied. No distinction is made between
column constraints and table constraints. Not-null constraints are
always copied to the new table.

drop table if exists t, t_1,ssa;
create table t(a int, b int, not null a no inherit);
create table ssa (like t INCLUDING all);

Here create table like won't include no inherit not-null constraint,
seems to conflict with the doc?

------<<<<<<<<------------------
drop table if exists t, t_1;
create table t(a int primary key, b int,  not null a no inherit);
create table t_1 () inherits (t);

t_1 will inherit the not-null constraint from t,
so the syntax "not null a no inherit" information is ignored.

other cases:
create table t(a int not null, b int,  not null a no inherit);
create table t(a int not null no inherit, b int,  not null a);

seems currently, column constraint have not-null constraint, then use
it and table constraint (not-null)
are ignored.
but if column constraint don't have not-null then according to table constraint.

On 2024-Sep-24, jian he wrote:

> sql-createtable.html
> SECTION: LIKE source_table [ like_option ... ]
> INCLUDING CONSTRAINTS
> CHECK constraints will be copied. No distinction is made between
> column constraints and table constraints. Not-null constraints are
> always copied to the new table.
> 
> drop table if exists t, t_1,ssa;
> create table t(a int, b int, not null a no inherit);
> create table ssa (like t INCLUDING all);
> 
> Here create table like won't include no inherit not-null constraint,
> seems to conflict with the doc?

Hmm, actually I think this is a bug, because if you have CHECK
constraint with NO INHERIT, it will be copied:

create table t (a int check (a > 0) no inherit);
create table ssa (like t including constraints);

55490 18devel 141626=# \d+ ssa
                                                 Tabla «public.ssa»
 Columna │  Tipo   │ Ordenamiento │ Nulable │ Por omisión │ Almacenamiento>
─────────┼─────────┼──────────────┼─────────┼─────────────┼───────────────>
 a       │ integer │              │         │             │ plain         >
Restricciones CHECK:
    "t_a_check" CHECK (a > 0) NO INHERIT
Método de acceso: heap

It seems that NOT NULL constraint should behave the same as CHECK
constraints in this regard, i.e., we should not heed NO INHERIT in this
case.


I have made these changes and added some tests, and will be posting a v5
shortly.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
<inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell
<crab> inflex: you know that "amalgam" means "mixture with mercury",
       more or less, right?
<crab> i.e., "deadly poison"