Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitf1fcf2d

Browse files
committed
Fix timing issue with ALTER TABLE's validate constraint
An ALTER TABLE to validate a foreign key in which another subcommandalready caused a pending table rewrite could fail due to ALTER TABLEattempting to validate the foreign key before the actual table rewritetakes place. This situation could result in an error such as:ERROR: could not read block 0 in file "base/nnnnn/nnnnn": read only 0 of 8192 bytesThe failure here was due to the SPI call which validates the foreign keytrying to access an index which is yet to be rebuilt.Similarly, we also incorrectly tried to validate CHECK constraints beforethe heap had been rewritten.The fix for both is to delay constraint validation until phase 3, afterthe table has been rewritten. For CHECK constraints this means a slightbehavioral change. Previously ALTER TABLE VALIDATE CONSTRAINT oninheritance tables would be validated from the bottom up. This wasdifferent from the order of evaluation when a new CHECK constraint wasadded. The changes made here aligns the VALIDATE CONSTRAINT evaluationorder for inheritance tables to be the same as ADD CONSTRAINT, which isgenerally top-down.Reported-by: Nazli Ugur Koyluoglu, using SQLancerDiscussion:https://postgr.es/m/CAApHDvp%3DZXv8wiRyk_0rWr00skhGkt8vXDrHJYXRMft3TjkxCA%40mail.gmail.comBackpatch-through: 9.5 (all supported versions)
1 parentb8401c3 commitf1fcf2d

File tree

3 files changed

+93
-106
lines changed

3 files changed

+93
-106
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 50 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,8 @@ static void AlterSeqNamespaces(Relation classRel, Relation rel,
328328
LOCKMODE lockmode);
329329
static ObjectAddress ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
330330
bool recurse, bool recursing, LOCKMODE lockmode);
331-
static ObjectAddress ATExecValidateConstraint(Relation rel, char *constrName,
331+
static ObjectAddress ATExecValidateConstraint(List **wqueue,
332+
Relation rel, char *constrName,
332333
bool recurse, bool recursing, LOCKMODE lockmode);
333334
static inttransformColumnNameList(Oid relId, List *colList,
334335
int16 *attnums, Oid *atttypids);
@@ -342,7 +343,6 @@ static OidtransformFkeyCheckAttrs(Relation pkrel,
342343
static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts);
343344
static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId,
344345
Oid *funcid);
345-
static void validateCheckConstraint(Relation rel, HeapTuple constrtup);
346346
static void validateForeignKeyConstraint(char *conname,
347347
Relation rel, Relation pkrel,
348348
Oid pkindOid, Oid constraintOid);
@@ -4500,13 +4500,13 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
45004500
address = ATExecAlterConstraint(rel, cmd, false, false, lockmode);
45014501
break;
45024502
case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
4503-
address = ATExecValidateConstraint(rel, cmd->name, false, false,
4504-
lockmode);
4503+
address = ATExecValidateConstraint(wqueue,rel, cmd->name, false,
4504+
false,lockmode);
45054505
break;
45064506
case AT_ValidateConstraintRecurse:/* VALIDATE CONSTRAINT with
45074507
* recursion */
4508-
address = ATExecValidateConstraint(rel, cmd->name, true, false,
4509-
lockmode);
4508+
address = ATExecValidateConstraint(wqueue,rel, cmd->name, true,
4509+
false,lockmode);
45104510
break;
45114511
case AT_DropConstraint: /* DROP CONSTRAINT */
45124512
ATExecDropConstraint(rel, cmd->name, cmd->behavior,
@@ -9727,8 +9727,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
97279727
* was already validated, InvalidObjectAddress is returned.
97289728
*/
97299729
static ObjectAddress
9730-
ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
9731-
bool recursing, LOCKMODE lockmode)
9730+
ATExecValidateConstraint(List **wqueue,Relation rel, char *constrName,
9731+
boolrecurse, boolrecursing, LOCKMODE lockmode)
97329732
{
97339733
Relationconrel;
97349734
SysScanDesc scan;
@@ -9774,27 +9774,31 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
97749774

97759775
if (!con->convalidated)
97769776
{
9777+
AlteredTableInfo *tab;
97779778
HeapTuplecopyTuple;
97789779
Form_pg_constraint copy_con;
97799780

97809781
if (con->contype == CONSTRAINT_FOREIGN)
97819782
{
9782-
Relationrefrel;
9783+
NewConstraint *newcon;
9784+
Constraint *fkconstraint;
97839785

9784-
/*
9785-
* Triggers are already in place on both tables, so a concurrent
9786-
* write that alters the result here is not possible. Normally we
9787-
* can run a query here to do the validation, which would only
9788-
* require AccessShareLock. In some cases, it is possible that we
9789-
* might need to fire triggers to perform the check, so we take a
9790-
* lock at RowShareLock level just in case.
9791-
*/
9792-
refrel = table_open(con->confrelid, RowShareLock);
9786+
/* Queue validation for phase 3 */
9787+
fkconstraint = makeNode(Constraint);
9788+
/* for now this is all we need */
9789+
fkconstraint->conname = constrName;
97939790

9794-
validateForeignKeyConstraint(constrName, rel, refrel,
9795-
con->conindid,
9796-
con->oid);
9797-
table_close(refrel, NoLock);
9791+
newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
9792+
newcon->name = constrName;
9793+
newcon->contype = CONSTR_FOREIGN;
9794+
newcon->refrelid = con->confrelid;
9795+
newcon->refindid = con->conindid;
9796+
newcon->conid = con->oid;
9797+
newcon->qual = (Node *) fkconstraint;
9798+
9799+
/* Find or create work queue entry for this table */
9800+
tab = ATGetQueueEntry(wqueue, rel);
9801+
tab->constraints = lappend(tab->constraints, newcon);
97989802

97999803
/*
98009804
* We disallow creating invalid foreign keys to or from
@@ -9805,6 +9809,10 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
98059809
{
98069810
List *children = NIL;
98079811
ListCell *child;
9812+
NewConstraint *newcon;
9813+
boolisnull;
9814+
Datumval;
9815+
char *conbin;
98089816

98099817
/*
98109818
* If we're recursing, the parent has already done this, so skip
@@ -9844,12 +9852,30 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
98449852
/* find_all_inheritors already got lock */
98459853
childrel = table_open(childoid, NoLock);
98469854

9847-
ATExecValidateConstraint(childrel, constrName, false,
9855+
ATExecValidateConstraint(wqueue,childrel, constrName, false,
98489856
true, lockmode);
98499857
table_close(childrel, NoLock);
98509858
}
98519859

9852-
validateCheckConstraint(rel, tuple);
9860+
/* Queue validation for phase 3 */
9861+
newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
9862+
newcon->name = constrName;
9863+
newcon->contype = CONSTR_CHECK;
9864+
newcon->refrelid = InvalidOid;
9865+
newcon->refindid = InvalidOid;
9866+
newcon->conid = con->oid;
9867+
9868+
val = SysCacheGetAttr(CONSTROID, tuple,
9869+
Anum_pg_constraint_conbin, &isnull);
9870+
if (isnull)
9871+
elog(ERROR, "null conbin for constraint %u", con->oid);
9872+
9873+
conbin = TextDatumGetCString(val);
9874+
newcon->qual = (Node *) stringToNode(conbin);
9875+
9876+
/* Find or create work queue entry for this table */
9877+
tab = ATGetQueueEntry(wqueue, rel);
9878+
tab->constraints = lappend(tab->constraints, newcon);
98539879

98549880
/*
98559881
* Invalidate relcache so that others see the new validated
@@ -10223,87 +10249,6 @@ checkFkeyPermissions(Relation rel, int16 *attnums, int natts)
1022310249
}
1022410250
}
1022510251

10226-
/*
10227-
* Scan the existing rows in a table to verify they meet a proposed
10228-
* CHECK constraint.
10229-
*
10230-
* The caller must have opened and locked the relation appropriately.
10231-
*/
10232-
static void
10233-
validateCheckConstraint(Relation rel, HeapTuple constrtup)
10234-
{
10235-
EState *estate;
10236-
Datumval;
10237-
char *conbin;
10238-
Expr *origexpr;
10239-
ExprState *exprstate;
10240-
TableScanDesc scan;
10241-
ExprContext *econtext;
10242-
MemoryContext oldcxt;
10243-
TupleTableSlot *slot;
10244-
Form_pg_constraint constrForm;
10245-
boolisnull;
10246-
Snapshotsnapshot;
10247-
10248-
/*
10249-
* VALIDATE CONSTRAINT is a no-op for foreign tables and partitioned
10250-
* tables.
10251-
*/
10252-
if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
10253-
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
10254-
return;
10255-
10256-
constrForm = (Form_pg_constraint) GETSTRUCT(constrtup);
10257-
10258-
estate = CreateExecutorState();
10259-
10260-
/*
10261-
* XXX this tuple doesn't really come from a syscache, but this doesn't
10262-
* matter to SysCacheGetAttr, because it only wants to be able to fetch
10263-
* the tupdesc
10264-
*/
10265-
val = SysCacheGetAttr(CONSTROID, constrtup, Anum_pg_constraint_conbin,
10266-
&isnull);
10267-
if (isnull)
10268-
elog(ERROR, "null conbin for constraint %u",
10269-
constrForm->oid);
10270-
conbin = TextDatumGetCString(val);
10271-
origexpr = (Expr *) stringToNode(conbin);
10272-
exprstate = ExecPrepareExpr(origexpr, estate);
10273-
10274-
econtext = GetPerTupleExprContext(estate);
10275-
slot = table_slot_create(rel, NULL);
10276-
econtext->ecxt_scantuple = slot;
10277-
10278-
snapshot = RegisterSnapshot(GetLatestSnapshot());
10279-
scan = table_beginscan(rel, snapshot, 0, NULL);
10280-
10281-
/*
10282-
* Switch to per-tuple memory context and reset it for each tuple
10283-
* produced, so we don't leak memory.
10284-
*/
10285-
oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
10286-
10287-
while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
10288-
{
10289-
if (!ExecCheck(exprstate, econtext))
10290-
ereport(ERROR,
10291-
(errcode(ERRCODE_CHECK_VIOLATION),
10292-
errmsg("check constraint \"%s\" of relation \"%s\" is violated by some row",
10293-
NameStr(constrForm->conname),
10294-
RelationGetRelationName(rel)),
10295-
errtableconstraint(rel, NameStr(constrForm->conname))));
10296-
10297-
ResetExprContext(econtext);
10298-
}
10299-
10300-
MemoryContextSwitchTo(oldcxt);
10301-
table_endscan(scan);
10302-
UnregisterSnapshot(snapshot);
10303-
ExecDropSingleTupleTableSlot(slot);
10304-
FreeExecutorState(estate);
10305-
}
10306-
1030710252
/*
1030810253
* Scan the existing rows in a table to verify they meet a proposed FK
1030910254
* constraint.

‎src/test/regress/expected/alter_table.out

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,8 +487,8 @@ NOTICE: boo: 18
487487
ALTER TABLE attmp3 ADD CONSTRAINT IDENTITY check (b = boo(b)) NOT VALID;
488488
NOTICE: merging constraint "identity" with inherited definition
489489
ALTER TABLE attmp3 VALIDATE CONSTRAINT identity;
490-
NOTICE: boo: 16
491490
NOTICE: boo: 20
491+
NOTICE: boo: 16
492492
-- A NO INHERIT constraint should not be looked for in children during VALIDATE CONSTRAINT
493493
create table parent_noinh_convalid (a int);
494494
create table child_noinh_convalid () inherits (parent_noinh_convalid);
@@ -997,6 +997,26 @@ alter table atacc1
997997
add column b float8 not null default random(),
998998
add primary key(a);
999999
drop table atacc1;
1000+
-- additionally, we've seen issues with foreign key validation not being
1001+
-- properly delayed until after a table rewrite. Check that works ok.
1002+
create table atacc1 (a int primary key);
1003+
alter table atacc1 add constraint atacc1_fkey foreign key (a) references atacc1 (a) not valid;
1004+
alter table atacc1 validate constraint atacc1_fkey, alter a type bigint;
1005+
drop table atacc1;
1006+
-- we've also seen issues with check constraints being validated at the wrong
1007+
-- time when there's a pending table rewrite.
1008+
create table atacc1 (a bigint, b int);
1009+
insert into atacc1 values(1,1);
1010+
alter table atacc1 add constraint atacc1_chk check(b = 1) not valid;
1011+
alter table atacc1 validate constraint atacc1_chk, alter a type int;
1012+
drop table atacc1;
1013+
-- same as above, but ensure the constraint violation is detected
1014+
create table atacc1 (a bigint, b int);
1015+
insert into atacc1 values(1,2);
1016+
alter table atacc1 add constraint atacc1_chk check(b = 1) not valid;
1017+
alter table atacc1 validate constraint atacc1_chk, alter a type int;
1018+
ERROR: check constraint "atacc1_chk" of relation "atacc1" is violated by some row
1019+
drop table atacc1;
10001020
-- something a little more complicated
10011021
create table atacc1 ( test int, test2 int);
10021022
-- add a primary key constraint

‎src/test/regress/sql/alter_table.sql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,28 @@ alter table atacc1
757757
addprimary key(a);
758758
droptable atacc1;
759759

760+
-- additionally, we've seen issues with foreign key validation not being
761+
-- properly delayed until after a table rewrite. Check that works ok.
762+
createtableatacc1 (aintprimary key);
763+
altertable atacc1 addconstraint atacc1_fkeyforeign key (a)references atacc1 (a) not valid;
764+
altertable atacc1 validateconstraint atacc1_fkey, alter a typebigint;
765+
droptable atacc1;
766+
767+
-- we've also seen issues with check constraints being validated at the wrong
768+
-- time when there's a pending table rewrite.
769+
createtableatacc1 (abigint, bint);
770+
insert into atacc1values(1,1);
771+
altertable atacc1 addconstraint atacc1_chkcheck(b=1) not valid;
772+
altertable atacc1 validateconstraint atacc1_chk, alter a typeint;
773+
droptable atacc1;
774+
775+
-- same as above, but ensure the constraint violation is detected
776+
createtableatacc1 (abigint, bint);
777+
insert into atacc1values(1,2);
778+
altertable atacc1 addconstraint atacc1_chkcheck(b=1) not valid;
779+
altertable atacc1 validateconstraint atacc1_chk, alter a typeint;
780+
droptable atacc1;
781+
760782
-- something a little more complicated
761783
createtableatacc1 ( testint, test2int);
762784
-- add a primary key constraint

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp