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

jian he <jian(dot)universality(at)gmail(dot)com> 于2024年3月28日周四 13:18写道：

> On Wed, Mar 27, 2024 at 10:26 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
> >
> > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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/