Обсуждение: 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"

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?

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)

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.

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.

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)

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)

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.

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

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?

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.

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)

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)

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.

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)

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

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"

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