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

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?