validate range tombstone covers positive range #6788
Conversation
|
this enters read-only mode, which is a pretty harsh change. Also the validation occurs after the empty/negative range tombstone is in the WAL, so recovery will log at least a warning (depending on mode). It is also extra strict in that it prevents empty ranges, which don't really cause a problem, except that it's hard to say whether they cover a single point or nothing. I tried another approach: validating during insertion to write batch. The problem it ran into, though, is it does not always have access to a |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Need to update the PR summary. |
|
@ajkr has updated the pull request. Re-import the pull request |
|
@ajkr has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
HISTORY.md
Outdated
| @@ -16,6 +16,7 @@ | |||
| * Add NewFileChecksumGenCrc32cFactory to the file checksum public API, such that the builtin Crc32c based file checksum generator factory can be used by applications. | |||
| * Add IsDirectory to Env and FS to indicate if a path is a directory. | |||
| * Flush(..., column_family) may return Status::ColumnFamilyDropped() instead of Status::InvalidArgument() if column_family is dropped while processing the flush request. | |||
| * DeleteRange now returns InvalidArgument if the range's begin key is at or after the end key according to the user comparator. Previously the behavior was undefined. | |||
There was a problem hiding this comment.
An empty range (begin == end) was previously undefined? What's wrong with an empty range that we should reject it rather than make it a noop?
There was a problem hiding this comment.
An empty range (begin == end) was previously undefined?
I only know about problems for (begin > end). Will update this.
What's wrong with an empty range that we should reject it rather than make it a noop?
My understanding of what an interval covers comes from wikipedia: [a, b) = {x | a <= x < b}. From that perspective, the intervals where a == b and a > b are both empty. So I thought they can be treated the same.
There was a problem hiding this comment.
OK, not crazy.
But I suspect passing a > b is more diagnostic of a programming error than a == b. For example, it's common to have loops that might execute zero iterations.
There was a problem hiding this comment.
If in Math, [3, 3) is valid, allowing it is a good idea. If it doesn't, we can make it invalidate too.
There was a problem hiding this comment.
I made an update to allow it now. It seems allowed in math. Plus allowing it has the side benefit of reducing unnecessary API change.
|
@ajkr has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ajkr has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ajkr has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
We found some files containing nothing but negative range tombstones, and unsurprisingly their metadata specified a negative range, which made things crash. Time to add a bit of user input validation.
|
@ajkr has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: We found some files containing nothing but negative range tombstones, and unsurprisingly their metadata specified a negative range, which made things crash. Time to add a bit of user input validation. Pull Request resolved: facebook#6788 Reviewed By: zhichao-cao Differential Revision: D21343719 Pulled By: ajkr fbshipit-source-id: f1c16e4c3e9fa150958c8c866176632a3206fb74
We found some files containing nothing but negative range tombstones,
and unsurprisingly their metadata specified a negative range, which made
things crash. Time to add a bit of user input validation.