Can't find not null constraint, but \d+ shows that

On 2024-Mar-26, Tender Wang wrote:

postgres=# CREATE TABLE t1(c0 int, c1 int);
postgres=# ALTER TABLE t1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
postgres=# ALTER TABLE t1 DROP c1;

postgres=# ALTER TABLE t1 ALTER c0 DROP NOT NULL;
ERROR: could not find not-null constraint on column "c0", relation "t1"

Ooh, hah, what happens here is that we drop the PK constraint
indirectly, so we only go via doDeletion rather than the tablecmds.c
code, so we don't check the attnotnull flags that the PK was protecting.

The attached patch is my workaround solution. Look forward your apply.

Yeah, this is not a very good approach -- I think you're just guessing
that the column is marked NOT NULL because a PK was dropped in the
past -- but really what this catalog state is, is corrupted contents
because the PK drop was mishandled. At least in theory there are other
ways to drop a constraint other than dropping one of its columns (for
example, maybe this could happen if you drop a collation that the PK
depends on). The right approach is to ensure that the PK drop always
does the dance that ATExecDropConstraint does. A good fix probably just
moves some code from dropconstraint_internal to RemoveConstraintById.

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

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年3月26日周二 23:25写道：

On 2024-Mar-26, Tender Wang wrote:

postgres=# CREATE TABLE t1(c0 int, c1 int);
postgres=# ALTER TABLE t1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
postgres=# ALTER TABLE t1 DROP c1;

postgres=# ALTER TABLE t1 ALTER c0 DROP NOT NULL;
ERROR: could not find not-null constraint on column "c0", relation "t1"

Ooh, hah, what happens here is that we drop the PK constraint
indirectly, so we only go via doDeletion rather than the tablecmds.c
code, so we don't check the attnotnull flags that the PK was protecting.

Yeah, Indeed, as you said.

The attached patch is my workaround solution. Look forward your apply.

Yeah, this is not a very good approach -- I think you're just guessing
that the column is marked NOT NULL because a PK was dropped in the
past -- but really what this catalog state is, is corrupted contents
because the PK drop was mishandled. At least in theory there are other
ways to drop a constraint other than dropping one of its columns (for
example, maybe this could happen if you drop a collation that the PK
depends on). The right approach is to ensure that the PK drop always
does the dance that ATExecDropConstraint does. A good fix probably just
moves some code from dropconstraint_internal to RemoveConstraintById.

Agreed. It is look better. But it will not work if simply move some codes
from dropconstraint_internal
to RemoveConstraintById. I have tried this fix before 0001 patch, but
failed.

For example:
create table skip_wal_skip_rewrite_index (c varchar(10) primary key);
alter table skip_wal_skip_rewrite_index alter c type varchar(20);
ERROR: primary key column "c" is not marked NOT NULL

index_check_primary_key() in index.c has below comments;

"We check for a pre-existing primary key, and that all columns of the index
are simple column references (not expressions), and that all those columns
are marked NOT NULL. If not, fail."

So in aboved example, RemoveConstraintById() can't reset attnotnull. We can
pass some information to
RemoveConstraintById() like a bool var to indicate that attnotnull should
be reset or not.

--
Tender Wang
OpenPie: https://en.openpie.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年3月26日周二 23:25写道：

On 2024-Mar-26, Tender Wang wrote:

postgres=# CREATE TABLE t1(c0 int, c1 int);
postgres=# ALTER TABLE t1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
postgres=# ALTER TABLE t1 DROP c1;

postgres=# ALTER TABLE t1 ALTER c0 DROP NOT NULL;
ERROR: could not find not-null constraint on column "c0", relation "t1"

Ooh, hah, what happens here is that we drop the PK constraint
indirectly, so we only go via doDeletion rather than the tablecmds.c
code, so we don't check the attnotnull flags that the PK was protecting.

The attached patch is my workaround solution. Look forward your apply.

Yeah, this is not a very good approach -- I think you're just guessing
that the column is marked NOT NULL because a PK was dropped in the
past -- but really what this catalog state is, is corrupted contents
because the PK drop was mishandled. At least in theory there are other
ways to drop a constraint other than dropping one of its columns (for
example, maybe this could happen if you drop a collation that the PK
depends on). The right approach is to ensure that the PK drop always
does the dance that ATExecDropConstraint does. A good fix probably just
moves some code from dropconstraint_internal to RemoveConstraintById.

I think again, below solutin maybe looks more better:
i. move some code from dropconstraint_internal to RemoveConstraintById,
not change the RemoveConstraintById interface. Ensure that the PK drop
always
does the dance that ATExecDropConstraint does.

ii. After i phase, the attnotnull of some column of primary key may be
reset to false as I provided example in last email.
We can set attnotnull to true again in some place.

--
Tender Wang
OpenPie: https://en.openpie.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年3月26日周二 23:25写道：

On 2024-Mar-26, Tender Wang wrote:

postgres=# CREATE TABLE t1(c0 int, c1 int);
postgres=# ALTER TABLE t1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
postgres=# ALTER TABLE t1 DROP c1;

postgres=# ALTER TABLE t1 ALTER c0 DROP NOT NULL;
ERROR: could not find not-null constraint on column "c0", relation "t1"

Ooh, hah, what happens here is that we drop the PK constraint
indirectly, so we only go via doDeletion rather than the tablecmds.c
code, so we don't check the attnotnull flags that the PK was protecting.

The attached patch is my workaround solution. Look forward your apply.

Yeah, this is not a very good approach -- I think you're just guessing
that the column is marked NOT NULL because a PK was dropped in the
past -- but really what this catalog state is, is corrupted contents
because the PK drop was mishandled. At least in theory there are other
ways to drop a constraint other than dropping one of its columns (for
example, maybe this could happen if you drop a collation that the PK
depends on). The right approach is to ensure that the PK drop always
does the dance that ATExecDropConstraint does. A good fix probably just
moves some code from dropconstraint_internal to RemoveConstraintById.

I found some types ddl would check the attnotnull of column is true, for
example: AT_ReAddIndex, AT_ReplicaIdentity.
So we should add AT_SetAttNotNull sub-command to the wqueue. I add a
new AT_PASS_OLD_COL_ATTRS to make sure
AT_SetAttNotNull will have done when do AT_ReAddIndex or
AT_ReplicaIdentity.

--
Tender Wang
OpenPie: https://en.openpie.com/

On Wed, Mar 27, 2024 at 10:26 PM Tender Wang <tndrwang@gmail.com> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年3月26日周二 23:25写道：

On 2024-Mar-26, Tender Wang wrote:

postgres=# CREATE TABLE t1(c0 int, c1 int);
postgres=# ALTER TABLE t1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
postgres=# ALTER TABLE t1 DROP c1;

postgres=# ALTER TABLE t1 ALTER c0 DROP NOT NULL;
ERROR: could not find not-null constraint on column "c0", relation "t1"

Ooh, hah, what happens here is that we drop the PK constraint
indirectly, so we only go via doDeletion rather than the tablecmds.c
code, so we don't check the attnotnull flags that the PK was protecting.

The attached patch is my workaround solution. Look forward your apply.

after applying v2-0001-Fix-pg_attribute-attnotnull-not-reset-when-droppe.patch

something is off, now i cannot drop a table.
demo:
CREATE TABLE t2(c0 int, c1 int);
ALTER TABLE t2 ADD CONSTRAINT t2_pk PRIMARY KEY(c0, c1);
ALTER TABLE t2 ALTER COLUMN c0 ADD GENERATED ALWAYS AS IDENTITY;
DROP TABLE t2 cascade;
Similarly, maybe there will be some issue with replica identity.

+ /*
+ * If this was a NOT NULL or the primary key, the constrained columns must
+ * have had pg_attribute.attnotnull set.  See if we need to reset it, and
+ * do so.
+ */
+ if (unconstrained_cols)
it should be if (unconstrained_cols != NIL)?,
given unconstrained_cols is a List, also "unconstrained_cols" naming
seems not intuitive.
maybe pk_attnums or pk_cols or pk_columns.

+ attrel = table_open(AttributeRelationId, RowExclusiveLock);
+ rel = table_open(con->conrelid, RowExclusiveLock);
I am not sure why we using RowExclusiveLock for con->conrelid?
given we use AccessExclusiveLock at:
/*
* If the constraint is for a relation, open and exclusive-lock the
* relation it's for.
*/
rel = table_open(con->conrelid, AccessExclusiveLock);

+ /*
+ * Since the above deletion has been made visible, we can now
+ * search for any remaining constraints on this column (or these
+ * columns, in the case we're dropping a multicol primary key.)
+ * Then, verify whether any further NOT NULL or primary key
+ * exists, and reset attnotnull if none.
+ *
+ * However, if this is a generated identity column, abort the
+ * whole thing with a specific error message, because the
+ * constraint is required in that case.
+ */
+ contup = findNotNullConstraintAttnum(RelationGetRelid(rel), attnum);
+ if (contup ||
+ bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber,
+  pkcols))
+ continue;

I didn't get this part.
if you drop delete a primary key,
the "NOT NULL" constraint within pg_constraint should definitely be removed?
therefore contup should be pretty sure is NULL?

  /*
- * The parser will create AT_AttSetNotNull subcommands for
- * columns of PRIMARY KEY indexes/constraints, but we need
- * not do anything with them here, because the columns'
- * NOT NULL marks will already have been propagated into
- * the new table definition.
+ * PK drop now will reset pg_attribute attnotnull to false.
+ * We should set attnotnull to true again.
  */
PK drop now will reset pg_attribute attnotnull to false,
which is what we should be expecting.
the comment didn't explain why should set attnotnull to true again?

jian he <jian.universality@gmail.com> 于2024年3月28日周四 13:18写道：

On Wed, Mar 27, 2024 at 10:26 PM Tender Wang <tndrwang@gmail.com> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年3月26日周二 23:25写道：

On 2024-Mar-26, Tender Wang wrote:

postgres=# CREATE TABLE t1(c0 int, c1 int);
postgres=# ALTER TABLE t1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
postgres=# ALTER TABLE t1 DROP c1;

postgres=# ALTER TABLE t1 ALTER c0 DROP NOT NULL;
ERROR: could not find not-null constraint on column "c0", relation

"t1"

Ooh, hah, what happens here is that we drop the PK constraint
indirectly, so we only go via doDeletion rather than the tablecmds.c
code, so we don't check the attnotnull flags that the PK was protecting.

The attached patch is my workaround solution. Look forward your

apply.

after applying
v2-0001-Fix-pg_attribute-attnotnull-not-reset-when-droppe.patch

something is off, now i cannot drop a table.
demo:
CREATE TABLE t2(c0 int, c1 int);
ALTER TABLE t2 ADD CONSTRAINT t2_pk PRIMARY KEY(c0, c1);
ALTER TABLE t2 ALTER COLUMN c0 ADD GENERATED ALWAYS AS IDENTITY;
DROP TABLE t2 cascade;
Similarly, maybe there will be some issue with replica identity.

Thanks for review this patch. Yeah, I can reproduce it. The error reported
in RemoveConstraintById(), where I moved
some codes from dropconstraint_internal(). But some check seems to no need
in RemoveConstraintById(). For example:

/*
* It's not valid to drop the not-null constraint for a GENERATED
* AS IDENTITY column.
*/
if (attForm->attidentity)
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("column \"%s\" of relation \"%s\" is an identity column",
get_attname(RelationGetRelid(rel), attnum,
false),
RelationGetRelationName(rel)));

/*
* It's not valid to drop the not-null constraint for a column in
* the replica identity index, either. (FULL is not affected.)
*/
if (bms_is_member(lfirst_int(lc) - FirstLowInvalidHeapAttributeNumber,
ircols))
ereport(ERROR,
errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("column \"%s\" is in index used as replica identity",
get_attname(RelationGetRelid(rel), lfirst_int(lc), false)));

Above two check can remove from RemoveConstraintById()? I need more test.

+ /*
+ * If this was a NOT NULL or the primary key, the constrained columns must
+ * have had pg_attribute.attnotnull set.  See if we need to reset it, and
+ * do so.
+ */
+ if (unconstrained_cols)
it should be if (unconstrained_cols != NIL)?,
given unconstrained_cols is a List, also "unconstrained_cols" naming
seems not intuitive.
maybe pk_attnums or pk_cols or pk_columns.

As I said above, the codes were copied from dropconstraint_internal(). NOT
NULL columns were not alwayls PK.
So I thinks "unconstrained_cols" is OK.

+ attrel = table_open(AttributeRelationId, RowExclusiveLock);
+ rel = table_open(con->conrelid, RowExclusiveLock);
I am not sure why we using RowExclusiveLock for con->conrelid?
given we use AccessExclusiveLock at:
/*
* If the constraint is for a relation, open and exclusive-lock the
* relation it's for.
*/
rel = table_open(con->conrelid, AccessExclusiveLock);

Yeah, you are right.

+ /*
+ * Since the above deletion has been made visible, we can now
+ * search for any remaining constraints on this column (or these
+ * columns, in the case we're dropping a multicol primary key.)
+ * Then, verify whether any further NOT NULL or primary key
+ * exists, and reset attnotnull if none.
+ *
+ * However, if this is a generated identity column, abort the
+ * whole thing with a specific error message, because the
+ * constraint is required in that case.
+ */
+ contup = findNotNullConstraintAttnum(RelationGetRelid(rel), attnum);
+ if (contup ||
+ bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber,
+  pkcols))
+ continue;

I didn't get this part.
if you drop delete a primary key,
the "NOT NULL" constraint within pg_constraint should definitely be
removed?
therefore contup should be pretty sure is NULL?

No, If the original definaiton of column includes NOT NULL, we can't reset
attnotnull to false when
We we drop PK.

/*
- * The parser will create AT_AttSetNotNull subcommands for
- * columns of PRIMARY KEY indexes/constraints, but we need
- * not do anything with them here, because the columns'
- * NOT NULL marks will already have been propagated into
- * the new table definition.
+ * PK drop now will reset pg_attribute attnotnull to false.
+ * We should set attnotnull to true again.
*/
PK drop now will reset pg_attribute attnotnull to false,
which is what we should be expecting.
the comment didn't explain why should set attnotnull to true again?

The V2 patch still needs more cases to test, Probably not right solution.
Anyway, I will send a v3 version patch after I do more test.

--
Tender Wang
OpenPie: https://en.openpie.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年3月26日周二 23:25写道：

On 2024-Mar-26, Tender Wang wrote:

postgres=# CREATE TABLE t1(c0 int, c1 int);
postgres=# ALTER TABLE t1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
postgres=# ALTER TABLE t1 DROP c1;

postgres=# ALTER TABLE t1 ALTER c0 DROP NOT NULL;
ERROR: could not find not-null constraint on column "c0", relation "t1"

Ooh, hah, what happens here is that we drop the PK constraint
indirectly, so we only go via doDeletion rather than the tablecmds.c
code, so we don't check the attnotnull flags that the PK was protecting.

The attached patch is my workaround solution. Look forward your apply.

Yeah, this is not a very good approach -- I think you're just guessing
that the column is marked NOT NULL because a PK was dropped in the
past -- but really what this catalog state is, is corrupted contents
because the PK drop was mishandled. At least in theory there are other
ways to drop a constraint other than dropping one of its columns (for
example, maybe this could happen if you drop a collation that the PK
depends on). The right approach is to ensure that the PK drop always
does the dance that ATExecDropConstraint does. A good fix probably just
moves some code from dropconstraint_internal to RemoveConstraintById.

RemoveConstraintById() should think recurse(e.g. partition table)? I'm not
sure now.
If we should think process recurse in RemoveConstraintById(), the
function will look complicated than before.

--
Tender Wang
OpenPie: https://en.openpie.com/

On 2024-Mar-28, Tender Wang wrote:

RemoveConstraintById() should think recurse(e.g. partition table)? I'm not
sure now.
If we should think process recurse in RemoveConstraintById(), the
function will look complicated than before.

No -- this function handles just a single constraint, as identified by
OID. The recursion is handled by upper layers, which can be either
dependency.c or tablecmds.c. I think the problem you found is caused by
the fact that I worked with the tablecmds.c recursion and neglected the
one in dependency.c.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"No nos atrevemos a muchas cosas porque son difíciles,
pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)

Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年3月28日周四 17:18写道：

On 2024-Mar-28, Tender Wang wrote:

RemoveConstraintById() should think recurse(e.g. partition table)? I'm

not

sure now.
If we should think process recurse in RemoveConstraintById(), the
function will look complicated than before.

No -- this function handles just a single constraint, as identified by
OID. The recursion is handled by upper layers, which can be either
dependency.c or tablecmds.c. I think the problem you found is caused by
the fact that I worked with the tablecmds.c recursion and neglected the
one in dependency.c.

Indeed.

create table skip_wal_skip_rewrite_index (c varchar(10) primary key);
alter table skip_wal_skip_rewrite_index alter c type varchar(20);

Above SQL need attnotnull to be true when re-add index, but
RemoveConstraintById() is hard to recognize this scenario as I know.
We should re-set attnotnull to be true before re-add index. I add a new
AT_PASS in attached patch.
Any thoughts?
--
Tender Wang
OpenPie: https://en.openpie.com/

hi.
about v4, i think, i understand the changes you made.
RemoveConstraintById(Oid conId)
will drop a single constraint record.
if the constraint is primary key, then primary key associated
attnotnull should set to false.
but sometimes it shouldn't.

for example:
drop table if exists t2;
CREATE TABLE t2(c0 int, c1 int);
ALTER TABLE t2 ADD CONSTRAINT t2_pk PRIMARY KEY(c0, c1);
ALTER TABLE t2 ALTER COLUMN c0 ADD GENERATED ALWAYS AS IDENTITY;
ALTER TABLE t2 DROP c1;

+ * If this was a NOT NULL or the primary key, the constrained columns must
+ * have had pg_attribute.attnotnull set.  See if we need to reset it, and
+ * do so.
+ */
+ if (unconstrained_cols != NIL)

unconstrained_cols is not NIL, which means we have dropped a primary key.
I am confused by "If this was a NOT NULL or the primary key".

+
+ /*
+ * Since the above deletion has been made visible, we can now
+ * search for any remaining constraints on this column (or these
+ * columns, in the case we're dropping a multicol primary key.)
+ * Then, verify whether any further NOT NULL exists, and reset
+ * attnotnull if none.
+ */
+ contup = findNotNullConstraintAttnum(RelationGetRelid(rel), attnum);
+ if (HeapTupleIsValid(contup))
+ {
+ heap_freetuple(contup);
+ heap_freetuple(atttup);
+ continue;
+ }

I am a little bit confused by the above comment.
I think the comments should say,
if contup is valid, that means, we already have one "not null"
constraint associate with the attnum
in that condition, we must not set attnotnull, otherwise the
consistency between attnotnull and "not null"
table constraint will be broken.

other than that, the change in RemoveConstraintById looks sane.

jian he <jian.universality@gmail.com> 于2024年3月29日周五 14:56写道：

hi.
about v4, i think, i understand the changes you made.
RemoveConstraintById(Oid conId)
will drop a single constraint record.
if the constraint is primary key, then primary key associated
attnotnull should set to false.
but sometimes it shouldn't.

Yeah, indeed.

for example:
drop table if exists t2;
CREATE TABLE t2(c0 int, c1 int);
ALTER TABLE t2 ADD CONSTRAINT t2_pk PRIMARY KEY(c0, c1);
ALTER TABLE t2 ALTER COLUMN c0 ADD GENERATED ALWAYS AS IDENTITY;
ALTER TABLE t2 DROP c1;

+ * If this was a NOT NULL or the primary key, the constrained columns must
+ * have had pg_attribute.attnotnull set.  See if we need to reset it, and
+ * do so.
+ */
+ if (unconstrained_cols != NIL)

unconstrained_cols is not NIL, which means we have dropped a primary key.
I am confused by "If this was a NOT NULL or the primary key".

NOT NULL means the definition of column having not-null constranit. For
example:
create table t1(a int not null);
the pg_attribute.attnotnull is set to true.
primary key case like below:
create table t2(a int primary key);
the pg_attribute.attnotnull is set to true.

I think aboved case can explain what's meaning about comments in
dropconstraint_internal.
But here, in RemoveConstraintById() , we only care about primary key case,
so NOT NULL is better
to removed from comments.

+
+ /*
+ * Since the above deletion has been made visible, we can now
+ * search for any remaining constraints on this column (or these
+ * columns, in the case we're dropping a multicol primary key.)
+ * Then, verify whether any further NOT NULL exists, and reset
+ * attnotnull if none.
+ */
+ contup = findNotNullConstraintAttnum(RelationGetRelid(rel), attnum);
+ if (HeapTupleIsValid(contup))
+ {
+ heap_freetuple(contup);
+ heap_freetuple(atttup);
+ continue;
+ }

I am a little bit confused by the above comment.
I think the comments should say,
if contup is valid, that means, we already have one "not null"
constraint associate with the attnum
in that condition, we must not set attnotnull, otherwise the
consistency between attnotnull and "not null"
table constraint will be broken.

other than that, the change in RemoveConstraintById looks sane.

Above comments want to say that after pk constranit dropped, if there are
tuples in
pg_constraint, that means the definition of column has not-null constraint.
So we can't
set pg_attribute.attnotnull to false.

For example:
create table t1(a int not null);
alter table t1 add constraint t1_pk primary key(a);
alter table t1 drop constraint t1_pk;

--
Tender Wang
OpenPie: https://en.openpie.com/

It has been several days since the last email. Do you have any
suggestions, please?

What I'm concerned about is that adding a new AT_PASS is good fix? Is it
a big try?
More concerned is that it can cover all ALTER TABLE cases?

Any thoughts.
--
Tender Wang
OpenPie: https://en.openpie.com/

On 2024-Mar-29, Tender Wang wrote:

I think aboved case can explain what's meaning about comments in
dropconstraint_internal.
But here, in RemoveConstraintById() , we only care about primary key case,
so NOT NULL is better to removed from comments.

Actually, I think it's better if all the resets of attnotnull occur in
RemoveConstraintById, for both types of constraints; we would keep that
block in dropconstraint_internal only to raise errors in the cases where
the constraint is protecting a replica identity or a generated column.
Something like the attached, perhaps, may need more polish.

I'm not really sure about the business of adding a new pass value
-- it's clear that things don't work if we don't do *something* -- I'm
just not sure if this is the something that we want to do.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)

On Wed, Apr 10, 2024 at 1:29 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2024-Mar-29, Tender Wang wrote:

I think aboved case can explain what's meaning about comments in
dropconstraint_internal.
But here, in RemoveConstraintById() , we only care about primary key case,
so NOT NULL is better to removed from comments.

Actually, I think it's better if all the resets of attnotnull occur in
RemoveConstraintById, for both types of constraints; we would keep that
block in dropconstraint_internal only to raise errors in the cases where
the constraint is protecting a replica identity or a generated column.
Something like the attached, perhaps, may need more polish.

DROP TABLE if exists notnull_tbl2;
CREATE TABLE notnull_tbl2 (c0 int generated by default as IDENTITY, c1 int);
ALTER TABLE notnull_tbl2 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
ALTER TABLE notnull_tbl2 DROP CONSTRAINT notnull_tbl2_c0_not_null;
ALTER TABLE notnull_tbl2 DROP c1;
\d notnull_tbl2

last "\d notnull_tbl2" command, master output is:
Table "public.notnull_tbl2"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+----------------------------------
c0 | integer | | not null | generated by default as identity

last "\d notnull_tbl2" command, applying
0001-Correctly-reset-attnotnull-when-constraints-dropped-.patch
output:
Table "public.notnull_tbl2"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+----------------------------------
c0 | integer | | | generated by default as identity

there may also have similar issues with the replicate identity.

jian he <jian.universality@gmail.com> 于2024年4月10日周三 14:10写道：

On Wed, Apr 10, 2024 at 1:29 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2024-Mar-29, Tender Wang wrote:

I think aboved case can explain what's meaning about comments in
dropconstraint_internal.
But here, in RemoveConstraintById() , we only care about primary key

case,

so NOT NULL is better to removed from comments.

Actually, I think it's better if all the resets of attnotnull occur in
RemoveConstraintById, for both types of constraints; we would keep that
block in dropconstraint_internal only to raise errors in the cases where
the constraint is protecting a replica identity or a generated column.
Something like the attached, perhaps, may need more polish.

DROP TABLE if exists notnull_tbl2;
CREATE TABLE notnull_tbl2 (c0 int generated by default as IDENTITY, c1
int);
ALTER TABLE notnull_tbl2 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
ALTER TABLE notnull_tbl2 DROP CONSTRAINT notnull_tbl2_c0_not_null;
ALTER TABLE notnull_tbl2 DROP c1;
\d notnull_tbl2

last "\d notnull_tbl2" command, master output is:
Table "public.notnull_tbl2"
Column | Type | Collation | Nullable | Default

--------+---------+-----------+----------+----------------------------------
c0 | integer | | not null | generated by default as identity

last "\d notnull_tbl2" command, applying
0001-Correctly-reset-attnotnull-when-constraints-dropped-.patch
output:
Table "public.notnull_tbl2"
Column | Type | Collation | Nullable | Default

--------+---------+-----------+----------+----------------------------------
c0 | integer | | | generated by default as identity

Hmm,
ALTER TABLE notnull_tbl2 DROP c1; will not call dropconstraint_internal().
When dropping PK constraint indirectly, c0's attnotnull was set to false in
RemoveConstraintById().

--
Tender Wang
OpenPie: https://en.openpie.com/

another related bug, in master.

drop table if exists notnull_tbl1;
CREATE TABLE notnull_tbl1 (c0 int not null, c1 int);
ALTER TABLE notnull_tbl1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
\d+ notnull_tbl1
ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;
ALTER TABLE notnull_tbl1 ALTER c1 DROP NOT NULL;

"ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;"
should fail?

I didn't investigate deep enough.

jian he <jian.universality@gmail.com> 于2024年4月10日周三 17:34写道：

another related bug, in master.

drop table if exists notnull_tbl1;
CREATE TABLE notnull_tbl1 (c0 int not null, c1 int);
ALTER TABLE notnull_tbl1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
\d+ notnull_tbl1
ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;
ALTER TABLE notnull_tbl1 ALTER c1 DROP NOT NULL;

"ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;"
should fail?

Yeah, it should fail as before, because c0 is primary key.
In master, although c0's pg_attribute.attnotnull is still true, but its
not-null constraint has been deleted
in dropconstraint_internal().

If we drop column c1 after dropping c0 not null, the primary key will be
dropped indirectly.
And now you can see c0 is still not-null if you do \d+ notnull_tbl1. But it
will report error "not found not-null"
if you alter c0 drop not null.

postgres=# ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;
ALTER TABLE
postgres=# \d+ notnull_tbl1
Table "public.notnull_tbl1"
Column | Type | Collation | Nullable | Default | Storage | Compression
| Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
c0 | integer | | not null | | plain |
| |
c1 | integer | | not null | | plain |
| |
Indexes:
"q" PRIMARY KEY, btree (c0, c1)
Access method: heap

postgres=# alter table notnull_tbl1 drop c1;
ALTER TABLE
postgres=# \d notnull_tbl1
Table "public.notnull_tbl1"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
c0 | integer | | not null |

postgres=# alter table notnull_tbl1 alter c0 drop not null;
ERROR: could not find not-null constraint on column "c0", relation
"notnull_tbl1"

--
Tender Wang
OpenPie: https://en.openpie.com/

On 2024-Apr-10, Tender Wang wrote:

Yeah, it should fail as before, because c0 is primary key.
In master, although c0's pg_attribute.attnotnull is still true, but its
not-null constraint has been deleted
in dropconstraint_internal().

Yeah, the problem here is that we need to do the checks whenever the
constraints are dropped, either directly or indirectly ... but we cannot
do them in RemoveConstraintById, because if you elog(ERROR) there, it
won't let you use DROP TABLE (which will also arrive at
RemoveConstraintById):

55490 17devel 2220048=# drop table notnull_tbl2;
ERROR: column "c0" of relation "notnull_tbl2" is an identity column

... which is of course kinda ridiculous, so this is not a viable
alternative. The problem is that RemoveConstraintById doesn't have
sufficient context about the whole operation. Worse: it cannot feed
its operations back into the alter table state.

I had a thought yesterday about making the resets of attnotnull and the
tests for replica identity and PKs to a separate ALTER TABLE pass,
independent of RemoveConstraintById (which would continue to be
responsible only for dropping the catalog row, as currently).
This may make the whole thing simpler. I'm on it.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)

On 2024-Apr-10, jian he wrote:

another related bug, in master.

drop table if exists notnull_tbl1;
CREATE TABLE notnull_tbl1 (c0 int not null, c1 int);
ALTER TABLE notnull_tbl1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
\d+ notnull_tbl1
ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;
ALTER TABLE notnull_tbl1 ALTER c1 DROP NOT NULL;

"ALTER TABLE notnull_tbl1 ALTER c0 DROP NOT NULL;"
should fail?

No, this should not fail, and it is working correctly in master. You
can drop the not-null constraint, but the column will still be
non-nullable, because the primary key still exists. If you drop the
primary key later, then the column becomes nullable. This is by design.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"El miedo atento y previsor es la madre de la seguridad" (E. Burke)