Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug in MANIFEST group commit #4157

Closed

Conversation

riversand963
Copy link
Contributor

PR #3944 introduces group commit of VersionEdit in MANIFEST. The
implementation has a bug. When updating the log file number of each column
family, we must consider only VersionEdits that operate on the same column
family. Otherwise, a column family may accidentally set its log file number
higher than actual value, indicating that log files with smaller file number
will be ignored, thus causing some updates to be lost.

Test plan:

$make clean && make -j16 all check
$./db_stress -set_options_one_in=20 -ingest_external_file_one_in=20 -nooverwritepercent=0 -ops_per_thread=10000

PR facebook#3944 introduces group commit of `VersionEdit` in MANIFEST. The
implementation has a bug. When updating the log file number of each column
family, we must consider only `VersionEdit`s that operate on the same column
family. Otherwise, a column family may accidentally set its log file number
higher than actual value, indicating that log files with smaller file number
will be ignored, thus causing some updates to be lost.

Test plan:
```
$make clean && make -j16 all check
$./db_stress -set_options_one_in=20 -ingest_external_file_one_in=20 -nooverwritepercent=0 -ops_per_thread=10000
```
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@riversand963 riversand963 self-assigned this Jul 19, 2018
@riversand963 riversand963 requested a review from ajkr July 19, 2018 16:38
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

nice

@DorianZheng
Copy link
Contributor

DorianZheng commented Jul 20, 2018

@riversand963 Thank you fox fixing this bug, but I have a question about this PR. I found that VersionSet::LogAndApplyHelper has codes like this:

  if (edit->has_log_number_) {
    assert(edit->log_number_ >= cfd->GetLogNumber());
    assert(edit->log_number_ < next_file_number_.load());
  }

So If we ignore some log number, Isn't it asserting fail?

@riversand963
Copy link
Contributor Author

@DorianZheng I think there is some confusion about 'ignoring'. PR #4157 does not violate the assertions because newer version edit will always have larger file number than current column family.

Original code may mistakenly set the column family's log file number (cfd->GetLogNumber()) to a higher value while some of the updates to the column family reside in a lower log. The log files with smaller number will be ignored, causing some updates to be missing.

riversand963 added a commit that referenced this pull request Jul 24, 2018
Summary:
PR #3944 introduces group commit of `VersionEdit` in MANIFEST. The
implementation has a bug. When updating the log file number of each column
family, we must consider only `VersionEdit`s that operate on the same column
family. Otherwise, a column family may accidentally set its log file number
higher than actual value, indicating that log files with smaller file number
will be ignored, thus causing some updates to be lost.
Pull Request resolved: #4157

Differential Revision: D8916650

Pulled By: riversand963

fbshipit-source-id: 8f456cf688f17bf35ad87b38e30e899aa162f201
@riversand963 riversand963 deleted the fix_group_commit_manifest branch July 26, 2018 18:35
rcane pushed a commit to rcane/rocksdb that referenced this pull request Sep 13, 2018
Summary:
PR facebook#3944 introduces group commit of `VersionEdit` in MANIFEST. The
implementation has a bug. When updating the log file number of each column
family, we must consider only `VersionEdit`s that operate on the same column
family. Otherwise, a column family may accidentally set its log file number
higher than actual value, indicating that log files with smaller file number
will be ignored, thus causing some updates to be lost.
Pull Request resolved: facebook#4157

Differential Revision: D8916650

Pulled By: riversand963

fbshipit-source-id: 8f456cf688f17bf35ad87b38e30e899aa162f201
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.

4 participants