Обсуждение: Assert failure when validating foreign keys

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.

[1] https://www.postgresql.org/message-id/CAK%3D1%3DWrnNmBbe5D9sm3t0a6dnAq3cdbF1vXY816j1wsMqzC8bw%40mail.gmail.com

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

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

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

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 <tgl@sss.pgh.pa.us>
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