Some fixes and enhancements to `ldb repair` #8544

pdillinger · 2021-07-16T06:06:54Z

Summary:

Basic handling of SST file with just range tombstones rather than
failing assertion about smallest_seqno <= largest_seqno
Adds --verbose option so that there exists a way to see the INFO
output from Repairer.

Test Plan: unit test added, manual testing for --verbose

Summary: * Properly handles SST file with just range tombstones rather than failing assertion about smallest_seqno <= largest_seqno * Adds --verbose option so that there exists a way to see the INFO output from Repairer. Test Plan: TODO: unit test

ajkr

IIRC the range tombstones can stick out arbitrarily past the file's MANIFEST-specified bounds. This doc (https://github.com/cockroachdb/pebble/blob/master/docs/range_deletions.md) explains why the final state we are in is necessary (although tbh I still feel we arrived there by chance).

So including range tombstones can extend a file's bounds beyond what it is supposed to cover. Meanwhile (before this PR) excluding range tombstones could make a file's bounds too small. Maybe we can just add a comment that the current solution is known to be problematic.

ajkr · 2021-07-21T17:46:31Z

I wonder if we should add table properties for smallest/largest key stored to MANIFEST, at least when they don't match the smallest/largest key in the file.

pdillinger · 2021-07-21T18:18:28Z

I see, it should ideally have the extra logic that is in compaction_job.cc but not in builder.cc (re: UpdateBoundariesForRange)?

Summary: FileOptions has an implicit conversion from EnvOptions and some internal APIs take `const FileOptions&` and save the reference, which is counter to Google C++ guidelines, > Avoid defining functions that require a const reference parameter to outlive the call, because const reference parameters bind to temporaries. Instead, find a way to eliminate the lifetime requirement (for example, by copying the parameter), or pass it by const pointer and document the lifetime and non-null requirements. This is at least a problem for repair.cc, which passes an EnvOptions to TableCache(), which would save a reference to the temporary copy as FileOptions. This was unfortunately only caught as a side effect of changes in facebook#8544. This change fixes the repair.cc case and updates the involved internal APIs that save a reference to use `const FileOptions*` instead. Unfortunately, I don't know how to get any of our sanitizers to reliably report bugs like this, so I can't rule out more existing in our codebase. Test Plan: Test that issues seen with facebook#8544 are fixed (can reproduce on AWS EC2)

ajkr · 2021-07-21T19:49:35Z

I see, it should ideally have the extra logic that is in compaction_job.cc but not in builder.cc (re: UpdateBoundariesForRange)?

Right, CompactionJob uses its knowledge about the neighboring files to decide.

pdillinger · 2021-07-21T23:23:23Z

Unfortunately, there is some other issue (alleged memory leak) being reported by ASAN with this change, even with fix #8571 applied.

Summary: This appears to be little used code so not a major bug, but is blocking facebook#8544 Test Plan: Added regression test to the end of DBRangeDelTest::TableEvictedDuringScan. Without this fix, ASAN reports memory leak.

…8589) Summary: This appears to be little used code so not a major bug, but is blocking #8544 Pull Request resolved: #8589 Test Plan: Added regression test to the end of DBRangeDelTest::TableEvictedDuringScan. Without this fix, ASAN reports memory leak. Reviewed By: ajkr Differential Revision: D29943623 Pulled By: pdillinger fbshipit-source-id: f7115fa6d4440aef83888ff609aa03d09216463b

Summary: FileOptions has an implicit conversion from EnvOptions and some internal APIs take `const FileOptions&` and save the reference, which is counter to Google C++ guidelines, > Avoid defining functions that require a const reference parameter to outlive the call, because const reference parameters bind to temporaries. Instead, find a way to eliminate the lifetime requirement (for example, by copying the parameter), or pass it by const pointer and document the lifetime and non-null requirements. This is at least a problem for repair.cc, which passes an EnvOptions to TableCache(), which would save a reference to the temporary copy as FileOptions. This was unfortunately only caught as a side effect of changes in #8544. This change fixes the repair.cc case and updates the involved internal APIs that save a reference to use `const FileOptions*` instead. Unfortunately, I don't know how to get any of our sanitizers to reliably report bugs like this, so I can't rule out more existing in our codebase. Pull Request resolved: #8571 Test Plan: Test that issues seen with #8544 are fixed (can reproduce on AWS EC2) Reviewed By: ajkr Differential Revision: D29943890 Pulled By: pdillinger fbshipit-source-id: 95f9c5251548777b4dc994c1a083dd2add5799c9

facebook-github-bot · 2021-07-28T05:15:31Z

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

pdillinger · 2021-07-28T17:19:57Z

@ajkr After more consideration (but not willing to invest a lot of time into this flawed repair tool), do you think it's better only to run this logic if counter==0, i.e. the SST file is only range tombstones?

ajkr · 2021-07-28T23:32:12Z

@ajkr After more consideration (but not willing to invest a lot of time into this flawed repair tool), do you think it's better only to run this logic if counter==0, i.e. the SST file is only range tombstones?

Hm, not particularly. A couple options that feel consistent to me are:

Always consider range tombstone boundaries that stick out past the point keys (i.e., this PR).
Never consider range tombstone boundaries sticking out past the point keys. In case of counter == 0 this would mean dropping the file.

facebook-github-bot · 2021-07-28T23:44:24Z

@pdillinger merged this pull request in 0804b44.

…acebook#8589) Summary: This appears to be little used code so not a major bug, but is blocking facebook#8544 Pull Request resolved: facebook#8589 Test Plan: Added regression test to the end of DBRangeDelTest::TableEvictedDuringScan. Without this fix, ASAN reports memory leak. Reviewed By: ajkr Differential Revision: D29943623 Pulled By: pdillinger fbshipit-source-id: f7115fa6d4440aef83888ff609aa03d09216463b

Summary: FileOptions has an implicit conversion from EnvOptions and some internal APIs take `const FileOptions&` and save the reference, which is counter to Google C++ guidelines, > Avoid defining functions that require a const reference parameter to outlive the call, because const reference parameters bind to temporaries. Instead, find a way to eliminate the lifetime requirement (for example, by copying the parameter), or pass it by const pointer and document the lifetime and non-null requirements. This is at least a problem for repair.cc, which passes an EnvOptions to TableCache(), which would save a reference to the temporary copy as FileOptions. This was unfortunately only caught as a side effect of changes in facebook#8544. This change fixes the repair.cc case and updates the involved internal APIs that save a reference to use `const FileOptions*` instead. Unfortunately, I don't know how to get any of our sanitizers to reliably report bugs like this, so I can't rule out more existing in our codebase. Pull Request resolved: facebook#8571 Test Plan: Test that issues seen with facebook#8544 are fixed (can reproduce on AWS EC2) Reviewed By: ajkr Differential Revision: D29943890 Pulled By: pdillinger fbshipit-source-id: 95f9c5251548777b4dc994c1a083dd2add5799c9

pdillinger requested a review from ajkr July 16, 2021 06:06

facebook-github-bot added the CLA Signed label Jul 16, 2021

pdillinger added 4 commits July 19, 2021 12:26

Regression test for repair with SST with only range tombstone

672f7f8

Merge branch 'master' into repair_fixes1

e82e64c

Fixes

654d072

Another 'make format'

2dbe6b2

pdillinger removed the request for review from ajkr July 21, 2021 14:59

ajkr approved these changes Jul 21, 2021

View reviewed changes

pdillinger mentioned this pull request Jul 21, 2021

Fix use-after-free on implicit temporary FileOptions #8571

Closed

pdillinger mentioned this pull request Jul 27, 2021

Fix missing Handle release in TableCache::GetRangeTombstoneIterator #8589

Closed

pdillinger added 2 commits July 27, 2021 22:00

Merge remote-tracking branch 'origin/master' into repair_fixes1

7ab41d8

Warn about naive logic / incomplete fix

a5ff0cb

facebook-github-bot closed this in 0804b44 Jul 28, 2021

facebook-github-bot added the Merged label Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some fixes and enhancements to `ldb repair` #8544

Some fixes and enhancements to `ldb repair` #8544

pdillinger commented Jul 16, 2021 •

edited

Loading

ajkr left a comment

ajkr commented Jul 21, 2021

pdillinger commented Jul 21, 2021 •

edited

Loading

ajkr commented Jul 21, 2021

pdillinger commented Jul 21, 2021

facebook-github-bot commented Jul 28, 2021

pdillinger commented Jul 28, 2021

ajkr commented Jul 28, 2021 •

edited

Loading

facebook-github-bot commented Jul 28, 2021

Some fixes and enhancements to ldb repair #8544

Some fixes and enhancements to ldb repair #8544

Conversation

pdillinger commented Jul 16, 2021 • edited Loading

ajkr left a comment

Choose a reason for hiding this comment

ajkr commented Jul 21, 2021

pdillinger commented Jul 21, 2021 • edited Loading

ajkr commented Jul 21, 2021

pdillinger commented Jul 21, 2021

facebook-github-bot commented Jul 28, 2021

pdillinger commented Jul 28, 2021

ajkr commented Jul 28, 2021 • edited Loading

facebook-github-bot commented Jul 28, 2021

Some fixes and enhancements to `ldb repair` #8544

Some fixes and enhancements to `ldb repair` #8544

pdillinger commented Jul 16, 2021 •

edited

Loading

pdillinger commented Jul 21, 2021 •

edited

Loading

ajkr commented Jul 28, 2021 •

edited

Loading