jian he <jian.universal...@gmail.com> 于2024年3月28日周四 13:18写道:
> On Wed, Mar 27, 2024 at 10:26 PM Tender Wang <tndrw...@gmail.com> wrote: > > > > Alvaro Herrera <alvhe...@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/