Re: Assert failure when validating foreign keys
Hi, On 2019-03-25 11:04:05 -0700, Andres Freund wrote: > Hi, > > On 2019-03-24 23:54:53 +1300, David Rowley wrote: > > This results in an Assert failure on master and an elog ERROR prior to > > c2fe139c201: > > > > create role test_role with login; > > create table ref(a int primary key); > > grant references on ref to test_role; > > set role test_role; > > create table t1(a int, b int); > > insert into t1 values(1,1); > > alter table t1 add constraint t1_b_key foreign key (b) references ref(a); > > server closed the connection unexpectedly > > This probably means the server terminated abnormally > > before or while processing the request. > > The connection to the server was lost. Attempting reset: Failed. > > > > Fails in heapam_tuple_satisfies_snapshot() at > > Assert(BufferIsValid(bslot->buffer)); > > > > c2fe139c201~1: > > ERROR: expected buffer tuple > > > > The test case is just a variation of the case in [1], but a different > > bug, so reporting it on a different thread. > > > > I've not looked into the cause or when it started happening. > > I think the cause is stupidity of mine. In > validateForeignKeyConstraint() I passed true to the materialize argument > of ExecFetchSlotHeapTuple(). Which therefore is made independent of > buffers. Which this assert then notices. Just changing that to false, > which is correct, fixes the issue for me. > > I'm a bit confused as to how we have no tests for this code? Is it just > that the left join codepath is "too good"? > > I've also noticed that we should free the tuple - that doesn't matter > for heap, but it sure does for other callers. But uh, is it actually ok > to validate an entire table's worth of foreign keys without a memory > context reset? I.e. shouldn't we have a memory context that we reset > after each iteration? > > Also, why's there no CHECK_FOR_INTERRUPTS()? heap has some internally on > a page level, but that doesn't seem all that granular? Tom pushed a part of this earlier in commit 46e3442c9ec858071d60a1c0fae2e9868aeaa0c8 Author: Tom Lane Date: 2019-04-06 15:09:09 -0400 Fix failures in validateForeignKeyConstraint's slow path. I've now added a fixed version of the memory context portion of this patch. Greetings, Andres Freund
Re: Assert failure when validating foreign keys
Hi,
On 2019-03-24 23:54:53 +1300, David Rowley wrote:
> This results in an Assert failure on master and an elog ERROR prior to
> c2fe139c201:
>
> create role test_role with login;
> create table ref(a int primary key);
> grant references on ref to test_role;
> set role test_role;
> create table t1(a int, b int);
> insert into t1 values(1,1);
> alter table t1 add constraint t1_b_key foreign key (b) references ref(a);
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
>
> Fails in heapam_tuple_satisfies_snapshot() at
> Assert(BufferIsValid(bslot->buffer));
>
> c2fe139c201~1:
> ERROR: expected buffer tuple
>
> The test case is just a variation of the case in [1], but a different
> bug, so reporting it on a different thread.
>
> I've not looked into the cause or when it started happening.
I think the cause is stupidity of mine. In
validateForeignKeyConstraint() I passed true to the materialize argument
of ExecFetchSlotHeapTuple(). Which therefore is made independent of
buffers. Which this assert then notices. Just changing that to false,
which is correct, fixes the issue for me.
I'm a bit confused as to how we have no tests for this code? Is it just
that the left join codepath is "too good"?
I've also noticed that we should free the tuple - that doesn't matter
for heap, but it sure does for other callers. But uh, is it actually ok
to validate an entire table's worth of foreign keys without a memory
context reset? I.e. shouldn't we have a memory context that we reset
after each iteration?
Also, why's there no CHECK_FOR_INTERRUPTS()? heap has some internally on
a page level, but that doesn't seem all that granular?
Greetings,
Andres Freund
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3183b2aaa12..6cb2b8079cf 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9017,6 +9017,8 @@ validateForeignKeyConstraint(char *conname,
TableScanDesc scan;
Trigger trig;
Snapshot snapshot;
+ MemoryContext oldcxt,
+perTupCxt;
ereport(DEBUG1,
(errmsg("validating foreign key constraint \"%s\"", conname)));
@@ -9052,11 +9054,19 @@ validateForeignKeyConstraint(char *conname,
slot = table_slot_create(rel, NULL);
scan = table_beginscan(rel, snapshot, 0, NULL);
+ perTupCxt = AllocSetContextCreate(CurrentMemoryContext,
+ "validateForeignKeyConstraint",
+ ALLOCSET_SMALL_SIZES);
+
while (table_scan_getnextslot(scan, ForwardScanDirection, slot))
{
LOCAL_FCINFO(fcinfo, 0);
TriggerData trigdata;
+ CHECK_FOR_INTERRUPTS();
+
+ oldcxt = MemoryContextSwitchTo(perTupCxt);
+
/*
* Make a call to the trigger function
*
@@ -9070,16 +9080,22 @@ validateForeignKeyConstraint(char *conname,
trigdata.type = T_TriggerData;
trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
trigdata.tg_relation = rel;
- trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, true, NULL);
+ trigdata.tg_trigtuple = ExecFetchSlotHeapTuple(slot, false, NULL);
trigdata.tg_trigslot = slot;
trigdata.tg_newtuple = NULL;
+ trigdata.tg_newslot = NULL;
trigdata.tg_trigger = &trig;
fcinfo->context = (Node *) &trigdata;
RI_FKey_check_ins(fcinfo);
+
+ MemoryContextReset(perTupCxt);
}
+ MemoryContextSwitchTo(oldcxt);
+ MemoryContextDelete(perTupCxt);
+
table_endscan(scan);
UnregisterSnapshot(snapshot);
ExecDropSingleTupleTableSlot(slot);
Re: Assert failure when validating foreign keys
Hi, On 2019-03-24 23:54:53 +1300, David Rowley wrote: > This results in an Assert failure on master and an elog ERROR prior to > c2fe139c201: > > create role test_role with login; > create table ref(a int primary key); > grant references on ref to test_role; > set role test_role; > create table t1(a int, b int); > insert into t1 values(1,1); > alter table t1 add constraint t1_b_key foreign key (b) references ref(a); > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. > > Fails in heapam_tuple_satisfies_snapshot() at > Assert(BufferIsValid(bslot->buffer)); > > c2fe139c201~1: > ERROR: expected buffer tuple > > The test case is just a variation of the case in [1], but a different > bug, so reporting it on a different thread. > > I've not looked into the cause or when it started happening. That's probably my fault somehow, I'll look into it. Got some urgent errands to run first (and it's still early here). Thanks for noticing, Andres Freund
