Enforce the contract of SingleDelete #9888
Conversation
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
riversand963
force-pushed
the
enforce-sd-contract
branch
from
e7f1dbb
to
cfa0ae6
Compare
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
db/compaction/compaction_iterator.cc
Outdated
| ROCKS_LOG_FATAL( | ||
| info_log_, | ||
| "Found SD and type:%d on the same key, violating the contract " | ||
| "of SingleDelete. Check your application to make sure the " | ||
| "application does not mix SingleDelete and Delete for " | ||
| "the same key. If you are using " | ||
| "write-prepared/write-unprepared transactions, and use " | ||
| "SingleDelete to delete certain keys, then make sure " | ||
| "TransactionDBOptions::rollback_deletion_type_callback is " | ||
| "configured properly. Mixing SD and DEL can lead to undefined " | ||
| "behaviors", | ||
| static_cast<int>(next_ikey.type)); |
There was a problem hiding this comment.
Can we return a Status::Corruption with the message instead of crashing the process?
There was a problem hiding this comment.
We can set status_ and return.
riversand963
force-pushed
the
enforce-sd-contract
branch
from
cfa0ae6
to
d1a1c3b
Compare
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
riversand963
force-pushed
the
enforce-sd-contract
branch
from
d1a1c3b
to
2c365c0
Compare
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
riversand963
force-pushed
the
enforce-sd-contract
branch
from
2c365c0
to
6aff020
Compare
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
db/compaction/compaction_iterator.cc
Outdated
| "TransactionDBOptions::rollback_deletion_type_callback is " | ||
| "configured properly. Mixing SD and DEL can lead to " | ||
| "undefined behaviors"; | ||
| ROCKS_LOG_FATAL(info_log_, "%s", oss.str().c_str()); |
There was a problem hiding this comment.
Can it be warning or error? I think this still crashes the process.
There was a problem hiding this comment.
It can be an error, but ROCKS_LOG_FATAL(...) alone does not crash the process, or maybe I am missing something.
Applied the following
diff --git a/db/version_set.cc b/db/version_set.cc
index 81d254f2b..d2d2cea98 100644
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -4441,7 +4441,7 @@ Status VersionSet::ProcessManifestWrites(
// This is fine because everything inside of this block is serialized --
// only one thread can be here at the same time
// create new manifest file
- ROCKS_LOG_INFO(db_options_->info_log, "Creating manifest %" PRIu64 "\n",
+ ROCKS_LOG_FATAL(db_options_->info_log, "Creating manifest %" PRIu64 "\n",
pending_manifest_file_number_);
std::string descriptor_fname =
DescriptorFileName(dbname_, pending_manifest_file_number_);
db_basic_test still passes except the LOGs have lines as follows
2022/04/28-09:45:17.241735 817212 [FATAL] [/version_set.cc:4444] Creating manifest 314
There was a problem hiding this comment.
OK, maybe there is a bug in ROCKS_LOG_FATAL or I misunderstand the word "fatal".
There was a problem hiding this comment.
Logger::Logv() is public so somebody might be implementing fatal properly or we might in the future.
There was a problem hiding this comment.
Make sense. It's more likely a user may override FATAL in such a way than ERROR/WARN.
db/compaction/compaction_iterator.cc
Outdated
| "TransactionDBOptions::rollback_deletion_type_callback is " | ||
| "configured properly. Mixing SD and DEL can lead to " | ||
| "undefined behaviors"; | ||
| ROCKS_LOG_FATAL(info_log_, "%s", oss.str().c_str()); |
There was a problem hiding this comment.
OK, maybe there is a bug in ROCKS_LOG_FATAL or I misunderstand the word "fatal".
Enforce the contract of SingleDelete so that they are not mixed with Delete for the same key. Otherwise, it will lead to undefined behavior. See https://github.com/facebook/rocksdb/wiki/Single-Delete#notes. Also fix unit tests and write-unprepared. Test plan make check
riversand963
force-pushed
the
enforce-sd-contract
branch
from
6aff020
to
37bc9f6
Compare
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
|
@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Thanks @ajkr for the review! |
…e contract (#9983) Summary: PR #9888 started to enforce the contract of single delete described in https://github.com/facebook/rocksdb/wiki/Single-Delete. For some of existing use cases, it is desirable to have a transition during which compaction will not fail if the contract is violated. Therefore, we add a temporary option `enforce_single_del_contracts` to allow application to opt out from this new strict behavior. Once transition completes, the flag can be set to `true` again. In a future release, the option will be removed. Pull Request resolved: #9983 Test Plan: make check Reviewed By: ajkr Differential Revision: D36333672 Pulled By: riversand963 fbshipit-source-id: dcb703ea0ed08076a1422f1bfb9914afe3c2caa2
Enforce the contract of SingleDelete so that they are not mixed with
Delete for the same key. Otherwise, it will lead to undefined behavior.
See https://github.com/facebook/rocksdb/wiki/Single-Delete#notes.
Also fix unit tests and write-unprepared.
Test plan
make check