Horton

Thread

Commits

  1. Invalidate binary search bounds consistently.

  2. Refactor nbtree insertion scankeys.

  1. Server Crash due to assertion failure in _bt_check_unique()

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2019-04-04T07:10:55Z

    Hi All,

I'm getting a server crash in *_bt_check_unique*() when running the
following test-case.

*Steps to reproduce the crash:*
*Step1:* Create a test table with primary key and insert some data in it.

create table t1 (a integer primary key, b text);

insert into t1 values (1, 'text1');
insert into t1 values (2, 'text2');
insert into t1 values (3, 'text3');
insert into t1 values (4, 'text4');
insert into t1 values (5, 'text5');

*Step2:* Start 3 backend sessions and run the following anonymous block in
each of them in parallel:
*Session 1:*
do $$
declare
begin
 insert into t1 values (6, 'text6');
 update t1 set b = 'text66' where a=6;
 perform pg_sleep(7);
 delete from t1 where a=6;
end $$;

*Session 2:*
do $$
begin
 insert into t1 values (6, 'text6');
 perform pg_sleep('7');
 delete from t1 where a=6;
end $$;

*Session 3:*
do $$
begin
 insert into t1 values (6, 'text6');
 delete from t1 where a=6;
end $$;

Here is the backtrace for the crash:

#0  0x00007f096019f277 in raise () from /lib64/libc.so.6
#1  0x00007f09601a0968 in abort () from /lib64/libc.so.6
#2  0x0000000000a54296 in ExceptionalCondition (conditionName=0xafdbf8
"!(!_bt_isequal(itupdesc, itup_key, page, offset))",
    errorType=0xafd75a "FailedAssertion", fileName=0xafd81f "nbtinsert.c",
lineNumber=386) at assert.c:54
#3  0x0000000000509b0a in _bt_check_unique (rel=0x7f096101a030,
insertstate=0x7ffcbd5db9d0, heapRel=0x7f0961017c00,
checkUnique=UNIQUE_CHECK_YES,
    is_unique=0x7ffcbd5dba01, speculativeToken=0x7ffcbd5db9c8) at
nbtinsert.c:386
#4  0x00000000005096ab in _bt_doinsert (rel=0x7f096101a030, itup=0x27bedc0,
checkUnique=UNIQUE_CHECK_YES, heapRel=0x7f0961017c00) at nbtinsert.c:232
#5  0x0000000000514bb8 in btinsert (rel=0x7f096101a030,
values=0x7ffcbd5dbb50, isnull=0x7ffcbd5dbb30, ht_ctid=0x27be708,
heapRel=0x7f0961017c00,
    checkUnique=UNIQUE_CHECK_YES, indexInfo=0x27be048) at nbtree.c:205
#6  0x000000000050752a in index_insert (indexRelation=0x7f096101a030,
values=0x7ffcbd5dbb50, isnull=0x7ffcbd5dbb30, heap_t_ctid=0x27be708,
    heapRelation=0x7f0961017c00, checkUnique=UNIQUE_CHECK_YES,
indexInfo=0x27be048) at indexam.c:212
#7  0x00000000006e5d70 in ExecInsertIndexTuples (slot=0x27be6d8,
estate=0x27bd7a8, noDupErr=false, specConflict=0x0, arbiterIndexes=0x0)
    at execIndexing.c:390
#8  0x000000000071f11f in ExecInsert (mtstate=0x27bdb60, slot=0x27be6d8,
planSlot=0x27be6d8, estate=0x27bd7a8, canSetTag=true) at
nodeModifyTable.c:587
#9  0x0000000000721696 in ExecModifyTable (pstate=0x27bdb60) at
nodeModifyTable.c:2175
.......

The following Assert statement in *_bt_check_unique* fails

  >│386                                     *Assert(!_bt_isequal(itupdesc,
itup_key, page, offset));*


Upon quick look, it seems like the following git-commit has added above
Assert statement:

commit e5adcb789d80ba565ccacb1ed4341a7c29085238
Author: Peter Geoghegan <pg@bowt.ie>
Date:   Wed Mar 20 09:30:57 2019 -0700

    Refactor nbtree insertion scankeys.

    Use dedicated struct to represent nbtree insertion scan keys.  Having a
    dedicated struct makes the difference between search type scankeys and
    insertion scankeys a lot clearer, and simplifies the signature of
    several related functions.  This is based on a suggestion by Andrey
    Lepikhov.

....

Including Peter and Hekki in the CC as they are the main author of above
git-commit as per the commit message.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:*http://www.enterprisedb.com <http://www.enterprisedb.com/>*

  2. Re: Server Crash due to assertion failure in _bt_check_unique()

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2019-04-04T11:06:41Z

    I spent some time investigating on why this assertion failure is happening
and found the reason for it. It's basically happening because when we have
multiple transactions running in parallel and they all are trying to check
for the uniqueness of a tuple to be inserted, it's obvious that one
transaction will be waiting for other transaction to complete. But when we
come across such situation, in the current code, we are not invalidating
the search bounds in _bt_check_unique() that we saved during the previous
call to _bt_binsrch_insert(). I think, when a transaction has to wait for
some other transaction to complete, it should invalidate the binary search
bounds saved in the previous call to _bt_binsrch_insert() and do the fresh
binary search as the transaction on which it waited for might have inserted
a new tuple.

Here is the diff showing what I'm trying to say,

@@ -468,8 +468,19 @@ _bt_check_unique(Relation rel, BTInsertState
insertstate, Relation heapRel,
                                        {
                                                if (nbuf != InvalidBuffer)
                                                        _bt_relbuf(rel,
nbuf);
-                                               /* Tell _bt_doinsert to
wait... */
+                                               /*
+                                                * Tell _bt_doinsert to
wait...
+                                                *
+                                                * Also, invalidate the
search bounds saved in
+                                                * insertstate during the
previous call to
+                                                * _bt_binsrch_insert(). We
will do the fresh binary
+                                                * search as the
transaction on which we waited for
+                                                * might have inserted a
new tuple.
+                                                */
                                                *speculativeToken =
SnapshotDirty.speculativeToken;
+
+                                               insertstate->bounds_valid =
false;
+
                                                return xwait;

Attached is the patch with above changes. Please let me know if my
understanding is wrong. Thanks.

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

On Thu, Apr 4, 2019 at 12:40 PM Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

> Hi All,
>
> I'm getting a server crash in *_bt_check_unique*() when running the
> following test-case.
>
> *Steps to reproduce the crash:*
> *Step1:* Create a test table with primary key and insert some data in it.
>
> create table t1 (a integer primary key, b text);
>
> insert into t1 values (1, 'text1');
> insert into t1 values (2, 'text2');
> insert into t1 values (3, 'text3');
> insert into t1 values (4, 'text4');
> insert into t1 values (5, 'text5');
>
> *Step2:* Start 3 backend sessions and run the following anonymous block
> in each of them in parallel:
> *Session 1:*
> do $$
> declare
> begin
>  insert into t1 values (6, 'text6');
>  update t1 set b = 'text66' where a=6;
>  perform pg_sleep(7);
>  delete from t1 where a=6;
> end $$;
>
> *Session 2:*
> do $$
> begin
>  insert into t1 values (6, 'text6');
>  perform pg_sleep('7');
>  delete from t1 where a=6;
> end $$;
>
> *Session 3:*
> do $$
> begin
>  insert into t1 values (6, 'text6');
>  delete from t1 where a=6;
> end $$;
>
> Here is the backtrace for the crash:
>
> #0  0x00007f096019f277 in raise () from /lib64/libc.so.6
> #1  0x00007f09601a0968 in abort () from /lib64/libc.so.6
> #2  0x0000000000a54296 in ExceptionalCondition (conditionName=0xafdbf8
> "!(!_bt_isequal(itupdesc, itup_key, page, offset))",
>     errorType=0xafd75a "FailedAssertion", fileName=0xafd81f "nbtinsert.c",
> lineNumber=386) at assert.c:54
> #3  0x0000000000509b0a in _bt_check_unique (rel=0x7f096101a030,
> insertstate=0x7ffcbd5db9d0, heapRel=0x7f0961017c00,
> checkUnique=UNIQUE_CHECK_YES,
>     is_unique=0x7ffcbd5dba01, speculativeToken=0x7ffcbd5db9c8) at
> nbtinsert.c:386
> #4  0x00000000005096ab in _bt_doinsert (rel=0x7f096101a030,
> itup=0x27bedc0, checkUnique=UNIQUE_CHECK_YES, heapRel=0x7f0961017c00) at
> nbtinsert.c:232
> #5  0x0000000000514bb8 in btinsert (rel=0x7f096101a030,
> values=0x7ffcbd5dbb50, isnull=0x7ffcbd5dbb30, ht_ctid=0x27be708,
> heapRel=0x7f0961017c00,
>     checkUnique=UNIQUE_CHECK_YES, indexInfo=0x27be048) at nbtree.c:205
> #6  0x000000000050752a in index_insert (indexRelation=0x7f096101a030,
> values=0x7ffcbd5dbb50, isnull=0x7ffcbd5dbb30, heap_t_ctid=0x27be708,
>     heapRelation=0x7f0961017c00, checkUnique=UNIQUE_CHECK_YES,
> indexInfo=0x27be048) at indexam.c:212
> #7  0x00000000006e5d70 in ExecInsertIndexTuples (slot=0x27be6d8,
> estate=0x27bd7a8, noDupErr=false, specConflict=0x0, arbiterIndexes=0x0)
>     at execIndexing.c:390
> #8  0x000000000071f11f in ExecInsert (mtstate=0x27bdb60, slot=0x27be6d8,
> planSlot=0x27be6d8, estate=0x27bd7a8, canSetTag=true) at
> nodeModifyTable.c:587
> #9  0x0000000000721696 in ExecModifyTable (pstate=0x27bdb60) at
> nodeModifyTable.c:2175
> .......
>
> The following Assert statement in *_bt_check_unique* fails
>
>   >│386                                     *Assert(!_bt_isequal(itupdesc,
> itup_key, page, offset));*
>
>
> Upon quick look, it seems like the following git-commit has added above
> Assert statement:
>
> commit e5adcb789d80ba565ccacb1ed4341a7c29085238
> Author: Peter Geoghegan <pg@bowt.ie>
> Date:   Wed Mar 20 09:30:57 2019 -0700
>
>     Refactor nbtree insertion scankeys.
>
>     Use dedicated struct to represent nbtree insertion scan keys.  Having a
>     dedicated struct makes the difference between search type scankeys and
>     insertion scankeys a lot clearer, and simplifies the signature of
>     several related functions.  This is based on a suggestion by Andrey
>     Lepikhov.
>
> ....
>
> Including Peter and Hekki in the CC as they are the main author of above
> git-commit as per the commit message.
>
> --
> With Regards,
> Ashutosh Sharma
> EnterpriseDB:*http://www.enterprisedb.com <http://www.enterprisedb.com/>*
>

  3. Re: Server Crash due to assertion failure in _bt_check_unique()

    Peter Geoghegan <pg@bowt.ie> — 2019-04-04T16:42:23Z

    On Thu, Apr 4, 2019 at 4:06 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Attached is the patch with above changes. Please let me know if my understanding is wrong. Thanks.

You have it right. This bug slipped in towards the end of development,
when the insertstate struct was introduced.

I have pushed something very close to the patch you posted. (I added
an additional assertion, and tweaked the comments.)

Thanks for the report and patch!
-- 
Peter Geoghegan

  4. Re: Server Crash due to assertion failure in _bt_check_unique()

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2019-04-05T01:30:52Z

    On Thu, Apr 4, 2019 at 10:12 PM Peter Geoghegan <pg@bowt.ie> wrote:

> On Thu, Apr 4, 2019 at 4:06 AM Ashutosh Sharma <ashu.coek88@gmail.com>
> wrote:
> > Attached is the patch with above changes. Please let me know if my
> understanding is wrong. Thanks.
>
> You have it right. This bug slipped in towards the end of development,
> when the insertstate struct was introduced.
>
> I have pushed something very close to the patch you posted. (I added
> an additional assertion, and tweaked the comments.)
>
>
Thanks Peter :)


> Thanks for the report and patch!
> --
> Peter Geoghegan
>

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com