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

Commit1231a0b

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 parent330410e commit1231a0b

File tree

3 files changed

+94
-105
lines changed

3 files changed

+94
-105
lines changed

‎src/backend/commands/tablecmds.c‎

Lines changed: 51 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,8 @@ static void AlterSeqNamespaces(Relation classRel, Relation rel,
325325
LOCKMODE lockmode);
326326
static ObjectAddress ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
327327
bool recurse, bool recursing, LOCKMODE lockmode);
328-
static ObjectAddress ATExecValidateConstraint(Relation rel, char *constrName,
328+
static ObjectAddress ATExecValidateConstraint(List **wqueue,
329+
Relation rel, char *constrName,
329330
bool recurse, bool recursing, LOCKMODE lockmode);
330331
static inttransformColumnNameList(Oid relId, List *colList,
331332
int16 *attnums, Oid *atttypids);
@@ -339,7 +340,6 @@ static OidtransformFkeyCheckAttrs(Relation pkrel,
339340
static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts);
340341
static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId,
341342
Oid *funcid);
342-
static void validateCheckConstraint(Relation rel, HeapTuple constrtup);
343343
static void validateForeignKeyConstraint(char *conname,
344344
Relation rel, Relation pkrel,
345345
Oid pkindOid, Oid constraintOid);
@@ -4399,13 +4399,13 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
43994399
address = ATExecAlterConstraint(rel, cmd, false, false, lockmode);
44004400
break;
44014401
case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
4402-
address = ATExecValidateConstraint(rel, cmd->name, false, false,
4403-
lockmode);
4402+
address = ATExecValidateConstraint(wqueue,rel, cmd->name, false,
4403+
false,lockmode);
44044404
break;
44054405
case AT_ValidateConstraintRecurse:/* VALIDATE CONSTRAINT with
44064406
* recursion */
4407-
address = ATExecValidateConstraint(rel, cmd->name, true, false,
4408-
lockmode);
4407+
address = ATExecValidateConstraint(wqueue,rel, cmd->name, true,
4408+
false,lockmode);
44094409
break;
44104410
case AT_DropConstraint: /* DROP CONSTRAINT */
44114411
ATExecDropConstraint(rel, cmd->name, cmd->behavior,
@@ -9314,8 +9314,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
93149314
* was already validated, InvalidObjectAddress is returned.
93159315
*/
93169316
static ObjectAddress
9317-
ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
9318-
bool recursing, LOCKMODE lockmode)
9317+
ATExecValidateConstraint(List **wqueue,Relation rel, char *constrName,
9318+
boolrecurse, boolrecursing, LOCKMODE lockmode)
93199319
{
93209320
Relationconrel;
93219321
SysScanDesc scan;
@@ -9361,27 +9361,31 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
93619361

93629362
if (!con->convalidated)
93639363
{
9364+
AlteredTableInfo *tab;
93649365
HeapTuplecopyTuple;
93659366
Form_pg_constraint copy_con;
93669367

93679368
if (con->contype == CONSTRAINT_FOREIGN)
93689369
{
9369-
Relationrefrel;
9370+
NewConstraint *newcon;
9371+
Constraint *fkconstraint;
93709372

9371-
/*
9372-
* Triggers are already in place on both tables, so a concurrent
9373-
* write that alters the result here is not possible. Normally we
9374-
* can run a query here to do the validation, which would only
9375-
* require AccessShareLock. In some cases, it is possible that we
9376-
* might need to fire triggers to perform the check, so we take a
9377-
* lock at RowShareLock level just in case.
9378-
*/
9379-
refrel = table_open(con->confrelid, RowShareLock);
9373+
/* Queue validation for phase 3 */
9374+
fkconstraint = makeNode(Constraint);
9375+
/* for now this is all we need */
9376+
fkconstraint->conname = constrName;
93809377

9381-
validateForeignKeyConstraint(constrName, rel, refrel,
9382-
con->conindid,
9383-
con->oid);
9384-
table_close(refrel, NoLock);
9378+
newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
9379+
newcon->name = constrName;
9380+
newcon->contype = CONSTR_FOREIGN;
9381+
newcon->refrelid = con->confrelid;
9382+
newcon->refindid = con->conindid;
9383+
newcon->conid = con->oid;
9384+
newcon->qual = (Node *) fkconstraint;
9385+
9386+
/* Find or create work queue entry for this table */
9387+
tab = ATGetQueueEntry(wqueue, rel);
9388+
tab->constraints = lappend(tab->constraints, newcon);
93859389

93869390
/*
93879391
* We disallow creating invalid foreign keys to or from
@@ -9392,6 +9396,11 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
93929396
{
93939397
List *children = NIL;
93949398
ListCell *child;
9399+
NewConstraint *newcon;
9400+
boolisnull;
9401+
Datumval;
9402+
char *conbin;
9403+
93959404

93969405
/*
93979406
* If we're recursing, the parent has already done this, so skip
@@ -9431,12 +9440,30 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
94319440
/* find_all_inheritors already got lock */
94329441
childrel = table_open(childoid, NoLock);
94339442

9434-
ATExecValidateConstraint(childrel, constrName, false,
9443+
ATExecValidateConstraint(wqueue,childrel, constrName, false,
94359444
true, lockmode);
94369445
table_close(childrel, NoLock);
94379446
}
94389447

9439-
validateCheckConstraint(rel, tuple);
9448+
/* Queue validation for phase 3 */
9449+
newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
9450+
newcon->name = constrName;
9451+
newcon->contype = CONSTR_CHECK;
9452+
newcon->refrelid = InvalidOid;
9453+
newcon->refindid = InvalidOid;
9454+
newcon->conid = con->oid;
9455+
9456+
val = SysCacheGetAttr(CONSTROID, tuple,
9457+
Anum_pg_constraint_conbin, &isnull);
9458+
if (isnull)
9459+
elog(ERROR, "null conbin for constraint %u", con->oid);
9460+
9461+
conbin = TextDatumGetCString(val);
9462+
newcon->qual = (Node *) stringToNode(conbin);
9463+
9464+
/* Find or create work queue entry for this table */
9465+
tab = ATGetQueueEntry(wqueue, rel);
9466+
tab->constraints = lappend(tab->constraints, newcon);
94409467

94419468
/*
94429469
* Invalidate relcache so that others see the new validated
@@ -9810,86 +9837,6 @@ checkFkeyPermissions(Relation rel, int16 *attnums, int natts)
98109837
}
98119838
}
98129839

9813-
/*
9814-
* Scan the existing rows in a table to verify they meet a proposed
9815-
* CHECK constraint.
9816-
*
9817-
* The caller must have opened and locked the relation appropriately.
9818-
*/
9819-
static void
9820-
validateCheckConstraint(Relation rel, HeapTuple constrtup)
9821-
{
9822-
EState *estate;
9823-
Datumval;
9824-
char *conbin;
9825-
Expr *origexpr;
9826-
ExprState *exprstate;
9827-
TableScanDesc scan;
9828-
ExprContext *econtext;
9829-
MemoryContext oldcxt;
9830-
TupleTableSlot *slot;
9831-
Form_pg_constraint constrForm;
9832-
boolisnull;
9833-
Snapshotsnapshot;
9834-
9835-
/*
9836-
* VALIDATE CONSTRAINT is a no-op for foreign tables and partitioned
9837-
* tables.
9838-
*/
9839-
if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
9840-
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
9841-
return;
9842-
9843-
constrForm = (Form_pg_constraint) GETSTRUCT(constrtup);
9844-
9845-
estate = CreateExecutorState();
9846-
9847-
/*
9848-
* XXX this tuple doesn't really come from a syscache, but this doesn't
9849-
* matter to SysCacheGetAttr, because it only wants to be able to fetch
9850-
* the tupdesc
9851-
*/
9852-
val = SysCacheGetAttr(CONSTROID, constrtup, Anum_pg_constraint_conbin,
9853-
&isnull);
9854-
if (isnull)
9855-
elog(ERROR, "null conbin for constraint %u",
9856-
constrForm->oid);
9857-
conbin = TextDatumGetCString(val);
9858-
origexpr = (Expr *) stringToNode(conbin);
9859-
exprstate = ExecPrepareExpr(origexpr, estate);
9860-
9861-
econtext = GetPerTupleExprContext(estate);
9862-
slot = table_slot_create(rel, NULL);
9863-
econtext->ecxt_scantuple = slot;
9864-
9865-
snapshot = RegisterSnapshot(GetLatestSnapshot());
9866-
scan = table_beginscan(rel, snapshot, 0, NULL);
9867-
9868-
/*
9869-
* Switch to per-tuple memory context and reset it for each tuple
9870-
* produced, so we don't leak memory.
9871-
*/
9872-
oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
9873-
9874-
while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
9875-
{
9876-
if (!ExecCheck(exprstate, econtext))
9877-
ereport(ERROR,
9878-
(errcode(ERRCODE_CHECK_VIOLATION),
9879-
errmsg("check constraint \"%s\" is violated by some row",
9880-
NameStr(constrForm->conname)),
9881-
errtableconstraint(rel, NameStr(constrForm->conname))));
9882-
9883-
ResetExprContext(econtext);
9884-
}
9885-
9886-
MemoryContextSwitchTo(oldcxt);
9887-
table_endscan(scan);
9888-
UnregisterSnapshot(snapshot);
9889-
ExecDropSingleTupleTableSlot(slot);
9890-
FreeExecutorState(estate);
9891-
}
9892-
98939840
/*
98949841
* Scan the existing rows in a table to verify they meet a proposed FK
98959842
* 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" 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