Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Clone in Desktop Download ZIP

Loading…

SIGSEGV on create table contention #104

Closed
yoshinorim opened this Issue · 2 comments

1 participant

@yoshinorim
Owner

santosh found a crash bug on his tests. I could verify the crash in the following conditions.
CREATE TABLE t1:
1) index_id= ddl_manager.get_next_number();
4) dict_manager.update_max_index_id(batch, index_id)

CREATE TABLE t2:
2) index_id= ddl_manager.get_next_number();
3) dict_manager.update_max_index_id(batch, index_id)

(I tested by intentionally sleeping a few seconds between 1) and 4), then executed create table t2 in the meanwhile)

MyRocks returned error on CREATE TABLE t1, because index_id was smaller than index_id stored in data dictionary (by CREATE TABLE t2). This was caused by too aggressive error check logic -- I'll change just skip to store index_id if younger than stored index_id, instead of returning an error.

What I'm still not sure is why it crashed. Looks like "ddl_manager.put_and_write()" was not properly cleaned after failure.

write_err= ddl_manager.put_and_write(tbl_def, batch)
|| dict_manager.update_max_index_id(batch, index_id) // failed here
|| dict_manager.commit(batch);

@yoshinorim
Owner

I confirmed crash didn't happen by switching order.

  • write_err= ddl_manager.put_and_write(tbl_def, batch)
  • || dict_manager.update_max_index_id(batch, index_id)
  • write_err= dict_manager.update_max_index_id(batch, index_id)
  • || ddl_manager.put_and_write(tbl_def, batch) || dict_manager.commit(batch);

We haven't implemented cleanup logic on put_and_write() failure. put_and_write() is called here only, and failure on dict_manager.commit() always results in MyRocks abort, so I think currently switching order (and not raising an error on smaller index_id described above) would be fine.

@yoshinorim yoshinorim referenced this issue from a commit
@yoshinorim yoshinorim Fix SIGSEGV on CREATE TABLE contention (issue #104)
Summary:
MyRocks did strict dictionary consistency check that
newer index_id (passed by CREATE TABLE) must be larger than
max_index_id stored in data dictionary. This is not always true. Suppose
the following conditions.
CREATE TABLE t1:
1) index_id= ddl_manager.get_next_number();
4) dict_manager.update_max_index_id(batch, index_id)

CREATE TABLE t2:
2) index_id= ddl_manager.get_next_number();
3) dict_manager.update_max_index_id(batch, index_id)

MyRocks returned an error on CREATE TABLE t1 because of dictionary
condition check (dict_manager.update_max_index_id()). Then MyRocks
crashed later by memory violation, because ddl_manager.put_and_write()
was not cleaned up after failure.

This diff fixes the issue by updating max_index_id in dictionary
when getting new index id. This diff also renaming function name from
ddl_manager.get_next_number() to ddl_manager.get_and_update_next_number().

Test Plan: mtr

Reviewers: maykov, spetrunia, jkedgar, hermanlee4

Reviewed By: hermanlee4

Differential Revision: https://reviews.facebook.net/D45963
dd8164c
@yoshinorim yoshinorim closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.