Fix wrong value passed in compaction filter in BlobDB #10391
Conversation
|
@ltamasi @riversand963 could you please help review this bug fix? Thanks |
db/compaction/compaction_iterator.cc
Outdated
| @@ -214,6 +214,7 @@ bool CompactionIterator::InvokeFilterIfNeeded(bool* need_skip, | |||
| CompactionFilter::Decision filter = CompactionFilter::Decision::kUndetermined; | |||
| compaction_filter_value_.clear(); | |||
| compaction_filter_skip_until_.Clear(); | |||
| blob_value_.Reset(); | |||
There was a problem hiding this comment.
Thanks so much for reporting and for the PR @sherriiiliu !
This issue is somewhat similar to what I'm addressing in #10490 w/r/t the DB iterator. I'm wondering if we could reset blob_value_ in e.g. CompactionIterator::Next() instead of here? It would have the same benefit as in the DB iterator, namely we would release any cache handles as soon as possible. (Also note that GC uses blob_value_ as well, so it might be non-empty for non-compaction filter related reasons.)
There was a problem hiding this comment.
Thanks @ltamasi for your suggestion. I have changed blob_value_ reset to be in the beginning of CompactionIterator::NextFromInput() while loop
|
@ltamasi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
New blobdb has a bug in compaction filter, where
blob_value_is not reset for next iterated key. This will cause blob_value_ not empty and previous value read from blob is passed into the filter function for next key, even if its value is not in blob. Fixed by reseting regardless of key type.Test Case:
Add
FilterByValueLengthtest case inDBBlobCompactionTest