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);
