Persist the new MANIFEST after successfully syncing the new WAL during recovery #9922
Conversation
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
66929a1 to
5854406
Compare
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
0fbed05 to
3890202
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
Rebased the branch onto #9942 |
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
3890202 to
21cac8c
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
d96562a to
8102397
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
8102397 to
9b20e5e
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
Ran |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks @akankshamahajan15! Three minor comments.
We currently do not have coverage for this bug in our stress test. We can improve stress test in a separate PR.
Yes, I was going to follow up with you on stress_test coverage. |
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
517f617 to
9e00be0
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
9e00be0 to
90c989f
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
90c989f to
a7b5c67
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks @akankshamahajan15 for the PR! LGTM except a very minor comment.
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
akankshamahajan15
force-pushed
the
wal_corruption
branch
from
a7b5c67 to
a68880b
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…g recovery (facebook#9922) Summary: In case of non-TransactionDB and avoid_flush_during_recovery = true, RocksDB won't flush the data from WAL to L0 for all column families if possible. As a result, not all column families can increase their log_numbers, and min_log_number_to_keep won't change. For transaction DB (.allow_2pc), even with the flush, there may be old WAL files that it must not delete because they can contain data of uncommitted transactions and min_log_number_to_keep won't change. If we persist a new MANIFEST with advanced log_numbers for some column families, then during a second crash after persisting the MANIFEST, RocksDB will see some column families' log_numbers larger than the corrupted wal, and the "column family inconsistency" error will be hit, causing recovery to fail. As a solution, RocksDB will persist the new MANIFEST after successfully syncing the new WAL. If a future recovery starts from the new MANIFEST, then it means the new WAL is successfully synced. Due to the sentinel empty write batch at the beginning, kPointInTimeRecovery of WAL is guaranteed to go after this point. If future recovery starts from the old MANIFEST, it means the writing the new MANIFEST failed. We won't have the "SST ahead of WAL" error. Currently, RocksDB DB::Open() may creates and writes to two new MANIFEST files even before recovery succeeds. This PR buffers the edits in a structure and writes to a new MANIFEST after recovery is successful Pull Request resolved: facebook#9922 Test Plan: 1. Update unit tests to fail without this change 2. make crast_test -j Branch with unit test and no fix facebook#9942 to keep track of unit test (without fix) Reviewed By: riversand963 Differential Revision: D36043701 Pulled By: akankshamahajan15 fbshipit-source-id: 5760970db0a0920fb73d3c054a4155733500acd9
Summary: Pull Request resolved: facebook#10092 Reviewed By: riversand963 Differential Revision: D36832311 Pulled By: akankshamahajan15 fbshipit-source-id: 8fb1cf90b1d4dddebbfbeebeddb15f6905968e9b
…g recovery (#9922) Summary: In case of non-TransactionDB and avoid_flush_during_recovery = true, RocksDB won't flush the data from WAL to L0 for all column families if possible. As a result, not all column families can increase their log_numbers, and min_log_number_to_keep won't change. For transaction DB (.allow_2pc), even with the flush, there may be old WAL files that it must not delete because they can contain data of uncommitted transactions and min_log_number_to_keep won't change. If we persist a new MANIFEST with advanced log_numbers for some column families, then during a second crash after persisting the MANIFEST, RocksDB will see some column families' log_numbers larger than the corrupted wal, and the "column family inconsistency" error will be hit, causing recovery to fail. As a solution, RocksDB will persist the new MANIFEST after successfully syncing the new WAL. If a future recovery starts from the new MANIFEST, then it means the new WAL is successfully synced. Due to the sentinel empty write batch at the beginning, kPointInTimeRecovery of WAL is guaranteed to go after this point. If future recovery starts from the old MANIFEST, it means the writing the new MANIFEST failed. We won't have the "SST ahead of WAL" error. Currently, RocksDB DB::Open() may creates and writes to two new MANIFEST files even before recovery succeeds. This PR buffers the edits in a structure and writes to a new MANIFEST after recovery is successful Pull Request resolved: #9922 Test Plan: 1. Update unit tests to fail without this change 2. make crast_test -j Branch with unit test and no fix #9942 to keep track of unit test (without fix) Reviewed By: riversand963 Differential Revision: D36043701 Pulled By: akankshamahajan15 fbshipit-source-id: 5760970db0a0920fb73d3c054a4155733500acd9
Summary: In case of non-TransactionDB and avoid_flush_during_recovery = true, RocksDB won't
flush the data from WAL to L0 for all column families if possible. As a
result, not all column families can increase their log_numbers, and
min_log_number_to_keep won't change.
For transaction DB (.allow_2pc), even with the flush, there may be old WAL files that it must not delete because they can contain data of uncommitted transactions and min_log_number_to_keep won't change.
If we persist a new MANIFEST with
advanced log_numbers for some column families, then during a second
crash after persisting the MANIFEST, RocksDB will see some column
families' log_numbers larger than the corrupted wal, and the "column family inconsistency" error will be hit, causing recovery to fail.
As a solution, RocksDB will persist the new MANIFEST after successfully syncing the new WAL.
If a future recovery starts from the new MANIFEST, then it means the new WAL is successfully synced. Due to the sentinel empty write batch at the beginning, kPointInTimeRecovery of WAL is guaranteed to go after this point.
If future recovery starts from the old MANIFEST, it means the writing the new MANIFEST failed. We won't have the "SST ahead of WAL" error.
Currently, RocksDB DB::Open() may creates and writes to two new MANIFEST files even before recovery succeeds. This PR buffers the edits in a structure and writes to a new MANIFEST after recovery is successful
Test Plan: 1. Update unit tests to fail without this change
2. make crast_test -j
Branch with unit test and no fix #9942 to keep track of unit test (without fix)