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

Improve consistency checks in VersionBuilder #6901

Closed
wants to merge 21 commits into from

Conversation

ltamasi
Copy link
Contributor

@ltamasi ltamasi commented Jun 1, 2020

Summary:
The patch cleans up the code and improves the consistency checks around
adding/deleting table files in VersionBuilder. Namely, it makes the checks
stricter and improves them in the following ways:

  1. A table file can now only be deleted from the LSM tree using the level it
    resides on. Earlier, there was some unnecessary wiggle room for
    trivially moved files (they could be deleted using a lower level number than
    the actual one).
  2. A table file cannot be added to the tree if it is already present in the tree
    on any level (not just the target level). The earlier code only had an assertion
    (which is a no-op in release builds) that the newly added file is not already
    present on the target level.
  3. The above consistency checks around state transitions are now mandatory,
    as opposed to the earlier CheckConsistencyForDeletes, which was a no-op
    in release mode unless force_consistency_checks was set to true. The rationale
    here is that assuming that the initial state is consistent, a valid transition leads to a
    next state that is also consistent; however, an invalid transition offers no such
    guarantee. Hence it makes sense to validate the transitions unconditionally,
    and save force_consistency_checks for the paranoid checks that re-validate
    the entire state.
  4. The new checks build on the mechanism introduced in Make it possible to look up files by number in VersionStorageInfo #6862,
    which enables us to efficiently look up the location (level and position within level)
    of files in a Version by file number. This makes the consistency checks much more
    efficient than the earlier CheckConsistencyForDeletes, which essentially
    performed a linear search.

Test Plan:
Extended the unit tests and ran:

make check
make whitebox_crash_test

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.

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

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.

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

// storing them in levels_ to avoid regression in case there are no files
// on invalid levels. The version is not consistent if in the end the files
// on invalid levels don't cancel out.
std::map<int, std::unordered_set<uint64_t>> invalid_levels_;
std::unordered_map<int, size_t> invalid_level_sizes_;
Copy link
Contributor

@ajkr ajkr Jun 3, 2020

Choose a reason for hiding this comment

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

I know this is existing code, but do you happen to know under what condition we add or delete files from an invalid level?
edit: nvm the original commit explained it well enough (replay manifest while reopening DB with decreased num_levels).

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.

looks great! the new logic makes sense to me.

auto& del_files = level_state.deleted_files;
auto del_it = del_files.find(file_number);
if (del_it != del_files.end()) {
del_files.erase(del_it);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to allow this case (a file is added after being deleted causing them to cancel out)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. This path is currently not triggered by any actual VersionEdit but I would still keep it for the sake of completeness.

@facebook-github-bot
Copy link
Contributor

@ltamasi merged this pull request in 78e291b.

facebook-github-bot pushed a commit that referenced this pull request Jun 12, 2020
…level (#6939)

Summary:
#6901 subtly changed the handling of the corner case
when a table file is deleted from a level, then re-added to the same level. (Note: this
should be extremely rare; one scenario that comes to mind is a trivial move followed by
a call to `ReFitLevel` that moves the file back to the original level.) Before that change,
a new `FileMetaData` object was created as a result of this sequence; after the change,
the original `FileMetaData` was essentially resurrected (since the deletion and the addition
simply cancel each other out with the change). This patch restores the original behavior,
which is more intuitive considering the interface, and in sync with how trivial moves are handled.
(Also note that `FileMetaData` contains some mutable data members, the values of which
might be different in the resurrected object and the freshly created one.)
The PR also fixes a bug in this area: with the original pre-6901 code, `VersionBuilder`
would add the same file twice to the same level in the scenario described above.
Pull Request resolved: #6939

Test Plan: `make check`

Reviewed By: ajkr

Differential Revision: D21905580

Pulled By: ltamasi

fbshipit-source-id: da07ae45384ecf3c6c53506d106432d88a7ec9df
ltamasi added a commit to ltamasi/rocksdb that referenced this pull request Jun 23, 2021
Summary:
`VersionSet::VerifyCompactionFileConsistency` was superseded by the LSM tree
consistency checks introduced in facebook#6901,
which are more comprehensive, more efficient, and are performed unconditionally
even in release builds.

Test Plan:
`make check`
facebook-github-bot pushed a commit that referenced this pull request Jun 23, 2021
…8449)

Summary:
`VersionSet::VerifyCompactionFileConsistency` was superseded by the LSM tree
consistency checks introduced in #6901,
which are more comprehensive, more efficient, and are performed unconditionally
even in release builds.

Pull Request resolved: #8449

Test Plan: `make check`

Reviewed By: ajkr

Differential Revision: D29337441

Pulled By: ltamasi

fbshipit-source-id: a05324f88e3400e27e6a00406c878a6276e0c9cc
tabokie pushed a commit to tabokie/rocksdb that referenced this pull request Jul 7, 2021
Summary:
The patch cleans up the code and improves the consistency checks around
adding/deleting table files in `VersionBuilder`. Namely, it makes the checks
stricter and improves them in the following ways:
1) A table file can now only be deleted from the LSM tree using the level it
resides on. Earlier, there was some unnecessary wiggle room for
trivially moved files (they could be deleted using a lower level number than
the actual one).
2) A table file cannot be added to the tree if it is already present in the tree
on any level (not just the target level). The earlier code only had an assertion
(which is a no-op in release builds) that the newly added file is not already
present on the target level.
3) The above consistency checks around state transitions are now mandatory,
as opposed to the earlier `CheckConsistencyForDeletes`, which was a no-op
in release mode unless `force_consistency_checks` was set to `true`. The rationale
here is that assuming that the initial state is consistent, a valid transition leads to a
next state that is also consistent; however, an *invalid* transition offers no such
guarantee. Hence it makes sense to validate the transitions unconditionally,
and save `force_consistency_checks` for the paranoid checks that re-validate
the entire state.
4) The new checks build on the mechanism introduced in facebook#6862,
which enables us to efficiently look up the location (level and position within level)
of files in a `Version` by file number. This makes the consistency checks much more
efficient than the earlier `CheckConsistencyForDeletes`, which essentially
performed a linear search.
Pull Request resolved: facebook#6901

Test Plan:
Extended the unit tests and ran:

`make check`
`make whitebox_crash_test`

Reviewed By: ajkr

Differential Revision: D21822714

Pulled By: ltamasi

fbshipit-source-id: e2b29c8b6da1bf0f59004acc889e4870b2d18215
Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie pushed a commit to tabokie/rocksdb that referenced this pull request Jul 7, 2021
…level (facebook#6939)

Summary:
facebook#6901 subtly changed the handling of the corner case
when a table file is deleted from a level, then re-added to the same level. (Note: this
should be extremely rare; one scenario that comes to mind is a trivial move followed by
a call to `ReFitLevel` that moves the file back to the original level.) Before that change,
a new `FileMetaData` object was created as a result of this sequence; after the change,
the original `FileMetaData` was essentially resurrected (since the deletion and the addition
simply cancel each other out with the change). This patch restores the original behavior,
which is more intuitive considering the interface, and in sync with how trivial moves are handled.
(Also note that `FileMetaData` contains some mutable data members, the values of which
might be different in the resurrected object and the freshly created one.)
The PR also fixes a bug in this area: with the original pre-6901 code, `VersionBuilder`
would add the same file twice to the same level in the scenario described above.
Pull Request resolved: facebook#6939

Test Plan: `make check`

Reviewed By: ajkr

Differential Revision: D21905580

Pulled By: ltamasi

fbshipit-source-id: da07ae45384ecf3c6c53506d106432d88a7ec9df
Signed-off-by: tabokie <xy.tao@outlook.com>
yiwu-arbug pushed a commit to tikv/rocksdb that referenced this pull request Jul 19, 2021
Summary:
The patch cleans up the code and improves the consistency checks around
adding/deleting table files in `VersionBuilder`. Namely, it makes the checks
stricter and improves them in the following ways:
1) A table file can now only be deleted from the LSM tree using the level it
resides on. Earlier, there was some unnecessary wiggle room for
trivially moved files (they could be deleted using a lower level number than
the actual one).
2) A table file cannot be added to the tree if it is already present in the tree
on any level (not just the target level). The earlier code only had an assertion
(which is a no-op in release builds) that the newly added file is not already
present on the target level.
3) The above consistency checks around state transitions are now mandatory,
as opposed to the earlier `CheckConsistencyForDeletes`, which was a no-op
in release mode unless `force_consistency_checks` was set to `true`. The rationale
here is that assuming that the initial state is consistent, a valid transition leads to a
next state that is also consistent; however, an *invalid* transition offers no such
guarantee. Hence it makes sense to validate the transitions unconditionally,
and save `force_consistency_checks` for the paranoid checks that re-validate
the entire state.
4) The new checks build on the mechanism introduced in facebook#6862,
which enables us to efficiently look up the location (level and position within level)
of files in a `Version` by file number. This makes the consistency checks much more
efficient than the earlier `CheckConsistencyForDeletes`, which essentially
performed a linear search.
Pull Request resolved: facebook#6901

Test Plan:
Extended the unit tests and ran:

`make check`
`make whitebox_crash_test`

Reviewed By: ajkr

Differential Revision: D21822714

Pulled By: ltamasi

fbshipit-source-id: e2b29c8b6da1bf0f59004acc889e4870b2d18215
Signed-off-by: tabokie <xy.tao@outlook.com>
yiwu-arbug pushed a commit to tikv/rocksdb that referenced this pull request Jul 19, 2021
…level (facebook#6939)

Summary:
facebook#6901 subtly changed the handling of the corner case
when a table file is deleted from a level, then re-added to the same level. (Note: this
should be extremely rare; one scenario that comes to mind is a trivial move followed by
a call to `ReFitLevel` that moves the file back to the original level.) Before that change,
a new `FileMetaData` object was created as a result of this sequence; after the change,
the original `FileMetaData` was essentially resurrected (since the deletion and the addition
simply cancel each other out with the change). This patch restores the original behavior,
which is more intuitive considering the interface, and in sync with how trivial moves are handled.
(Also note that `FileMetaData` contains some mutable data members, the values of which
might be different in the resurrected object and the freshly created one.)
The PR also fixes a bug in this area: with the original pre-6901 code, `VersionBuilder`
would add the same file twice to the same level in the scenario described above.
Pull Request resolved: facebook#6939

Test Plan: `make check`

Reviewed By: ajkr

Differential Revision: D21905580

Pulled By: ltamasi

fbshipit-source-id: da07ae45384ecf3c6c53506d106432d88a7ec9df
Signed-off-by: tabokie <xy.tao@outlook.com>
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.

None yet

3 participants