diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 8ca2ab13eb4..4d1ecbaa935 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -290,8 +290,9 @@ static void AlterSeqNamespaces(Relation classRel, Relation rel, LOCKMODE lockmode); static ObjectAddress ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode); -static ObjectAddress ATExecValidateConstraint(Relation rel, char *constrName, - bool recurse, bool recursing, LOCKMODE lockmode); +static ObjectAddress ATExecValidateConstraint(List **wqueue, Relation rel, + char *constrName, bool recurse, bool recursing, + LOCKMODE lockmode); static int transformColumnNameList(Oid relId, List *colList, int16 *attnums, Oid *atttypids); static int transformFkeyGetPrimaryKey(Relation pkrel, Oid *indexOid, @@ -304,7 +305,6 @@ static Oid transformFkeyCheckAttrs(Relation pkrel, static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts); static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId, Oid *funcid); -static void validateCheckConstraint(Relation rel, HeapTuple constrtup); static void validateForeignKeyConstraint(char *conname, Relation rel, Relation pkrel, Oid pkindOid, Oid constraintOid); @@ -3588,13 +3588,13 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, address = ATExecAlterConstraint(rel, cmd, false, false, lockmode); break; case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */ - address = ATExecValidateConstraint(rel, cmd->name, false, false, - lockmode); + address = ATExecValidateConstraint(wqueue, rel, cmd->name, false, + false, lockmode); break; case AT_ValidateConstraintRecurse: /* VALIDATE CONSTRAINT with * recursion */ - address = ATExecValidateConstraint(rel, cmd->name, true, false, - lockmode); + address = ATExecValidateConstraint(wqueue, rel, cmd->name, true, + false, lockmode); break; case AT_DropConstraint: /* DROP CONSTRAINT */ ATExecDropConstraint(rel, cmd->name, cmd->behavior, @@ -6881,8 +6881,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd, * was already validated, InvalidObjectAddress is returned. */ static ObjectAddress -ATExecValidateConstraint(Relation rel, char *constrName, bool recurse, - bool recursing, LOCKMODE lockmode) +ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, + bool recurse, bool recursing, LOCKMODE lockmode) { Relation conrel; SysScanDesc scan; @@ -6929,27 +6929,31 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse, if (!con->convalidated) { + AlteredTableInfo *tab; HeapTuple copyTuple; Form_pg_constraint copy_con; if (con->contype == CONSTRAINT_FOREIGN) { - Relation refrel; + NewConstraint *newcon; + Constraint *fkconstraint; - /* - * Triggers are already in place on both tables, so a concurrent - * write that alters the result here is not possible. Normally we - * can run a query here to do the validation, which would only - * require AccessShareLock. In some cases, it is possible that we - * might need to fire triggers to perform the check, so we take a - * lock at RowShareLock level just in case. - */ - refrel = heap_open(con->confrelid, RowShareLock); + /* Queue validation for phase 3 */ + fkconstraint = makeNode(Constraint); + /* for now this is all we need */ + fkconstraint->conname = constrName; - validateForeignKeyConstraint(constrName, rel, refrel, - con->conindid, - HeapTupleGetOid(tuple)); - heap_close(refrel, NoLock); + newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); + newcon->name = constrName; + newcon->contype = CONSTR_FOREIGN; + newcon->refrelid = con->confrelid; + newcon->refindid = con->conindid; + newcon->conid = HeapTupleGetOid(tuple); + newcon->qual = (Node *) fkconstraint; + + /* Find or create work queue entry for this table */ + tab = ATGetQueueEntry(wqueue, rel); + tab->constraints = lappend(tab->constraints, newcon); /* * Foreign keys do not inherit, so we purposely ignore the @@ -6960,6 +6964,10 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse, { List *children = NIL; ListCell *child; + NewConstraint *newcon; + bool isnull; + Datum val; + char *conbin; /* * If we're recursing, the parent has already done this, so skip @@ -6998,12 +7006,31 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse, /* find_all_inheritors already got lock */ childrel = heap_open(childoid, NoLock); - ATExecValidateConstraint(childrel, constrName, false, + ATExecValidateConstraint(wqueue, childrel, constrName, false, true, lockmode); heap_close(childrel, NoLock); } - validateCheckConstraint(rel, tuple); + /* Queue validation for phase 3 */ + newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); + newcon->name = constrName; + newcon->contype = CONSTR_CHECK; + newcon->refrelid = InvalidOid; + newcon->refindid = InvalidOid; + newcon->conid = HeapTupleGetOid(tuple); + + val = SysCacheGetAttr(CONSTROID, tuple, + Anum_pg_constraint_conbin, &isnull); + if (isnull) + elog(ERROR, "null conbin for constraint %u", + HeapTupleGetOid(tuple)); + + conbin = TextDatumGetCString(val); + newcon->qual = (Node *) make_ands_implicit((Expr *) stringToNode(conbin)); + + /* Find or create work queue entry for this table */ + tab = ATGetQueueEntry(wqueue, rel); + tab->constraints = lappend(tab->constraints, newcon); /* * Invalidate relcache so that others see the new validated @@ -7375,88 +7402,6 @@ checkFkeyPermissions(Relation rel, int16 *attnums, int natts) } } -/* - * Scan the existing rows in a table to verify they meet a proposed - * CHECK constraint. - * - * The caller must have opened and locked the relation appropriately. - */ -static void -validateCheckConstraint(Relation rel, HeapTuple constrtup) -{ - EState *estate; - Datum val; - char *conbin; - Expr *origexpr; - List *exprstate; - TupleDesc tupdesc; - HeapScanDesc scan; - HeapTuple tuple; - ExprContext *econtext; - MemoryContext oldcxt; - TupleTableSlot *slot; - Form_pg_constraint constrForm; - bool isnull; - Snapshot snapshot; - - /* VALIDATE CONSTRAINT is a no-op for foreign tables */ - if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) - return; - - constrForm = (Form_pg_constraint) GETSTRUCT(constrtup); - - estate = CreateExecutorState(); - - /* - * XXX this tuple doesn't really come from a syscache, but this doesn't - * matter to SysCacheGetAttr, because it only wants to be able to fetch - * the tupdesc - */ - val = SysCacheGetAttr(CONSTROID, constrtup, Anum_pg_constraint_conbin, - &isnull); - if (isnull) - elog(ERROR, "null conbin for constraint %u", - HeapTupleGetOid(constrtup)); - conbin = TextDatumGetCString(val); - origexpr = (Expr *) stringToNode(conbin); - exprstate = (List *) - ExecPrepareExpr((Expr *) make_ands_implicit(origexpr), estate); - - econtext = GetPerTupleExprContext(estate); - tupdesc = RelationGetDescr(rel); - slot = MakeSingleTupleTableSlot(tupdesc); - econtext->ecxt_scantuple = slot; - - snapshot = RegisterSnapshot(GetLatestSnapshot()); - scan = heap_beginscan(rel, snapshot, 0, NULL); - - /* - * Switch to per-tuple memory context and reset it for each tuple - * produced, so we don't leak memory. - */ - oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate)); - - while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL) - { - ExecStoreTuple(tuple, slot, InvalidBuffer, false); - - if (!ExecQual(exprstate, econtext, true)) - ereport(ERROR, - (errcode(ERRCODE_CHECK_VIOLATION), - errmsg("check constraint \"%s\" is violated by some row", - NameStr(constrForm->conname)), - errtableconstraint(rel, NameStr(constrForm->conname)))); - - ResetExprContext(econtext); - } - - MemoryContextSwitchTo(oldcxt); - heap_endscan(scan); - UnregisterSnapshot(snapshot); - ExecDropSingleTupleTableSlot(slot); - FreeExecutorState(estate); -} - /* * Scan the existing rows in a table to verify they meet a proposed FK * constraint. diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 41db07911ed..9f611c67b72 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -436,8 +436,8 @@ NOTICE: boo: 18 ALTER TABLE tmp3 ADD CONSTRAINT IDENTITY check (b = boo(b)) NOT VALID; NOTICE: merging constraint "identity" with inherited definition ALTER TABLE tmp3 VALIDATE CONSTRAINT identity; -NOTICE: boo: 16 NOTICE: boo: 20 +NOTICE: boo: 16 -- A NO INHERIT constraint should not be looked for in children during VALIDATE CONSTRAINT create table parent_noinh_convalid (a int); create table child_noinh_convalid () inherits (parent_noinh_convalid); @@ -941,6 +941,26 @@ ERROR: column "test2" contains null values -- now add a primary key column with a default (succeeds). alter table atacc1 add column test2 int default 0 primary key; drop table atacc1; +-- additionally, we've seen issues with foreign key validation not being +-- properly delayed until after a table rewrite. Check that works ok. +create table atacc1 (a int primary key); +alter table atacc1 add constraint atacc1_fkey foreign key (a) references atacc1 (a) not valid; +alter table atacc1 validate constraint atacc1_fkey, alter a type bigint; +drop table atacc1; +-- we've also seen issues with check constraints being validated at the wrong +-- time when there's a pending table rewrite. +create table atacc1 (a bigint, b int); +insert into atacc1 values(1,1); +alter table atacc1 add constraint atacc1_chk check(b = 1) not valid; +alter table atacc1 validate constraint atacc1_chk, alter a type int; +drop table atacc1; +-- same as above, but ensure the constraint violation is detected +create table atacc1 (a bigint, b int); +insert into atacc1 values(1,2); +alter table atacc1 add constraint atacc1_chk check(b = 1) not valid; +alter table atacc1 validate constraint atacc1_chk, alter a type int; +ERROR: check constraint "atacc1_chk" is violated by some row +drop table atacc1; -- something a little more complicated create table atacc1 ( test int, test2 int); -- add a primary key constraint diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 584efd33d12..2a4eb51a3d6 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -713,6 +713,28 @@ alter table atacc1 add column test2 int primary key; alter table atacc1 add column test2 int default 0 primary key; drop table atacc1; +-- additionally, we've seen issues with foreign key validation not being +-- properly delayed until after a table rewrite. Check that works ok. +create table atacc1 (a int primary key); +alter table atacc1 add constraint atacc1_fkey foreign key (a) references atacc1 (a) not valid; +alter table atacc1 validate constraint atacc1_fkey, alter a type bigint; +drop table atacc1; + +-- we've also seen issues with check constraints being validated at the wrong +-- time when there's a pending table rewrite. +create table atacc1 (a bigint, b int); +insert into atacc1 values(1,1); +alter table atacc1 add constraint atacc1_chk check(b = 1) not valid; +alter table atacc1 validate constraint atacc1_chk, alter a type int; +drop table atacc1; + +-- same as above, but ensure the constraint violation is detected +create table atacc1 (a bigint, b int); +insert into atacc1 values(1,2); +alter table atacc1 add constraint atacc1_chk check(b = 1) not valid; +alter table atacc1 validate constraint atacc1_chk, alter a type int; +drop table atacc1; + -- something a little more complicated create table atacc1 ( test int, test2 int); -- add a primary key constraint