Skip to content

Commit

Permalink
Remove unnecessary checks before releasing resources
Browse files Browse the repository at this point in the history
Summary:
Continuation from the exploration of heap usage under `storage/rocksdb`. Essence of these changes:

  - Remove the code we don't need. Less code, less problems, better comprehension and readability.
  - Where appropriate set the released pointers to `nullptr` to make sure that all the "use after delete/free" type of bugs will manifest ASAP. Depending on OS, allocators used, what particular deallocator //really// does under the hood, the memory location may contain valid data for indeterminate amount of time and we won't be able to catch invalid usage till it's too late.

Why change this? Because there are better things in life than sleeping in your office for three days and trying to trace down that heap corruption which blocks shipping the product, causes random `SIGSEGV`-s, and happens once per week on a specific configuration when you happen to wear a certain brand of trail running shoes :-)

Test Plan:
`mysqlbuild.sh
mysqltest.sh --testset=RocksDB

mysqlbuild.sh --release
mysqltest.sh --release --testset=RocksDB`

Reviewers: yoshinorim, jtolmer

Reviewed By: jtolmer

Subscribers: jkedgar, webscalesql-eng

Differential Revision: https://reviews.facebook.net/D53439
  • Loading branch information
Gunnar Kudrjavets committed Jan 29, 2016
1 parent 6a0765d commit 1c7af80
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 46 deletions.
54 changes: 18 additions & 36 deletions storage/rocksdb/ha_rocksdb.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2705,11 +2705,8 @@ void ha_rocksdb::load_auto_incr_value()
(Why don't we use index_init/index_end? class handler defines index_init
as private, for some reason).
*/
if (scan_it)
{
delete scan_it;
scan_it= NULL;
}
delete scan_it;
scan_it= NULL;
}


Expand Down Expand Up @@ -3403,44 +3400,34 @@ int ha_rocksdb::close(void)
pk_descr= NULL;
key_descr= NULL;

if (pk_tuple)
my_free(pk_tuple);
my_free(pk_tuple);
pk_tuple= NULL;

if (pk_packed_tuple)
my_free(pk_packed_tuple);
my_free(pk_packed_tuple);
pk_packed_tuple= NULL;

if (sec_key_packed_tuple)
my_free(sec_key_packed_tuple);
my_free(sec_key_packed_tuple);
sec_key_packed_tuple= NULL;

if (sec_key_match_prefix_buf)
my_free(sec_key_match_prefix_buf);
my_free(sec_key_match_prefix_buf);
sec_key_match_prefix_buf= NULL;

if (sec_key_packed_tuple_old)
my_free(sec_key_packed_tuple_old);
my_free(sec_key_packed_tuple_old);
sec_key_packed_tuple_old= NULL;

if (end_key_packed_tuple)
my_free(end_key_packed_tuple);
my_free(end_key_packed_tuple);
end_key_packed_tuple= NULL;

if (sec_key_tails)
my_free(sec_key_tails);
my_free(sec_key_tails);
sec_key_tails= NULL;

if (sec_key_tails_old)
my_free(sec_key_tails_old);
my_free(sec_key_tails_old);
sec_key_tails_old= NULL;

if (pack_buffer)
my_free(pack_buffer);
my_free(pack_buffer);
pack_buffer= NULL;

if (field_enc)
my_free(field_enc);
my_free(field_enc);
field_enc= NULL;

DBUG_RETURN(free_share(share));
Expand Down Expand Up @@ -3636,6 +3623,7 @@ int ha_rocksdb::create_key_defs(TABLE *table_arg, const char *db_table,
error:
/* Delete what we have allocated so far */
delete tbl_def;
tbl_def = nullptr;

DBUG_RETURN(HA_ERR_INTERNAL_ERROR);
}
Expand Down Expand Up @@ -5702,11 +5690,9 @@ int ha_rocksdb::rnd_end()
{
DBUG_ENTER("ha_rocksdb::rnd_end");

if (scan_it)
{
delete scan_it;
scan_it= NULL;
}
delete scan_it;
scan_it= NULL;

DBUG_RETURN(0);
}

Expand All @@ -5726,12 +5712,8 @@ int ha_rocksdb::index_init(uint idx, bool sorted)

int ha_rocksdb::index_end()
{

if (scan_it)
{
delete scan_it;
scan_it= NULL;
}
delete scan_it;
scan_it= NULL;

active_index= MAX_KEY;
in_range_check_pushed_down= FALSE;
Expand Down
13 changes: 9 additions & 4 deletions storage/rocksdb/rdb_datadic.cc
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,11 @@ RDBSE_KEYDEF::~RDBSE_KEYDEF()
{
mysql_mutex_destroy(&mutex);

if (pk_part_no)
my_free(pk_part_no);
if (pack_info)
my_free(pack_info);
my_free(pk_part_no);
pk_part_no = nullptr;

my_free(pack_info);
pack_info = nullptr;
}

void RDBSE_KEYDEF::setup(TABLE *tbl)
Expand Down Expand Up @@ -1466,9 +1467,13 @@ RDBSE_TABLE_DEF::~RDBSE_TABLE_DEF()
if (ddl_manager && key_descr[i]) {
ddl_manager->erase_index_num(key_descr[i]->get_gl_index_id());
}

delete key_descr[i];
key_descr[i] = nullptr;
}

delete[] key_descr;
key_descr = nullptr;
}
}

Expand Down
11 changes: 5 additions & 6 deletions storage/rocksdb/rdb_locks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,9 @@
#pragma implementation // gcc: Class implementation
#endif

#include <mysql/plugin.h>

#include "ha_rocksdb.h" // TODO: this is too much

#include "rdb_locks.h"

#include <mysql/plugin.h>

typedef struct
{
Expand Down Expand Up @@ -306,8 +303,8 @@ Row_lock* LockTable::get_lock(LF_PINS *pins, const uchar* key, size_t keylen,
locked= do_locking_action(pins, found_lock, timeout_sec, is_write_lock);

err:
if (key_copy)
my_free(key_copy);
my_free(key_copy);
key_copy = nullptr;

return locked? found_lock->get_lock_handle(is_write_lock) : NULL;
}
Expand Down Expand Up @@ -469,6 +466,7 @@ bool LockTable::do_locking_action(LF_PINS *pins, Row_lock_impl *found_lock,
res= lf_hash_delete(&lf_hash, pins, found_lock->rowkey, found_lock->len);
DBUG_ASSERT(res == 0);
my_free(rowkey);
rowkey = nullptr;
}
}

Expand Down Expand Up @@ -579,6 +577,7 @@ void LockTable::release_lock(LF_PINS *pins, Row_lock *own_lock_handle)
res= lf_hash_delete(&lf_hash, pins, own_lock->rowkey, own_lock->len);
DBUG_ASSERT(res == 0);
my_free(rowkey);
rowkey = nullptr;
}
else
{
Expand Down

0 comments on commit 1c7af80

Please sign in to comment.