broken memory management in TokuDB 5.6.28-76.1

[Sergei Golubchik]

in ha_tokudb_admin.cc, tokudb::analyze::standard_t::analyze_key(), TokuDB does:

  DBT key, prev_key;
  ...
  error = cursor->c_get(cursor, &key, 0, _scan_direction);
  ...
  if (key.data) tokudb::memory::free(key.data);

tokudb::memory::free is server's my_free(), but the key is allocated in PerconaFT/portability/memory.cc using os_malloc(), that is plain malloc().

This is a major no-no, mixing memory allocators. In MariaDB this causes safemalloc warnings in debug builds and crashes because memory accounting doesn't add up. In MySQL 5.7, I suspect, it will break memory related performance schema tables.

A possible fix could be to use toku_set_func_malloc()/etc functions from <portability/memory.h>, but that causes crashes elsewhere. May be not all PerconaFT code is ready for that (currently toku_set_func_malloc()/etc functionality is unused).

[George Ormond Lorch III (gl-az)]

Thanks Sergei, we discovered this too in our 5.7 effort. It seems that FT allows a caller to have it return key/value data that was allocated within the FT lib but does not provide an API for the user of the library to free it.

We are considering implementing the same idea that you were looking for, passing in a set of allocator functions to the FT lib through the BDB/YDB API as part of the FT environment. Alternately, we will expose a proper API to allow a caller/recipient of internal FT memory to release it back through the FT BDB/YDB API.

[Sergei Golubchik (sergii)]

Great.

Proper API needs to support realloc too, if I rememeber correctly there's also tokudb::memory::realloc() of the key data in the same function.

On the other hand, passing a set of allocator functions (which might need to include a function for malloc_usable_size too) would allow the server to account properly for all memory that TokuDB uses. Oherwise some users might complain that performance schema doesn't show all used memory in Percona Server.

By the way, we'd really appreciate if you push that fix in your 5.6 branch, not only in 5.7. Thanks!

[George Ormond Lorch III (gl-az)]

Yes, PFS instrumentation is specifically the idea behind pushing alloc fn's down into FT even though it will be more work to ensure FT works correctly. We have an ongoing effort to get full PFS instrumentation implemented down in FT but it is tricky due to the nature of FT being an independent library shared across products and not just a part of a MySQL Storage Engine, so some cmake hackery will probably be in effect.

Any changes for this will absolutely originate in 5.6 first and then merge to 5.7 shortly after.

[George Ormond Lorch III (gl-az)]

https://tokutek.atlassian.net/browse/DB-962

[Laurynas Biveinis (laurynas-biveinis)]

George, you closed DB-962, should this one be closed too?

[George Ormond Lorch III (gl-az)]

Yup, sorry, I forgot/missed the uplink to the lp issue on this one.
Closing...

On 05/09/2016 12:24 AM, Laurynas Biveinis wrote:
> George, you closed DB-962, should this one be closed too?
>

--
George O. Lorch III
Software Engineer, Percona
US/Arizona (GMT -7)
skype: george.ormond.lorch.iii

[Laurynas Biveinis (laurynas-biveinis)]

https://github.com/percona/percona-server/pull/480, https://github.com/percona/percona-server/pull/481

[Shahriyar Rzayev (rzayev-sehriyar)]

Percona now uses JIRA for bug reports so this bug report is migrated to: https://jira.percona.com/browse/PS-966