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
Appearance settings

Commit c732df1

Browse filesBrowse files
david-rowleydavid-rowley
committed
Fix timing issue with ALTER TABLE's validate constraint
An ALTER TABLE to validate a foreign key in which another subcommand already caused a pending table rewrite could fail due to ALTER TABLE attempting to validate the foreign key before the actual table rewrite takes 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 bytes The failure here was due to the SPI call which validates the foreign key trying to access an index which is yet to be rebuilt. Similarly, we also incorrectly tried to validate CHECK constraints before the heap had been rewritten. The fix for both is to delay constraint validation until phase 3, after the table has been rewritten. For CHECK constraints this means a slight behavioral change. Previously ALTER TABLE VALIDATE CONSTRAINT on inheritance tables would be validated from the bottom up. This was different from the order of evaluation when a new CHECK constraint was added. The changes made here aligns the VALIDATE CONSTRAINT evaluation order for inheritance tables to be the same as ADD CONSTRAINT, which is generally top-down. Reported-by: Nazli Ugur Koyluoglu, using SQLancer Discussion: https://postgr.es/m/CAApHDvp%3DZXv8wiRyk_0rWr00skhGkt8vXDrHJYXRMft3TjkxCA%40mail.gmail.com Backpatch-through: 9.5 (all supported versions)
1 parent dc6bb09 commit c732df1
Copy full SHA for c732df1

File tree

Expand file treeCollapse file tree

3 files changed

+95
-108
lines changed
Filter options
Expand file treeCollapse file tree

3 files changed

+95
-108
lines changed

‎src/backend/commands/tablecmds.c

Copy file name to clipboardExpand all lines: src/backend/commands/tablecmds.c
+52-107Lines changed: 52 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -290,8 +290,9 @@ static void AlterSeqNamespaces(Relation classRel, Relation rel,
290290
LOCKMODE lockmode);
291291
static ObjectAddress ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
292292
bool recurse, bool recursing, LOCKMODE lockmode);
293-
static ObjectAddress ATExecValidateConstraint(Relation rel, char *constrName,
294-
bool recurse, bool recursing, LOCKMODE lockmode);
293+
static ObjectAddress ATExecValidateConstraint(List **wqueue, Relation rel,
294+
char *constrName, bool recurse, bool recursing,
295+
LOCKMODE lockmode);
295296
static int transformColumnNameList(Oid relId, List *colList,
296297
int16 *attnums, Oid *atttypids);
297298
static int transformFkeyGetPrimaryKey(Relation pkrel, Oid *indexOid,
@@ -304,7 +305,6 @@ static Oid transformFkeyCheckAttrs(Relation pkrel,
304305
static void checkFkeyPermissions(Relation rel, int16 *attnums, int natts);
305306
static CoercionPathType findFkeyCast(Oid targetTypeId, Oid sourceTypeId,
306307
Oid *funcid);
307-
static void validateCheckConstraint(Relation rel, HeapTuple constrtup);
308308
static void validateForeignKeyConstraint(char *conname,
309309
Relation rel, Relation pkrel,
310310
Oid pkindOid, Oid constraintOid);
@@ -3588,13 +3588,13 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
35883588
address = ATExecAlterConstraint(rel, cmd, false, false, lockmode);
35893589
break;
35903590
case AT_ValidateConstraint: /* VALIDATE CONSTRAINT */
3591-
address = ATExecValidateConstraint(rel, cmd->name, false, false,
3592-
lockmode);
3591+
address = ATExecValidateConstraint(wqueue, rel, cmd->name, false,
3592+
false, lockmode);
35933593
break;
35943594
case AT_ValidateConstraintRecurse: /* VALIDATE CONSTRAINT with
35953595
* recursion */
3596-
address = ATExecValidateConstraint(rel, cmd->name, true, false,
3597-
lockmode);
3596+
address = ATExecValidateConstraint(wqueue, rel, cmd->name, true,
3597+
false, lockmode);
35983598
break;
35993599
case AT_DropConstraint: /* DROP CONSTRAINT */
36003600
ATExecDropConstraint(rel, cmd->name, cmd->behavior,
@@ -6881,8 +6881,8 @@ ATExecAlterConstraint(Relation rel, AlterTableCmd *cmd,
68816881
* was already validated, InvalidObjectAddress is returned.
68826882
*/
68836883
static ObjectAddress
6884-
ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
6885-
bool recursing, LOCKMODE lockmode)
6884+
ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName,
6885+
bool recurse, bool recursing, LOCKMODE lockmode)
68866886
{
68876887
Relation conrel;
68886888
SysScanDesc scan;
@@ -6929,27 +6929,31 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
69296929

69306930
if (!con->convalidated)
69316931
{
6932+
AlteredTableInfo *tab;
69326933
HeapTuple copyTuple;
69336934
Form_pg_constraint copy_con;
69346935

69356936
if (con->contype == CONSTRAINT_FOREIGN)
69366937
{
6937-
Relation refrel;
6938+
NewConstraint *newcon;
6939+
Constraint *fkconstraint;
69386940

6939-
/*
6940-
* Triggers are already in place on both tables, so a concurrent
6941-
* write that alters the result here is not possible. Normally we
6942-
* can run a query here to do the validation, which would only
6943-
* require AccessShareLock. In some cases, it is possible that we
6944-
* might need to fire triggers to perform the check, so we take a
6945-
* lock at RowShareLock level just in case.
6946-
*/
6947-
refrel = heap_open(con->confrelid, RowShareLock);
6941+
/* Queue validation for phase 3 */
6942+
fkconstraint = makeNode(Constraint);
6943+
/* for now this is all we need */
6944+
fkconstraint->conname = constrName;
69486945

6949-
validateForeignKeyConstraint(constrName, rel, refrel,
6950-
con->conindid,
6951-
HeapTupleGetOid(tuple));
6952-
heap_close(refrel, NoLock);
6946+
newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
6947+
newcon->name = constrName;
6948+
newcon->contype = CONSTR_FOREIGN;
6949+
newcon->refrelid = con->confrelid;
6950+
newcon->refindid = con->conindid;
6951+
newcon->conid = HeapTupleGetOid(tuple);
6952+
newcon->qual = (Node *) fkconstraint;
6953+
6954+
/* Find or create work queue entry for this table */
6955+
tab = ATGetQueueEntry(wqueue, rel);
6956+
tab->constraints = lappend(tab->constraints, newcon);
69536957

69546958
/*
69556959
* Foreign keys do not inherit, so we purposely ignore the
@@ -6960,6 +6964,10 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
69606964
{
69616965
List *children = NIL;
69626966
ListCell *child;
6967+
NewConstraint *newcon;
6968+
bool isnull;
6969+
Datum val;
6970+
char *conbin;
69636971

69646972
/*
69656973
* If we're recursing, the parent has already done this, so skip
@@ -6998,12 +7006,31 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
69987006
/* find_all_inheritors already got lock */
69997007
childrel = heap_open(childoid, NoLock);
70007008

7001-
ATExecValidateConstraint(childrel, constrName, false,
7009+
ATExecValidateConstraint(wqueue, childrel, constrName, false,
70027010
true, lockmode);
70037011
heap_close(childrel, NoLock);
70047012
}
70057013

7006-
validateCheckConstraint(rel, tuple);
7014+
/* Queue validation for phase 3 */
7015+
newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
7016+
newcon->name = constrName;
7017+
newcon->contype = CONSTR_CHECK;
7018+
newcon->refrelid = InvalidOid;
7019+
newcon->refindid = InvalidOid;
7020+
newcon->conid = HeapTupleGetOid(tuple);
7021+
7022+
val = SysCacheGetAttr(CONSTROID, tuple,
7023+
Anum_pg_constraint_conbin, &isnull);
7024+
if (isnull)
7025+
elog(ERROR, "null conbin for constraint %u",
7026+
HeapTupleGetOid(tuple));
7027+
7028+
conbin = TextDatumGetCString(val);
7029+
newcon->qual = (Node *) make_ands_implicit((Expr *) stringToNode(conbin));
7030+
7031+
/* Find or create work queue entry for this table */
7032+
tab = ATGetQueueEntry(wqueue, rel);
7033+
tab->constraints = lappend(tab->constraints, newcon);
70077034

70087035
/*
70097036
* Invalidate relcache so that others see the new validated
@@ -7375,88 +7402,6 @@ checkFkeyPermissions(Relation rel, int16 *attnums, int natts)
73757402
}
73767403
}
73777404

7378-
/*
7379-
* Scan the existing rows in a table to verify they meet a proposed
7380-
* CHECK constraint.
7381-
*
7382-
* The caller must have opened and locked the relation appropriately.
7383-
*/
7384-
static void
7385-
validateCheckConstraint(Relation rel, HeapTuple constrtup)
7386-
{
7387-
EState *estate;
7388-
Datum val;
7389-
char *conbin;
7390-
Expr *origexpr;
7391-
List *exprstate;
7392-
TupleDesc tupdesc;
7393-
HeapScanDesc scan;
7394-
HeapTuple tuple;
7395-
ExprContext *econtext;
7396-
MemoryContext oldcxt;
7397-
TupleTableSlot *slot;
7398-
Form_pg_constraint constrForm;
7399-
bool isnull;
7400-
Snapshot snapshot;
7401-
7402-
/* VALIDATE CONSTRAINT is a no-op for foreign tables */
7403-
if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
7404-
return;
7405-
7406-
constrForm = (Form_pg_constraint) GETSTRUCT(constrtup);
7407-
7408-
estate = CreateExecutorState();
7409-
7410-
/*
7411-
* XXX this tuple doesn't really come from a syscache, but this doesn't
7412-
* matter to SysCacheGetAttr, because it only wants to be able to fetch
7413-
* the tupdesc
7414-
*/
7415-
val = SysCacheGetAttr(CONSTROID, constrtup, Anum_pg_constraint_conbin,
7416-
&isnull);
7417-
if (isnull)
7418-
elog(ERROR, "null conbin for constraint %u",
7419-
HeapTupleGetOid(constrtup));
7420-
conbin = TextDatumGetCString(val);
7421-
origexpr = (Expr *) stringToNode(conbin);
7422-
exprstate = (List *)
7423-
ExecPrepareExpr((Expr *) make_ands_implicit(origexpr), estate);
7424-
7425-
econtext = GetPerTupleExprContext(estate);
7426-
tupdesc = RelationGetDescr(rel);
7427-
slot = MakeSingleTupleTableSlot(tupdesc);
7428-
econtext->ecxt_scantuple = slot;
7429-
7430-
snapshot = RegisterSnapshot(GetLatestSnapshot());
7431-
scan = heap_beginscan(rel, snapshot, 0, NULL);
7432-
7433-
/*
7434-
* Switch to per-tuple memory context and reset it for each tuple
7435-
* produced, so we don't leak memory.
7436-
*/
7437-
oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
7438-
7439-
while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
7440-
{
7441-
ExecStoreTuple(tuple, slot, InvalidBuffer, false);
7442-
7443-
if (!ExecQual(exprstate, econtext, true))
7444-
ereport(ERROR,
7445-
(errcode(ERRCODE_CHECK_VIOLATION),
7446-
errmsg("check constraint \"%s\" is violated by some row",
7447-
NameStr(constrForm->conname)),
7448-
errtableconstraint(rel, NameStr(constrForm->conname))));
7449-
7450-
ResetExprContext(econtext);
7451-
}
7452-
7453-
MemoryContextSwitchTo(oldcxt);
7454-
heap_endscan(scan);
7455-
UnregisterSnapshot(snapshot);
7456-
ExecDropSingleTupleTableSlot(slot);
7457-
FreeExecutorState(estate);
7458-
}
7459-
74607405
/*
74617406
* Scan the existing rows in a table to verify they meet a proposed FK
74627407
* constraint.

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

Copy file name to clipboardExpand all lines: src/test/regress/expected/alter_table.out
+21-1Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -436,8 +436,8 @@ NOTICE: boo: 18
436436
ALTER TABLE tmp3 ADD CONSTRAINT IDENTITY check (b = boo(b)) NOT VALID;
437437
NOTICE: merging constraint "identity" with inherited definition
438438
ALTER TABLE tmp3 VALIDATE CONSTRAINT identity;
439-
NOTICE: boo: 16
440439
NOTICE: boo: 20
440+
NOTICE: boo: 16
441441
-- A NO INHERIT constraint should not be looked for in children during VALIDATE CONSTRAINT
442442
create table parent_noinh_convalid (a int);
443443
create table child_noinh_convalid () inherits (parent_noinh_convalid);
@@ -941,6 +941,26 @@ ERROR: column "test2" contains null values
941941
-- now add a primary key column with a default (succeeds).
942942
alter table atacc1 add column test2 int default 0 primary key;
943943
drop table atacc1;
944+
-- additionally, we've seen issues with foreign key validation not being
945+
-- properly delayed until after a table rewrite. Check that works ok.
946+
create table atacc1 (a int primary key);
947+
alter table atacc1 add constraint atacc1_fkey foreign key (a) references atacc1 (a) not valid;
948+
alter table atacc1 validate constraint atacc1_fkey, alter a type bigint;
949+
drop table atacc1;
950+
-- we've also seen issues with check constraints being validated at the wrong
951+
-- time when there's a pending table rewrite.
952+
create table atacc1 (a bigint, b int);
953+
insert into atacc1 values(1,1);
954+
alter table atacc1 add constraint atacc1_chk check(b = 1) not valid;
955+
alter table atacc1 validate constraint atacc1_chk, alter a type int;
956+
drop table atacc1;
957+
-- same as above, but ensure the constraint violation is detected
958+
create table atacc1 (a bigint, b int);
959+
insert into atacc1 values(1,2);
960+
alter table atacc1 add constraint atacc1_chk check(b = 1) not valid;
961+
alter table atacc1 validate constraint atacc1_chk, alter a type int;
962+
ERROR: check constraint "atacc1_chk" is violated by some row
963+
drop table atacc1;
944964
-- something a little more complicated
945965
create table atacc1 ( test int, test2 int);
946966
-- add a primary key constraint

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

Copy file name to clipboardExpand all lines: src/test/regress/sql/alter_table.sql
+22Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -713,6 +713,28 @@ alter table atacc1 add column test2 int primary key;
713713
alter table atacc1 add column test2 int default 0 primary key;
714714
drop table atacc1;
715715

716+
-- additionally, we've seen issues with foreign key validation not being
717+
-- properly delayed until after a table rewrite. Check that works ok.
718+
create table atacc1 (a int primary key);
719+
alter table atacc1 add constraint atacc1_fkey foreign key (a) references atacc1 (a) not valid;
720+
alter table atacc1 validate constraint atacc1_fkey, alter a type bigint;
721+
drop table atacc1;
722+
723+
-- we've also seen issues with check constraints being validated at the wrong
724+
-- time when there's a pending table rewrite.
725+
create table atacc1 (a bigint, b int);
726+
insert into atacc1 values(1,1);
727+
alter table atacc1 add constraint atacc1_chk check(b = 1) not valid;
728+
alter table atacc1 validate constraint atacc1_chk, alter a type int;
729+
drop table atacc1;
730+
731+
-- same as above, but ensure the constraint violation is detected
732+
create table atacc1 (a bigint, b int);
733+
insert into atacc1 values(1,2);
734+
alter table atacc1 add constraint atacc1_chk check(b = 1) not valid;
735+
alter table atacc1 validate constraint atacc1_chk, alter a type int;
736+
drop table atacc1;
737+
716738
-- something a little more complicated
717739
create table atacc1 ( test int, test2 int);
718740
-- add a primary key constraint

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.