not null constraints, again

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年8月31日周六 11:59写道：

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.

One thing that regretfully I haven't yet had time for, is paring down
the original test code: a lot of it is verifying the old semantics,
particularly for NO INHERIT constraints, which had grown ugly special
cases. It now mostly raises errors; or the tests are simply redundant.
I'm going to remove that stuff as soon as I'm back on my normal work
timezone.

sepgsql is untested.

I'm adding this to the September commitfest.

Thanks for working on this again.

AT_PASS_OLD_COL_ATTRS, I didn't see any other code that used it. I don't
think it's necessary.

I will take the time to look over the attached patch.

--
Tender Wang

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年8月31日周六 11:59写道：

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.

One thing that regretfully I haven't yet had time for, is paring down
the original test code: a lot of it is verifying the old semantics,
particularly for NO INHERIT constraints, which had grown ugly special
cases. It now mostly raises errors; or the tests are simply redundant.
I'm going to remove that stuff as soon as I'm back on my normal work
timezone.

sepgsql is untested.

I'm adding this to the September commitfest.

The attached patch adds List *nnconstraints, which store the not-null
definition, in struct CreateStmt.
This makes me a little confused about List *constraints in struct
CreateStmt. Actually, the List constraints
store ckeck constraint, and it will be better if the comments can reflect
that. Re-naming it to List *ckconstraints
seems more reasonable. But a lot of codes that use stmt->constraints will
be changed.

Since AddRelationNewConstraints() can now add not-null column constraint,
the comments about AddRelationNewConstraints()
should tweak a little.
"All entries in newColDefaults will be processed. Entries in
newConstraints
will be processed only if they are CONSTR_CHECK type."
Now, the type of new constraints may be not-null constraints.

If the column has already had one not-null constraint, and we add same
not-null constraint again.
Then the code will call AdjustNotNullInheritance1() in
AddRelationNewConstraints().
The comments
before entering AdjustNotNullInheritance1() in AddRelationNewConstraints()
look confusing to me.
Because constraint is not inherited.

--
Tender Wang

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年8月31日周六 11:59写道：

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.

One thing that regretfully I haven't yet had time for, is paring down
the original test code: a lot of it is verifying the old semantics,
particularly for NO INHERIT constraints, which had grown ugly special
cases. It now mostly raises errors; or the tests are simply redundant.
I'm going to remove that stuff as soon as I'm back on my normal work
timezone.

sepgsql is untested.

I'm adding this to the September commitfest.

The test case in constraints.sql, as below:
CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL);

^^^^^^^^^^
There are two not-null definitions, and is the second one redundant?

When we drop the column not-null constraint, we will enter
ATExecDropNotNull().
Then, it calls findNotNullConstraint() to get the constraint tuple. We
already have
attnum before the call findNotNullConstraint(). Can we directly call
findNotNullConstraintAttnum()?

In RemoveConstraintById(), I see below comments:

"For not-null and primary key constraints,
obtain the list of columns affected, so that we can reset their
attnotnull flags below."

However, there are no related codes that reflect the above comments.

--
Thanks,
Tender Wang

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 Mon, Sep 2, 2024 at 6:33 PM Tender Wang <tndrwang@gmail.com> wrote:

The attached patch adds List *nnconstraints, which store the not-null definition, in struct CreateStmt.
This makes me a little confused about List *constraints in struct CreateStmt. Actually, the List constraints
store ckeck constraint, and it will be better if the comments can reflect that. Re-naming it to List *ckconstraints
seems more reasonable. But a lot of codes that use stmt->constraints will be changed.

hi.
seems you forgot to attach the patch?
I also noticed this minor issue.
I have no preference for Renaming it to List *ckconstraints.
+1 for better comments. maybe reword to

List *constraints; /* CHECK constraints (list of Constraint nodes) */

On Tue, Sep 3, 2024 at 3:17 PM Tender Wang <tndrwang@gmail.com> wrote:

The test case in constraints.sql, as below:
CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL);
^^^^^^^^^^
There are two not-null definitions, and is the second one redundant?

hi.
i think this is ok. please see
function transformColumnDefinition and variable saw_nullable.

we need to make sure this:
CREATE TABLE notnull_tbl3 (a INTEGER NULL NOT NULL);
fails.

of course, it's also OK do this:
CREATE TABLE notnull_tbl3 (a INTEGER NULL NULL);

jian he <jian.universality@gmail.com> 于2024年9月9日周一 16:31写道：

On Mon, Sep 2, 2024 at 6:33 PM Tender Wang <tndrwang@gmail.com> wrote:

The attached patch adds List *nnconstraints, which store the not-null

definition, in struct CreateStmt.

This makes me a little confused about List *constraints in struct

CreateStmt. Actually, the List constraints

store ckeck constraint, and it will be better if the comments can

reflect that. Re-naming it to List *ckconstraints

seems more reasonable. But a lot of codes that use stmt->constraints

will be changed.

hi.
seems you forgot to attach the patch?
I also noticed this minor issue.
I have no preference for Renaming it to List *ckconstraints.
+1 for better comments. maybe reword to

List *constraints; /* CHECK constraints (list of Constraint
nodes) */

I just gave advice; whether it is accepted or not, it's up to Alvaro.
If Alvaro agrees with the advice, he will patch a new one. We can continue
to review the
new patch.
If Alvaro disagrees, he doesn't need to change the current patch. I think
this way will be
more straightforward for others who will review this feature.

On Tue, Sep 3, 2024 at 3:17 PM Tender Wang <tndrwang@gmail.com> wrote:

The test case in constraints.sql, as below:
CREATE TABLE notnull_tbl1 (a INTEGER NOT NULL NOT NULL);

^^^^^^^^^^

There are two not-null definitions, and is the second one redundant?

hi.
i think this is ok. please see
function transformColumnDefinition and variable saw_nullable.

Yeah, it is ok.

we need to make sure this:
CREATE TABLE notnull_tbl3 (a INTEGER NULL NOT NULL);
fails.

of course, it's also OK do this:
CREATE TABLE notnull_tbl3 (a INTEGER NULL NULL);

--
Thanks,
Tender Wang

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 2024-Sep-02, Tender Wang wrote:

The attached patch adds List *nnconstraints, which store the not-null
definition, in struct CreateStmt. This makes me a little confused
about List *constraints in struct CreateStmt. Actually, the List
constraints store ckeck constraint, and it will be better if the
comments can reflect that. Re-naming it to List *ckconstraints seems
more reasonable. But a lot of codes that use stmt->constraints will be
changed.

Well, if you look at the comment about CreateStmt, there's this:

/* ----------------------
* Create Table Statement
*
* NOTE: in the raw gram.y output, ColumnDef and Constraint nodes are
* intermixed in tableElts, and constraints and nnconstraints are NIL. After
* parse analysis, tableElts contains just ColumnDefs, nnconstraints contains
* Constraint nodes of CONSTR_NOTNULL type from various sources, and
* constraints contains just CONSTR_CHECK Constraint nodes.
* ----------------------
*/

So if we were to rename 'constraints' to 'ckconstraints', it would no
longer reflect the fact that not-null ones can be in the former list
initially.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"No hay ausente sin culpa ni presente sin disculpa" (Prov. francés)

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

It turns out I have not finished cleaning up the regression tests from
now-useless additions, because while doing so last time around I found
some bugs (especially one around comments in not-null constraints, which
weren't being preserved by an ALTER TABLE TYPE command; that required a
new strange hack in RememberConstraintForRebuilding), but also the LIKE
clause again). Also, in this version there's a problem in the
pg_upgrade test, which I hope to fix tomorrow.

This code can also be found at
https://github.com/alvherre/postgres/tree/notnull-init-18
(Please do ignore 89685e691f75309fec882723272c8b17106e6aa2, that was a
merge mistake).

On 2024-Sep-09, jian he wrote:

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);

Ah, obvious now that I see it, thanks.

============
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;

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

Yeah, I think we need to reject NO INHERIT constraints for domains.
I've done so in this new version.

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

True, assuming we reject the case for system columns as you say below.

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?

Added explicit rejection here and in a couple of other places.

foreach(lc, RelationGetNotNullConstraints(RelationGetRelid(relation), false))

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

I'm undecided about this, but seeing that we don't use this pattern
almost anywhere, I've gone ahead and added the extra local variable.

+ /*
+ * 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.

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.

Yes, this code is completely unnecessary now. Removed.

--
Álvaro Herrera PostgreSQL Developer — 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 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).

PREPARE constr_meta (name[]) AS
with cte as
(
select con.oid as conoid, conrelid::regclass, pc.relkind as relkind,
con.coninhcount as inhcnt
,con.conname, con.contype, con.connoinherit as noinherit
,con.conislocal as islocal, pa.attname, pa.attnum
from pg_constraint con join pg_class pc on pc.oid = con.conrelid
join pg_attribute pa on pa.attrelid = pc.oid
and pa.attnum = any(conkey)
where con.contype in ('n', 'p', 'c') and
pc.relname = ANY ($1)
)
select pg_get_constraintdef(conoid), * from cte
order by contype, inhcnt, islocal, attnum;

The above constr_meta is used to query meta info, nothing fancy.

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

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

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

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

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

In this case, why not make it no-op, this column "a" already NOT NULL.

so ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL
will change not-null information, no need to consider other not-null
related information.

 /*
- * 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.

" * 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"?

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

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

Sadly, there were some other time-wasting events that I failed to
consider, but here's now v3 which has fixed (AFAICS) all the problems
you reported.

On 2024-Sep-11, jian he wrote:

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.

Yeah, this is because ATPrepAddPrimaryKey was queueing SetNotNull nodes
for each column on each children, which is repetitive and causes the
problem you see. That was a leftover from the previous way we handled
PKs; we no longer need it to work that way. I have changed it so that
it queues one constraint addition per column, on the same table that
receives the PK. It now works correctly as far as I can tell.

Sadly, there's one more pg_dump issue, which causes the pg_upgrade tests
to fail. The problem is that if you have this sequence (taken from
constraints.sql):

CREATE TABLE notnull_tbl4 (a INTEGER PRIMARY KEY INITIALLY DEFERRED);
CREATE TABLE notnull_tbl4_cld2 (PRIMARY KEY (a) DEFERRABLE) INHERITS (notnull_tbl4);

this is dumped by pg_dump in this other way:

CREATE TABLE public.notnull_tbl4 (a integer NOT NULL);
CREATE TABLE public.notnull_tbl4_cld2 () INHERITS (public.notnull_tbl4);
ALTER TABLE ONLY public.notnull_tbl4_cld2 ADD CONSTRAINT notnull_tbl4_cld2_pkey PRIMARY KEY (a) DEFERRABLE;
ALTER TABLE ONLY public.notnull_tbl4 ADD CONSTRAINT notnull_tbl4_pkey PRIMARY KEY (a) DEFERRABLE INITIALLY DEFERRED;

This is almost exactly the same, except that the PK for
notnull_tbl4_cld2 is created in a separate command ... and IIUC this
causes the not-null constraint to obtain a different name, or a
different inheritance characteristic, and then from the
restored-by-pg_upgrade database, it's dumped by pg_dump separately.
This is what causes the pg_upgrade test to fail.

Anyway, this made me realize that there is a more general problem, to
wit, that pg_dump is not dumping not-null constraint names correctly --
sometimes they just not dumped, which is Not Good. I'll have to look
into that once more.

(Also: there are still a few additional test stanzas in regress/ that
ought to be removed; also, I haven't re-tested sepgsql, so it's probably
broken ATM.)

--
Álvaro Herrera PostgreSQL Developer — 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, &copytup->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)