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

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(dot)coek88(at)gmail(dot)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(at)bowt(dot)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/>*
>