Fix a bug in WriteBatchInternal::Append when write batch KV protection is turned on (#10201)

Summary:
This bug was discovered after write batch checksum verification before WAL is added (https://github.com/facebook/rocksdb/issues/10114) and stress test with write batch checksum protection is turned on (https://github.com/facebook/rocksdb/issues/10037). In this [line](d5d8920f2c/db/write_batch.cc (L2887)), the number of checksums may not be consistent with `batch->Count()`. This PR fixes this issue.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/10201

Test Plan:
```
./db_stress --batch_protection_bytes_per_key=8 --destroy_db_initially=1 --max_key=100000 --use_txn=1
```

Reviewed By: ajkr

Differential Revision: D37260799

Pulled By: cbi42

fbshipit-source-id: ff8dce7dcce295d689333bc9d892d17a843bf0ea
main
Changyu Bi 3 years ago committed by Facebook GitHub Bot
parent d5d8920f2c
commit 0e0a19832e
  1. 1
      HISTORY.md
  2. 12
      db/write_batch.cc

@ -12,6 +12,7 @@
* Fixed a bug in WAL tracking with wal_compression. WAL compression writes a kSetCompressionType record which is not associated with any sequence number. As result, WalManager::GetSortedWalsOfType() will skip these WALs and not return them to caller, e.g. Checkpoint, Backup, causing the operations to fail.
* Avoid a crash if the IDENTITY file is accidentally truncated to empty. A new DB ID will be written and generated on Open.
* Fixed a possible corruption for users of `manual_wal_flush` and/or `FlushWAL(true /* sync */)`, together with `track_and_verify_wals_in_manifest == true`. For those users, losing unsynced data (e.g., due to power loss) could make future DB opens fail with a `Status::Corruption` complaining about missing WAL data.
* Fixed a bug in `WriteBatchInternal::Append()` where WAL termination point in write batch was not considered and the function appends an incorrect number of checksums.
### Public API changes
* Add new API GetUnixTime in Snapshot class which returns the unix time at which Snapshot is taken.

@ -2879,12 +2879,18 @@ Status WriteBatchInternal::Append(WriteBatch* dst, const WriteBatch* src,
src_flags = src->content_flags_.load(std::memory_order_relaxed);
}
if (dst->prot_info_ != nullptr) {
if (src->prot_info_ != nullptr) {
if (dst->prot_info_ == nullptr) {
dst->prot_info_.reset(new WriteBatch::ProtectionInfo());
}
std::copy(src->prot_info_->entries_.begin(),
src->prot_info_->entries_.begin() + src_count,
std::back_inserter(dst->prot_info_->entries_));
} else if (src->prot_info_ != nullptr) {
dst->prot_info_.reset(new WriteBatch::ProtectionInfo(*src->prot_info_));
} else if (dst->prot_info_ != nullptr) {
// dst has empty prot_info->entries
// In this special case, we allow write batch without prot_info to
// be appende to write batch with empty prot_info
dst->prot_info_ = nullptr;
}
SetCount(dst, Count(dst) + src_count);
assert(src->rep_.size() >= WriteBatchInternal::kHeader);

Loading…
Cancel
Save