Horton

Thread

Commits

  1. Better handle indirect constraint drops

  2. Add macros for looping through a List without a ListCell.

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

    Tender Wang <tndrwang@gmail.com> — 2024-03-26T12:00:36Z

    Hi Alvaro,

I met an issue related to Catalog not-null commit on HEAD.

postgres=# CREATE TABLE t1(c0 int, c1 int);
CREATE TABLE
postgres=# ALTER TABLE  t1 ADD CONSTRAINT Q PRIMARY KEY(c0, c1);
ALTER TABLE
postgres=# \d+ t1
                                           Table "public.t1"
 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  t1 DROP c1;
ALTER TABLE
postgres=# \d+ t1
                                           Table "public.t1"
 Column |  Type   | Collation | Nullable | Default | Storage | Compression
| Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
 c0     | integer |           | not null |         | plain   |
|              |
Access method: heap

postgres=# ALTER TABLE  t1  ALTER c0 DROP NOT NULL;
ERROR:  could not find not-null constraint on column "c0", relation "t1"
postgres=# insert into t1 values (NULL);
ERROR:  null value in column "c0" of relation "t1" violates not-null
constraint
DETAIL:  Failing row contains (null).

I couldn't reproduce aboved issue on older version(12.12 ...16.1).
to be more precisely， since  b0e96f3119 commit.

Without the b0e96f3119, when we drop not null constraint, we  just update
the pg_attribute attnotnull to false
in ATExecDropNotNull().  But now we first check pg_constraint if has the
tuple. if attnotnull is ture, but pg_constraint
doesn't has that tuple. Aboved error will report.

It will be confuesed for users. Because \d shows the column c0 has not
null, and we cann't insert NULL value. But it
reports errore when users drop the NOT NULL constraint.

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

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

  2. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-03-26T15:25:42Z

    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"

  3. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-03-27T03:33:29Z

    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/

  4. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-03-27T08:21:35Z

    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/

  5. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-03-27T14:26:10Z

    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/

  6. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-03-28T05:18:41Z

    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?

  7. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-03-28T06:13:48Z

    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/

  8. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-03-28T08:32:41Z

    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/

  9. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-03-28T09:18:38Z

    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)

  10. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-03-28T12:05:04Z

    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/

  11. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-03-29T06:56:47Z

    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.

  12. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-03-29T07:51:39Z

    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/

  13. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-04-07T10:33:30Z

    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/

  14. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-09T17:29:03Z

    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)

  15. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-04-10T06:10:23Z

    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.

  16. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-04-10T06:36:33Z

    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/

  17. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-04-10T09:34:30Z

    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.

  18. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-04-10T10:11:02Z

    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/

  19. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-10T10:54:13Z

    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)

  20. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-10T11:01:52Z

    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)

  21. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-10T13:58:49Z

    It turns out that trying to close all holes that lead to columns marked
not-null without a pg_constraint row is not possible within the ALTER
TABLE framework, because it can happen outside it also.  Consider this

CREATE DOMAIN dom1 AS integer;
CREATE TABLE notnull_tbl (a dom1, b int, PRIMARY KEY (a, b));
DROP DOMAIN dom1 CASCADE;

In this case you'll end up with b having attnotnull=true and no
constraint; and no amount of messing with tablecmds.c will fix it.

So I propose to instead allow those constraints, and treat them as
second-class citizens.  We allow dropping them with ALTER TABLE DROP NOT
NULL, and we allow to create a backing full-fledged constraint with SET
NOT NULL or ADD CONSTRAINT.  So here's a partial crude initial patch to
do that.


One thing missing here is pg_dump support.  If you just dump this table,
it'll end up with no constraint at all.  That's obviously bad, so I
propose we have pg_dump add a regular NOT NULL constraint for those, to
avoid perpetuating the weird situation further.

Another thing I wonder if whether I should use the existing
set_attnotnull() instead of adding drop_orphaned_notnull().  Or we could
just inline the code in ATExecDropNotNull, since it's small and
self-contained.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
 PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)

  22. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-04-10T15:03:30Z

    On Wed, Apr 10, 2024 at 7:01 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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.
>

now I got it. the second time, it will fail.
it should be the expected behavior.

per commit:
https://git.postgresql.org/cgit/postgresql.git/commit/?id=14dd0f27d7cd56ffae9ecdbe324965073d01a9ff
In the function dropconstraint_internal, I changed "foreach" to
"foreach_int" in some places,
and other minor cosmetic changes within the function
dropconstraint_internal only.

Since I saw your changes in dropconstraint_internal, I posted here.
I will review your latest patch later.

  23. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-10T17:23:25Z

    On 2024-Apr-10, Alvaro Herrera wrote:

> One thing missing here is pg_dump support.  If you just dump this table,
> it'll end up with no constraint at all.  That's obviously bad, so I
> propose we have pg_dump add a regular NOT NULL constraint for those, to
> avoid perpetuating the weird situation further.

Here's another crude patchset, this time including the pg_dump aspect.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"On the other flipper, one wrong move and we're Fatal Exceptions"
(T.U.X.: Term Unit X  - http://www.thelinuxreview.com/TUX/)

  24. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-04-11T02:18:21Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年4月10日周三 21:58写道：

> It turns out that trying to close all holes that lead to columns marked
> not-null without a pg_constraint row is not possible within the ALTER
> TABLE framework, because it can happen outside it also.  Consider this
>
> CREATE DOMAIN dom1 AS integer;
> CREATE TABLE notnull_tbl (a dom1, b int, PRIMARY KEY (a, b));
> DROP DOMAIN dom1 CASCADE;
>
> In this case you'll end up with b having attnotnull=true and no
> constraint; and no amount of messing with tablecmds.c will fix it.
>

I try above case on my v4 patch[1], and it seems no result as what you said.
But, anyway, I now don't like updating other catalog in
RemoveConstraintById().
Because it will not be friendly for others who call RemoveConstraintById()
want only
to remove pg_constraint tuple, but actually it do more works stealthily.


> So I propose to instead allow those constraints, and treat them as
> second-class citizens.  We allow dropping them with ALTER TABLE DROP NOT
> NULL, and we allow to create a backing full-fledged constraint with SET
> NOT NULL or ADD CONSTRAINT.  So here's a partial crude initial patch to
> do that.
>

Hmm, the patch looks like the patch in my first email in this thread. But
my v1 patch seem
a poc at most.

>
>
> One thing missing here is pg_dump support.  If you just dump this table,
> it'll end up with no constraint at all.  That's obviously bad, so I
> propose we have pg_dump add a regular NOT NULL constraint for those, to
> avoid perpetuating the weird situation further.
>
> Another thing I wonder if whether I should use the existing
> set_attnotnull() instead of adding drop_orphaned_notnull().  Or we could
> just inline the code in ATExecDropNotNull, since it's small and
> self-contained.
>

I like just inline the code in  ATExecDropNotNull, as you said, it's small
and self-contained.
in ATExecDropNotNull(), we had open the pg_attributed table and hold
RowExclusiveLock,
the tuple we also get.
What we do is set attnotnull = false, and call CatalogTupleUpdate.

-- 
> Álvaro Herrera               48°01'N 7°57'E  —
> https://www.EnterpriseDB.com/
> "Postgres is bloatware by design: it was built to house
>  PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)
>

[1]
https://www.postgresql.org/message-id/CAHewXNn_So7LUCxxxyDNfdvCQp1TnD3gTVECBZX2bT_nbPgraQ%40mail.gmail.com
-- 
Tender Wang
OpenPie:  https://en.openpie.com/

  25. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-04-11T06:40:04Z

    On Wed, Apr 10, 2024 at 2:10 PM jian he <jian.universality@gmail.com> wrote:
>
> 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

> ALTER TABLE notnull_tbl2 DROP CONSTRAINT notnull_tbl2_c0_not_null;
per above sequence execution order, this should error out?

otherwise which "not null" (attribute|constraint) to anchor "generated
by default as identity" not null property?
"DROP c1" will drop the not null property for "c0" and "c1".
if "DROP CONSTRAINT notnull_tbl2_c0_not_nul" not error out, then
" ALTER TABLE notnull_tbl2 DROP c1;"
should either error out
or transform "c0" from "c0 int generated by default as identity"
to
"c0 int"


On Thu, Apr 11, 2024 at 1:23 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2024-Apr-10, Alvaro Herrera wrote:
>
> > One thing missing here is pg_dump support.  If you just dump this table,
> > it'll end up with no constraint at all.  That's obviously bad, so I
> > propose we have pg_dump add a regular NOT NULL constraint for those, to
> > avoid perpetuating the weird situation further.
>
> Here's another crude patchset, this time including the pg_dump aspect.
>

+DROP TABLE notnull_tbl1;
+-- make sure attnotnull is reset correctly when a PK is dropped indirectly
+CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
+ALTER TABLE  notnull_tbl1 DROP c1;
+\d+ notnull_tbl1
+                               Table "public.notnull_tbl1"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ c0     | integer |           | not null |         | plain   |              |
+

this is not what we expected?
"not null" for "c0" now should be false?
am I missing something?

  26. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-04-11T07:19:35Z

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

> On Wed, Apr 10, 2024 at 2:10 PM jian he <jian.universality@gmail.com>
> wrote:
> >
> > 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
>
> > ALTER TABLE notnull_tbl2 DROP CONSTRAINT notnull_tbl2_c0_not_null;
> per above sequence execution order, this should error out?
>
> otherwise which "not null" (attribute|constraint) to anchor "generated
> by default as identity" not null property?
> "DROP c1" will drop the not null property for "c0" and "c1".
> if "DROP CONSTRAINT notnull_tbl2_c0_not_nul" not error out, then
> " ALTER TABLE notnull_tbl2 DROP c1;"
> should either error out
> or transform "c0" from "c0 int generated by default as identity"
> to
> "c0 int"
>
> I try above case on MASTER and MASTER with Alvaro V2 patch, and all work
correctly.
\d+ notnull_tbl2 will see not-null of "c0".


>
> On Thu, Apr 11, 2024 at 1:23 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> >
> > On 2024-Apr-10, Alvaro Herrera wrote:
> >
> > > One thing missing here is pg_dump support.  If you just dump this
> table,
> > > it'll end up with no constraint at all.  That's obviously bad, so I
> > > propose we have pg_dump add a regular NOT NULL constraint for those, to
> > > avoid perpetuating the weird situation further.
> >
> > Here's another crude patchset, this time including the pg_dump aspect.
> >
>
> +DROP TABLE notnull_tbl1;
> +-- make sure attnotnull is reset correctly when a PK is dropped indirectly
> +CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
> +ALTER TABLE  notnull_tbl1 DROP c1;
> +\d+ notnull_tbl1
> +                               Table "public.notnull_tbl1"
> + Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
>
> +--------+---------+-----------+----------+---------+---------+--------------+-------------
> + c0     | integer |           | not null |         | plain   |
>   |
> +
>
> this is not what we expected?
> "not null" for "c0" now should be false?
> am I missing something?
>
Yeah, now this is expected behavior.
Users can  drop manually not-null of "c0" if they want, and no error
reporte.

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

  27. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-04-11T08:49:35Z

    On Thu, Apr 11, 2024 at 3:19 PM Tender Wang <tndrwang@gmail.com> wrote:
>
>> +DROP TABLE notnull_tbl1;
>> +-- make sure attnotnull is reset correctly when a PK is dropped indirectly
>> +CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
>> +ALTER TABLE  notnull_tbl1 DROP c1;
>> +\d+ notnull_tbl1
>> +                               Table "public.notnull_tbl1"
>> + Column |  Type   | Collation | Nullable | Default | Storage | Stats
>> target | Description
>> +--------+---------+-----------+----------+---------+---------+--------------+-------------
>> + c0     | integer |           | not null |         | plain   |              |
>> +
>>
>> this is not what we expected?
>> "not null" for "c0" now should be false?
>> am I missing something?
>
> Yeah, now this is expected behavior.
> Users can  drop manually not-null of "c0" if they want, and no error reporte.
>

sorry for the noise.
these two past patches confused me:
0001-Correctly-reset-attnotnull-when-constraints-dropped-.patch
v4-0001-Fix-pg_attribute-attnotnull-not-reset-when-droppe.patch

I thought dropping a column of primary key (ALTER TABLE notnull_tbl2 DROP c1)
will make the others key columns to not have "not null" property.

now I figured out that
dropping a column of primary key columns will not change other key
columns'  "not null" property.
dropping the primary key associated constraint will make all key
columns "not null" property disappear.

v2-0001-Handle-ALTER-.-DROP-NOT-NULL-when-no-pg_constrain.patch
behavior looks fine to me now.


inline drop_orphaned_notnull in ATExecDropNotNull looks fine to me.

  28. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-11T09:10:17Z

    On 2024-Apr-11, jian he wrote:

> now I figured out that
> dropping a column of primary key columns will not change other key
> columns'  "not null" property.
> dropping the primary key associated constraint will make all key
> columns "not null" property disappear.

Well, I think you were right that we should try to handle the situation
of unmarking attnotnull as much as possible, to decrease the chances
that the problematic situation occurs.  That means, we can use the
earlier code to handle DROP COLUMN when it causes a PK to be dropped --
even though we still need to handle the situation of an attnotnull flag
set with no pg_constraint row.  I mean, we still have to handle DROP
DOMAIN correctly (and there might be other cases that I haven't thought
about) ... but surely this is a much less common situation than the one
you reported.  So I'll merge everything and post an updated patch.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.              (Don Knuth)

  29. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-11T14:48:00Z

    On 2024-Apr-11, Alvaro Herrera wrote:

> Well, I think you were right that we should try to handle the situation
> of unmarking attnotnull as much as possible, to decrease the chances
> that the problematic situation occurs.  That means, we can use the
> earlier code to handle DROP COLUMN when it causes a PK to be dropped --
> even though we still need to handle the situation of an attnotnull flag
> set with no pg_constraint row.  I mean, we still have to handle DROP
> DOMAIN correctly (and there might be other cases that I haven't thought
> about) ... but surely this is a much less common situation than the one
> you reported.  So I'll merge everything and post an updated patch.

Here's roughly what I'm thinking.  If we drop a constraint, we can still
reset attnotnull in RemoveConstraintById(), but only after checking that
it's not a generated column or a replica identity.  If they are, we
don't error out -- just skip the attnotnull update.

Now, about the code to allow ALTER TABLE DROP NOT NULL in case there's
no pg_constraint row, I think at this point it's mostly dead code,
because it can only happen when you have a replica identity or generated
column ... and the DROP NOT NULL should still prevent you from dropping
the flag anyway.  But the case can still arise, if you change the
replica identity or ALTER TABLE ALTER COLUMN DROP DEFAULT, respectively.

I'm still not ready with this -- still not convinced about the new AT
pass.  Also, I want to add a test for the pg_dump behavior, and there's
an XXX comment.

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

  30. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-04-12T02:11:54Z

    On Thu, Apr 11, 2024 at 10:48 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
>
> I'm still not ready with this -- still not convinced about the new AT
> pass.  Also, I want to add a test for the pg_dump behavior, and there's
> an XXX comment.
>
Now I am more confused...

+-- make sure attnotnull is reset correctly when a PK is dropped indirectly,
+-- or kept if there's a reason for that
+CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
+ALTER TABLE  notnull_tbl1 DROP c1;
+\d+ notnull_tbl1
+                               Table "public.notnull_tbl1"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ c0     | integer |           |          |         | plain   |              |
+
+DROP TABLE notnull_tbl1;

same query, mysql make let "c0" be not null
mysql https://dbfiddle.uk/_ltoU7PO

for postgre
https://dbfiddle.uk/ZHJXEqL1
from 9.3 to 16 (click the link (https://dbfiddle.uk/ZHJXEqL1), then
click "9.3" choose which version you like)
all will make the remaining column "co" be not null.

latest
0001-Better-handle-indirect-constraint-drops.patch make c0 attnotnull be false.

previous patches:
v2-0001-Handle-ALTER-.-DROP-NOT-NULL-when-no-pg_constrain.patch  make
c0 attnotnull be true.
0001-Correctly-reset-attnotnull-when-constraints-dropped-.patch make
c0 attnotnull be false.

  31. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-04-12T03:26:00Z

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

> On Thu, Apr 11, 2024 at 10:48 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> >
> >
> > I'm still not ready with this -- still not convinced about the new AT
> > pass.  Also, I want to add a test for the pg_dump behavior, and there's
> > an XXX comment.
> >
> Now I am more confused...
>
> +-- make sure attnotnull is reset correctly when a PK is dropped
> indirectly,
> +-- or kept if there's a reason for that
> +CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
> +ALTER TABLE  notnull_tbl1 DROP c1;
> +\d+ notnull_tbl1
> +                               Table "public.notnull_tbl1"
> + Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
>
> +--------+---------+-----------+----------+---------+---------+--------------+-------------
> + c0     | integer |           |          |         | plain   |
>   |
> +
> +DROP TABLE notnull_tbl1;
>
> same query, mysql make let "c0" be not null
> mysql https://dbfiddle.uk/_ltoU7PO
>
> for postgre
> https://dbfiddle.uk/ZHJXEqL1
> from 9.3 to 16 (click the link (https://dbfiddle.uk/ZHJXEqL1), then
> click "9.3" choose which version you like)
> all will make the remaining column "co" be not null.
>
> latest
> 0001-Better-handle-indirect-constraint-drops.patch make c0 attnotnull be
> false.
>
> previous patches:
> v2-0001-Handle-ALTER-.-DROP-NOT-NULL-when-no-pg_constrain.patch  make
> c0 attnotnull be true.
> 0001-Correctly-reset-attnotnull-when-constraints-dropped-.patch make
> c0 attnotnull be false.
>

I'm not sure that SQL standard specifies what database must do for this
case.
If the standard does not specify, then it depends on each database vendor's
decision.

Some people like not-null retained, other people may like not-null removed.
I think it will be ok if people can drop not-null or add not-null back
again after dropping pk.

In Master, not-null will reset when we drop PK directly. I hope dropping pk
indirectly
is consistent with dropping PK directly.

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

  32. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-04-12T06:02:51Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年4月11日周四 22:48写道：

> On 2024-Apr-11, Alvaro Herrera wrote:
>
> > Well, I think you were right that we should try to handle the situation
> > of unmarking attnotnull as much as possible, to decrease the chances
> > that the problematic situation occurs.  That means, we can use the
> > earlier code to handle DROP COLUMN when it causes a PK to be dropped --
> > even though we still need to handle the situation of an attnotnull flag
> > set with no pg_constraint row.  I mean, we still have to handle DROP
> > DOMAIN correctly (and there might be other cases that I haven't thought
> > about) ... but surely this is a much less common situation than the one
> > you reported.  So I'll merge everything and post an updated patch.
>
> Here's roughly what I'm thinking.  If we drop a constraint, we can still
> reset attnotnull in RemoveConstraintById(), but only after checking that
> it's not a generated column or a replica identity.  If they are, we
> don't error out -- just skip the attnotnull update.
>
> Now, about the code to allow ALTER TABLE DROP NOT NULL in case there's
> no pg_constraint row, I think at this point it's mostly dead code,
> because it can only happen when you have a replica identity or generated
> column ... and the DROP NOT NULL should still prevent you from dropping
> the flag anyway.  But the case can still arise, if you change the
> replica identity or ALTER TABLE ALTER COLUMN DROP DEFAULT, respectively.
>
> I'm still not ready with this -- still not convinced about the new AT
> pass.


Yeah, at first, I was also hesitant. Two reasons make me convinced.
in ATPostAlterTypeParse()
-----
                               else if (cmd->subtype == AT_SetAttNotNull)
                                {
                                        /*
                                         * 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.
                                         */
                                }
-------
The new table difinition continues to use old column not-null, so here does
nothing.
If we reset NOT NULL marks in RemoveConstrainById() when dropping PK
indirectly,
we need to do something here or somewhere else.

Except AT_SetAttNotNull type, other types add a AT pass to tab->subcmds.
Because
not-null should be added before re-adding index, there is no right AT pass
in current AlterTablePass.
So a new AT pass ahead AT_PASS_OLD_INDEX  is needed.

Another reason is that it can use ALTER TABLE frame to set not-null.
This way looks simpler and better than hardcode to re-install not-null in
some funciton.

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

  33. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-12T07:52:05Z

    On 2024-Apr-12, jian he wrote:

> Now I am more confused...

> +CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
> +ALTER TABLE  notnull_tbl1 DROP c1;

> same query, mysql make let "c0" be not null

Yes, that was Postgres' old model.  But the way we think of it now, is
that a column is marked attnotnull when a pg_constraint entry exists to
support that flag, which can be a not-null constraint, or a primary key
constraint.  In the old Postgres model, you're right that we would
continue to have c0 as not-null, just like mysql.  In the new model,
that flag no longer has no reason to be there, because the backing
primary key constraint has been removed, which is why we reset it.

So what I was saying in the cases with replica identity and generated
columns, is that there's an attnotnull flag we cannot remove, because of
either of those things, but we don't have any backing constraint for it,
which is an inconsistency with the view of the world that I described
above.  I would like to manufacture one not-null constraint at that
point, or just abort the drop of the PK ... but I don't see how to do
either of those things.


If you want the c0 column to be still not-null after dropping the
primary key, you need to SET NOT NULL:

CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));                                                                                           
ALTER TABLE notnull_tbl1 ALTER c0 SET NOT NULL;
ALTER TABLE  notnull_tbl1 DROP c1;
\d+ notnull_tbl1
                                      Table "public.notnull_tbl1"
 Column │  Type   │ Collation │ Nullable │ Default │ Storage │ Compression │ Stats target │ Description 
────────┼─────────┼───────────┼──────────┼─────────┼─────────┼─────────────┼──────────────┼─────────────
 c0     │ integer │           │ not null │         │ plain   │             │              │ 
Not-null constraints:
    "notnull_tbl1_c0_not_null" NOT NULL "c0"
Access method: heap


One thing that's not quite ideal, is that the "Nullable" column doesn't
make it obvious that the flag is going to be removed if you drop the PK;
you have to infer that that's going to happen by noticing that there's
no explicit not-null constraint listed for that column -- maybe too
subtle, especially if you have a lot of columns (luckily, PKs normally
don't have too many columns).  This is why I suggested to change the
contents of that column if the flag is sustained by the PK.  Something
like this, perhaps:

=# CREATE TABLE notnull_tbl1 (c0 int not null, c1 int, PRIMARY KEY (c0, c1));                                                                                              
=# \d+ notnull_tbl1
                                      Table "public.notnull_tbl1"
 Column │  Type   │ Collation │   Nullable  │ Default │ Storage │ Compression │ Stats target │ Description 
────────┼─────────┼───────────┼─────────────┼─────────┼─────────┼─────────────┼──────────────┼─────────────
 c0     │ integer │           │ not null    │         │ plain   │             │              │ 
 c1     │ integer │           │ primary key │         │ plain   │             │              │ 
Indexes:
    "notnull_tbl1_pkey" PRIMARY KEY, btree (c0, c1)
Not-null constraints:
    "notnull_tbl1_c0_not_null" NOT NULL "c0"
Access method: heap

which should make it obvious.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Right now the sectors on the hard disk run clockwise, but I heard a rumor that
you can squeeze 0.2% more throughput by running them counterclockwise.
It's worth the effort. Recommended."  (Gerry Pourwelle)

  34. Re: Can't find not null constraint, but \d+ shows that

    jian he <jian.universality@gmail.com> — 2024-04-13T00:00:00Z

    On Fri, Apr 12, 2024 at 3:52 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2024-Apr-12, jian he wrote:
>
> > Now I am more confused...
>
> > +CREATE TABLE notnull_tbl1 (c0 int, c1 int, PRIMARY KEY (c0, c1));
> > +ALTER TABLE  notnull_tbl1 DROP c1;
>
> > same query, mysql make let "c0" be not null
>
> Yes, that was Postgres' old model.  But the way we think of it now, is
> that a column is marked attnotnull when a pg_constraint entry exists to
> support that flag, which can be a not-null constraint, or a primary key
> constraint.  In the old Postgres model, you're right that we would
> continue to have c0 as not-null, just like mysql.  In the new model,
> that flag no longer has no reason to be there, because the backing
> primary key constraint has been removed, which is why we reset it.
>
> So what I was saying in the cases with replica identity and generated
> columns, is that there's an attnotnull flag we cannot remove, because of
> either of those things, but we don't have any backing constraint for it,
> which is an inconsistency with the view of the world that I described
> above.  I would like to manufacture one not-null constraint at that
> point, or just abort the drop of the PK ... but I don't see how to do
> either of those things.
>

thanks for your explanation.
now I understand it.
I wonder is there any incompatibility issue, or do we need to say something
about the new behavior when dropping a key column?

the comments look good to me.

only minor cosmetic issue:
+ if (unconstrained_cols)
i would like change it to
+ if (unconstrained_cols != NIL)

+ foreach(lc, unconstrained_cols)
we can change to
+ foreach_int(attnum, unconstrained_cols)
per commit
https://git.postgresql.org/cgit/postgresql.git/commit/?id=14dd0f27d7cd56ffae9ecdbe324965073d01a9ff

  35. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-18T18:49:52Z

    On 2024-Apr-13, jian he wrote:

> I wonder is there any incompatibility issue, or do we need to say something
> about the new behavior when dropping a key column?

Umm, yeah, maybe we should document it in ALTER TABLE DROP PRIMARY KEY
and in the release notes to note the different behavior.

> only minor cosmetic issue:
> + if (unconstrained_cols)
> i would like change it to
> + if (unconstrained_cols != NIL)
> 
> + foreach(lc, unconstrained_cols)
> we can change to
> + foreach_int(attnum, unconstrained_cols)
> per commit
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=14dd0f27d7cd56ffae9ecdbe324965073d01a9ff

Ah, yeah.  I did that, rewrote some comments and refined the tests a
little bit to ensure the pg_upgrade behavior is sane.  I intend to get
this pushed tomorrow, if nothing ugly comes up.

CI run: https://cirrus-ci.com/build/5471117953990656

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

  36. Re: Can't find not null constraint, but \d+ shows that

    Tender Wang <tndrwang@gmail.com> — 2024-04-19T06:41:26Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> 于2024年4月19日周五 02:49写道：

> On 2024-Apr-13, jian he wrote:
>
> > I wonder is there any incompatibility issue, or do we need to say
> something
> > about the new behavior when dropping a key column?
>
> Umm, yeah, maybe we should document it in ALTER TABLE DROP PRIMARY KEY
> and in the release notes to note the different behavior.
>
> > only minor cosmetic issue:
> > + if (unconstrained_cols)
> > i would like change it to
> > + if (unconstrained_cols != NIL)
> >
> > + foreach(lc, unconstrained_cols)
> > we can change to
> > + foreach_int(attnum, unconstrained_cols)
> > per commit
> >
> https://git.postgresql.org/cgit/postgresql.git/commit/?id=14dd0f27d7cd56ffae9ecdbe324965073d01a9ff
>
> Ah, yeah.  I did that, rewrote some comments and refined the tests a
> little bit to ensure the pg_upgrade behavior is sane.  I intend to get
> this pushed tomorrow, if nothing ugly comes up.
>

The new patch looks good to me.


>
> CI run: https://cirrus-ci.com/build/5471117953990656
>
>

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

  37. Re: Can't find not null constraint, but \d+ shows that

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-04-19T10:40:11Z

    On 2024-Apr-19, Tender Wang wrote:

> The new patch looks good to me.

Thanks for looking once more.  I have pushed it now.  I didn't try
pg_upgrade other than running the tests, so maybe buildfarm member crake
will have more to complain about -- we'll see.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php