From 6c5c1f0b2fc71ab7e90dc7798180135e7967a264 Mon Sep 17 00:00:00 2001 From: Eugene Kosov Date: Thu, 25 Apr 2019 16:29:55 +0300 Subject: [PATCH] MDEV-19231 make DB_SUCCESS equal to 0 It's a micro optimization. On most platforms CPUs has instructions to compare with 0 fast. DB_SUCCESS is the most popular outcome of functions and this patch optimized code like (err == DB_SUCCESS) BtrBulk::finish(): bogus assertion fixed fil_node_t::read_page0(): corrected usage of os_file_read() que_eval_sql(): bugus assertion removed. Apparently it checked that the field was assigned after having been zero-initialized at object creation. It turns out that the return type of os_file_read_func() was changed in mysql/mysql-server@98909cefbc37e54efc6452c7e95bccbf64ac9213 (MySQL 5.7) from ibool to dberr_t. The reviewer (if there was any) failed to point out that because of future merges, it could be a bad idea to change the return type of a function without changing the function name. This change was applied to MariaDB 10.2.2 in commit 2e814d4702d71a04388386a9f591d14a35980bfe but the MariaDB-specific code was not fully adjusted accordingly, e.g. in fil_node_open_file(). Essentially, code like !os_file_read(...) became dead code in MariaDB and later in Mariabackup 10.2, and we could be dealing with an uninitialized buffer after a failed page read. --- extra/mariabackup/backup_copy.cc | 4 ++-- extra/mariabackup/changed_page_bitmap.cc | 2 +- extra/mariabackup/fil_cur.cc | 6 +++--- extra/mariabackup/xtrabackup.cc | 12 ++++++------ storage/innobase/btr/btr0bulk.cc | 3 ++- storage/innobase/fil/fil0fil.cc | 2 +- storage/innobase/include/db0err.h | 5 +++-- storage/innobase/que/que0que.cc | 2 -- 8 files changed, 18 insertions(+), 18 deletions(-) diff --git a/extra/mariabackup/backup_copy.cc b/extra/mariabackup/backup_copy.cc index 30ec5539c2d..fa665a798ac 100644 --- a/extra/mariabackup/backup_copy.cc +++ b/extra/mariabackup/backup_copy.cc @@ -559,9 +559,9 @@ datafile_read(datafile_cur_t *cursor) return(XB_FIL_CUR_EOF); } - if (!os_file_read(IORequestRead, + if (os_file_read(IORequestRead, cursor->file, cursor->buf, cursor->buf_offset, - to_read)) { + to_read) != DB_SUCCESS) { return(XB_FIL_CUR_ERROR); } diff --git a/extra/mariabackup/changed_page_bitmap.cc b/extra/mariabackup/changed_page_bitmap.cc index f6e9812b0c5..a21c498527a 100644 --- a/extra/mariabackup/changed_page_bitmap.cc +++ b/extra/mariabackup/changed_page_bitmap.cc @@ -195,7 +195,7 @@ log_online_read_bitmap_page( ut_a(bitmap_file->offset % MODIFIED_PAGE_BLOCK_SIZE == 0); success = os_file_read(IORequestRead, bitmap_file->file, page, bitmap_file->offset, - MODIFIED_PAGE_BLOCK_SIZE); + MODIFIED_PAGE_BLOCK_SIZE) == DB_SUCCESS; if (UNIV_UNLIKELY(!success)) { diff --git a/extra/mariabackup/fil_cur.cc b/extra/mariabackup/fil_cur.cc index fd5e1bc02dd..b5e7ba6a83e 100644 --- a/extra/mariabackup/fil_cur.cc +++ b/extra/mariabackup/fil_cur.cc @@ -253,7 +253,7 @@ xb_fil_cur_open( if (!node->space->crypt_data && os_file_read(IORequestRead, node->handle, cursor->buf, 0, - page_size.physical())) { + page_size.physical()) == DB_SUCCESS) { mutex_enter(&fil_system->mutex); if (!node->space->crypt_data) { node->space->crypt_data @@ -442,8 +442,8 @@ read_retry: cursor->buf_offset = offset; cursor->buf_page_no = (ulint)(offset / page_size); - if (!os_file_read(IORequestRead, cursor->file, cursor->buf, offset, - (ulint) to_read)) { + if (os_file_read(IORequestRead, cursor->file, cursor->buf, offset, + (ulint) to_read) != DB_SUCCESS) { ret = XB_FIL_CUR_ERROR; goto func_exit; } diff --git a/extra/mariabackup/xtrabackup.cc b/extra/mariabackup/xtrabackup.cc index 4b6ba960c05..07d21029733 100644 --- a/extra/mariabackup/xtrabackup.cc +++ b/extra/mariabackup/xtrabackup.cc @@ -3302,8 +3302,8 @@ static dberr_t xb_assign_undo_space_start() page = static_cast(ut_align(buf, UNIV_PAGE_SIZE)); retry: - if (!os_file_read(IORequestRead, file, page, TRX_SYS_PAGE_NO * UNIV_PAGE_SIZE, - UNIV_PAGE_SIZE)) { + if (os_file_read(IORequestRead, file, page, TRX_SYS_PAGE_NO * UNIV_PAGE_SIZE, + UNIV_PAGE_SIZE) != DB_SUCCESS) { msg("Reading TRX_SYS page failed."); error = DB_ERROR; goto func_exit; @@ -4655,7 +4655,7 @@ xb_space_create_file( free(buf); - if (!ret) { + if (ret != DB_SUCCESS) { msg("mariabackup: could not write the first page to %s", path); os_file_close(*file); @@ -4947,7 +4947,7 @@ xtrabackup_apply_delta( << page_size_shift); success = os_file_read(IORequestRead, src_file, incremental_buffer, offset, page_size); - if (!success) { + if (success != DB_SUCCESS) { goto error; } @@ -4980,7 +4980,7 @@ xtrabackup_apply_delta( success = os_file_read(IORequestRead, src_file, incremental_buffer, offset, page_in_buffer * page_size); - if (!success) { + if (success != DB_SUCCESS) { goto error; } @@ -5029,7 +5029,7 @@ xtrabackup_apply_delta( success = os_file_write(IORequestWrite, dst_path, dst_file, buf, off, page_size); - if (!success) { + if (success != DB_SUCCESS) { goto error; } } diff --git a/storage/innobase/btr/btr0bulk.cc b/storage/innobase/btr/btr0bulk.cc index 998ff3f1e45..2655ce2a2ce 100644 --- a/storage/innobase/btr/btr0bulk.cc +++ b/storage/innobase/btr/btr0bulk.cc @@ -1050,6 +1050,7 @@ BtrBulk::finish(dberr_t err) ut_ad(!sync_check_iterate(dict_sync_check())); - ut_ad(err != DB_SUCCESS || btr_validate_index(m_index, NULL, false)); + ut_ad(err == DB_SUCCESS + || btr_validate_index(m_index, NULL, false) == DB_SUCCESS); return(err); } diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.cc index 0e6f1b05cd6..ccf1ac0ecef 100644 --- a/storage/innobase/fil/fil0fil.cc +++ b/storage/innobase/fil/fil0fil.cc @@ -539,7 +539,7 @@ bool fil_node_t::read_page0(bool first) /* Align the memory for file i/o if we might have O_DIRECT set */ byte* page = static_cast(ut_align(buf2, psize)); IORequest request(IORequest::READ); - if (!os_file_read(request, handle, page, 0, psize)) { + if (os_file_read(request, handle, page, 0, psize) != DB_SUCCESS) { ib::error() << "Unable to read first page of file " << name; ut_free(buf2); return false; diff --git a/storage/innobase/include/db0err.h b/storage/innobase/include/db0err.h index ec8e29d458c..0d7c7b28ea7 100644 --- a/storage/innobase/include/db0err.h +++ b/storage/innobase/include/db0err.h @@ -30,12 +30,13 @@ Created 5/24/1996 Heikki Tuuri /* Do not include univ.i because univ.i includes this. */ enum dberr_t { + DB_SUCCESS, + DB_SUCCESS_LOCKED_REC = 9, /*!< like DB_SUCCESS, but a new explicit record lock was created */ - DB_SUCCESS = 10, /* The following are error codes */ - DB_ERROR, + DB_ERROR = 11, DB_INTERRUPTED, DB_OUT_OF_MEMORY, DB_OUT_OF_FILE_SPACE, diff --git a/storage/innobase/que/que0que.cc b/storage/innobase/que/que0que.cc index 05964403543..57c7a6216de 100644 --- a/storage/innobase/que/que0que.cc +++ b/storage/innobase/que/que0que.cc @@ -1233,7 +1233,5 @@ que_eval_sql( mutex_exit(&dict_sys->mutex); } - ut_a(trx->error_state != 0); - DBUG_RETURN(trx->error_state); }