| Bug #61546 | Bogus calls to btr_cur_optimistic_insert() and btr_cur_optimistic_update() | ||
|---|---|---|---|
| Submitted: | 16 Jun 2011 21:20 | Modified: | 14 May 2013 12:07 |
| Reporter: | Nizameddin Ordulu | Email Updates: | |
| Status: | Closed | Impact on me: | |
| Category: | MySQL Server: InnoDB Plugin storage engine | Severity: | S4 (Feature request) |
| Version: | 5.1.52, 5.1.59, 5.6.99 | OS: | Linux |
| Assigned to: | CPU Architecture: | Any | |
| Tags: | compression, Contribution | ||
[16 Jun 2011 21:21]
Nizameddin Ordulu
The fix here only fixes the redundant calls to btr_cur_optimistic_insert(). btr_cur_pessimistic_update() needs the return code from btr_cur_optimistic_update() so it's a bit more complicated to fix this case.
[23 Jul 2011 11:37]
Sveta Smirnova
Thank you for the report.
Verified as described. Regular InnoDB affected as well. Here is excerpt for btr_cur_optimistic_update:
$vim storage/innobase/row/row0upd.c
...
} else {
1551 err = btr_cur_optimistic_update(BTR_NO_LOCKING_FLAG,
1552 btr_cur, node->update,
1553 node->cmpl_info, thr, mtr);
1554 }
1555
1556 mtr_commit(mtr);
1557
1558 if (err == DB_SUCCESS) {
1559
1560 return(err);
1561 }
1562
1563 if (buf_LRU_buf_pool_running_out()) {
1564
1565 return(DB_LOCK_TABLE_FULL);
1566 }
1567 /* We may have to modify the tree structure: do a pessimistic descent
1568 down the index tree */
1569
1570 mtr_start(mtr);
1571
1572 /* NOTE: this transaction has an s-lock or x-lock on the record and
1573 therefore other transactions cannot modify the record when we have no
1574 latch on the page. In addition, we assume that other query threads of
1575 the same transaction do not modify the record in the meantime.
1576 Therefore we can assert that the restoration of the cursor succeeds. */
1577
1578 ut_a(btr_pcur_restore_position(BTR_MODIFY_TREE, pcur, mtr));
1579
1580 ut_ad(!rec_get_deleted_flag(btr_pcur_get_rec(pcur),
1581 dict_table_is_comp(index->table)));
1582
1583 err = btr_cur_pessimistic_update(
1584 BTR_NO_LOCKING_FLAG | BTR_KEEP_POS_FLAG, btr_cur,
1585 &big_rec, node->update, node->cmpl_info, thr, mtr);
[2 Aug 2011 20:12]
Nizameddin Ordulu
Sveta: Does your excerpt include a fix? It doesn't seem to be different from the original source code. Are you planning to fix the bug for redundant btr_cur_optimistic_update() calls?
[4 Aug 2011 16:37]
Sveta Smirnova
Nizameddin, status "Verified" means behavior reported in the bug report confirmed inside the company. Now it is job of InnoDB team to fix this or explain why this would not be fixed if they decide not to fix.
[13 May 2013 20:38]
Marko Mäkelä
It turns out that the calls are not entirely bogus. There was only one call to known-to-fail btr_cur_optimistic_insert(), in btr_cur_pessimistic_update(). All other callers of btr_cur_pessimistic_insert() would release and reacquire the B-tree page latch before attempting the pessimistic insert. This would allow other threads to restructure the B-tree, allowing the insert to succeed (and requiring the insert to be attempted) as an optimistic (single-page) operation. The fixable case was fixed as part of Bug#61456.
Description: In several functions InnoDB first calls btr_cur_optimistic_insert() and when that fails it calls btr_cur_pessimistic_insert(). The latter calls the former internally, so this wastes CPU time and increases the number of compressions when the compression is enabled. Similar issue exists for btr_cur_optimistic_update() and btr_cur_pessimistic_update(). In our test servers we noticed that half of the failed compressions were caused by these redundant calls to btr_cur_optimistic_insert() and btr_cur_optimistic_update() (the number of redundant calls caused from each were approximately the same.) How to repeat: Just run an insert/update benchmark and either intercept or collect statistics about the redundant calls to btr_cur_optimistic_insert()/update(). Suggested fix: diff --git a/storage/innodb_plugin/btr/btr0btr.c b/storage/innodb_plugin/btr/btr0btr.c index 7289935..91645a1 100644 --- a/storage/innodb_plugin/btr/btr0btr.c +++ b/storage/innodb_plugin/btr/btr0btr.c @@ -1692,7 +1692,7 @@ btr_insert_on_non_leaf_level_func( | BTR_KEEP_SYS_FLAG | BTR_NO_UNDO_LOG_FLAG, &cursor, tuple, &rec, - &dummy_big_rec, 0, NULL, mtr); + &dummy_big_rec, 0, NULL, mtr, TRUE); ut_a(err == DB_SUCCESS); } diff --git a/storage/innodb_plugin/btr/btr0cur.c b/storage/innodb_plugin/btr/btr0cur.c index a54fd65..23dbcd4 100644 --- a/storage/innodb_plugin/btr/btr0cur.c +++ b/storage/innodb_plugin/btr/btr0cur.c @@ -1328,7 +1328,8 @@ btr_cur_pessimistic_insert( NULL */ ulint n_ext, /*!< in: number of externally stored columns */ que_thr_t* thr, /*!< in: query thread or NULL */ - mtr_t* mtr) /*!< in: mtr */ + mtr_t* mtr, /*!< in: mtr */ + ibool try_optimistic) /*!< in: if set, try optimistic insert first */ { dict_index_t* index = cursor->index; ulint zip_size = dict_table_zip_size(index->table); @@ -1355,11 +1356,13 @@ btr_cur_pessimistic_insert( cursor->flag = BTR_CUR_BINARY; - err = btr_cur_optimistic_insert(flags, cursor, entry, rec, - big_rec, n_ext, TRUE, thr, mtr); - if (err != DB_FAIL) { + if (try_optimistic) { + err = btr_cur_optimistic_insert(flags, cursor, entry, rec, + big_rec, n_ext, TRUE, thr, mtr); + if (err != DB_FAIL) { - return(err); + return(err); + } } /* Retry with a pessimistic insert. Check locks and write to undo log, @@ -2325,13 +2328,14 @@ make_external: was_first = page_cur_is_before_first(page_cursor); /* The first parameter means that no lock checking and undo logging - is made in the insert */ - + is made in the insert. + We don't ask btr_cur_pessimistic_insert() to try the optimistic insert + because btr_cur_insert_if_possible() already failed above */ err = btr_cur_pessimistic_insert(BTR_NO_UNDO_LOG_FLAG | BTR_NO_LOCKING_FLAG | BTR_KEEP_SYS_FLAG, cursor, new_entry, &rec, - &dummy_big_rec, n_ext, NULL, mtr); + &dummy_big_rec, n_ext, NULL, mtr, FALSE); ut_a(rec); ut_a(err == DB_SUCCESS); ut_a(dummy_big_rec == NULL); diff --git a/storage/innodb_plugin/ibuf/ibuf0ibuf.c b/storage/innodb_plugin/ibuf/ibuf0ibuf.c index 5136848..018f833 100644 --- a/storage/innodb_plugin/ibuf/ibuf0ibuf.c +++ b/storage/innodb_plugin/ibuf/ibuf0ibuf.c @@ -2752,7 +2752,7 @@ ibuf_insert_low( | BTR_NO_UNDO_LOG_FLAG, cursor, ibuf_entry, &ins_rec, - &dummy_big_rec, 0, thr, &mtr); + &dummy_big_rec, 0, thr, &mtr, FALSE); if (err == DB_SUCCESS) { /* Update the page max trx id field */ page_update_max_trx_id(btr_cur_get_block(cursor), NULL, diff --git a/storage/innodb_plugin/include/btr0cur.h b/storage/innodb_plugin/include/btr0cur.h index 58d7ae6..ef0df1b 100644 --- a/storage/innodb_plugin/include/btr0cur.h +++ b/storage/innodb_plugin/include/btr0cur.h @@ -242,7 +242,8 @@ btr_cur_pessimistic_insert( NULL */ ulint n_ext, /*!< in: number of externally stored columns */ que_thr_t* thr, /*!< in: query thread or NULL */ - mtr_t* mtr); /*!< in: mtr */ + mtr_t* mtr, /*!< in: mtr */ + ibool try_optimistic); /*!< in: try optimistic insert first, if set */ /*************************************************************//** Updates a record when the update causes no size changes in its fields. @return DB_SUCCESS or error number */ diff --git a/storage/innodb_plugin/row/row0ins.c b/storage/innodb_plugin/row/row0ins.c index 59e2c68..9401826 100644 --- a/storage/innodb_plugin/row/row0ins.c +++ b/storage/innodb_plugin/row/row0ins.c @@ -2112,7 +2112,7 @@ row_ins_index_entry_low( } err = btr_cur_pessimistic_insert( 0, &cursor, entry, &insert_rec, &big_rec, - n_ext, thr, &mtr); + n_ext, thr, &mtr, FALSE); } }