Skip to content

Conversation

@ajkr
Copy link
Contributor

@ajkr ajkr commented Sep 2, 2017

Problem:

  • DB::SanitizeOptions strips trailing slash from wal_dir but not dbname
  • We check whether wal_dir and dbname refer to the same directory using string equality: https://github.com/facebook/rocksdb/blob/master/db/repair.cc#L258
  • Providing dbname with trailing slash causes default wal_dir to be misidentified as a separate directory.
  • Then the repair tries to add all SST files to the VersionEdit twice (once for dbname dir, once for wal_dir) and fails with coredump.

Solution:

  • Add a new Env function, AreFilesSame, which uses device and inode number to check whether files are the same. It's currently only implemented in PosixEnv.
  • Migrate repair to use AreFilesSame to check whether dbname and wal_dir are same. If unsupported, falls back to string comparison.

Test Plan:

  • RepairTest.DbNameContainsTrailingSlash is regression test for the coredump
  • RepairTest.SeparateWalDir (an existing test) covers the case where wal_dir and dbname are deliberately different.
  • Also ran the two above tests with Env::AreFilesSame that returns Status::Unsupported

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ajkr ajkr force-pushed the repair-dbname-with-trailing-slash branch from 9185b89 to 289c6df Compare September 22, 2017 18:25
@facebook-github-bot
Copy link
Contributor

@ajkr has updated the pull request.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants