WritePrepared Txn: Duplicate Keys, Memtable part #3418
Conversation
There was a problem hiding this comment.
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/dbformat.h
Outdated
| @@ -162,6 +162,8 @@ class InternalKeyComparator | |||
|
|
|||
| virtual const char* Name() const override; | |||
| virtual int Compare(const Slice& a, const Slice& b) const override; | |||
| // Same as Compare except that it excludes teh value type from comparison | |||
db/dbformat.h
Outdated
| @@ -162,6 +162,8 @@ class InternalKeyComparator | |||
|
|
|||
| virtual const char* Name() const override; | |||
| virtual int Compare(const Slice& a, const Slice& b) const override; | |||
| // Same as Compare except that it excludes teh value type from comparison | |||
There was a problem hiding this comment.
Where else is using Compare()?
There was a problem hiding this comment.
Compare() the is essential part of Comparator. I guess you mean where else we use InternalKeyComparator to which the answer would be everywhere :) It is all over the source code.
There was a problem hiding this comment.
I'm just wondering whether it is possible to make Compare() not taking types into account, although it would be very risky to do so.
db/memtable.cc
Outdated
| @@ -473,7 +473,10 @@ void MemTable::Add(SequenceNumber s, ValueType type, | |||
| Slice prefix = insert_with_hint_prefix_extractor_->Transform(key_slice); | |||
| table->InsertWithHint(handle, &insert_hints_[prefix]); | |||
| } else { | |||
| table->Insert(handle); | |||
| bool res = table->Insert(handle); | |||
There was a problem hiding this comment.
you should do the same for InsertWithHint().
db/memtable.cc
Outdated
| @@ -815,7 +820,12 @@ void MemTable::Update(SequenceNumber seq, | |||
| } | |||
|
|
|||
| // key doesn't exist | |||
| Add(seq, kTypeValue, key, value); | |||
| #ifndef NDEBUG | |||
| bool add_res = | |||
There was a problem hiding this comment.
use __attribute__((__unused__)) is cleaner than use NDEBUG macro.
db/memtable.cc
Outdated
| @@ -793,6 +797,7 @@ void MemTable::Update(SequenceNumber seq, | |||
| ValueType type; | |||
| SequenceNumber unused; | |||
| UnPackSequenceAndType(tag, &unused, &type); | |||
| assert(unused != seq); | |||
There was a problem hiding this comment.
rename unused to something else (e.g. existing_seq)?
include/rocksdb/memtablerep.h
Outdated
| @@ -81,7 +81,7 @@ class MemTableRep { | |||
| // single buffer and pass that in as the parameter to Insert). | |||
| // REQUIRES: nothing that compares equal to key is currently in the | |||
| // collection, and no concurrent modifications to the table in progress | |||
| virtual void Insert(KeyHandle handle) = 0; | |||
| virtual bool Insert(KeyHandle handle) = 0; | |||
There was a problem hiding this comment.
- Comment what's return means
- InsertWithHint and InsertConcurrent should also return duplicate key error.
- Shall we handle duplicate keys for other memtable types if that's not too difficult to do? Or alternatively, shall we add a method (e.g. MemTableRep::CanHandleDuplicatedKey() ?) to denote the behavior difference?
|
@maysamyabandeh has updated the pull request. View: changes, changes since last import |
batch_cnt = 1 count batches for CommitBatchInternal commit multiple batches in WriteBatch exclude type from memtable key compare Replace Collapse with DuplicateKeysCnt handle out-of-order duplicates rollback with duplicate keys commititmewritebatch with duplicatae keys test dup key with different cf fix compile problems rollback Update return status move the test to db_memtable_test
maysamyabandeh
force-pushed
the
duplicatekey-memtable
branch
from
7d8eeac
to
5ba0548
Compare
|
@maysamyabandeh has updated the pull request. |
|
@maysamyabandeh has updated the pull request. View: changes, changes since last import |
There was a problem hiding this comment.
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@maysamyabandeh is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Currently DB does not accept duplicate keys (keys with the same user key and the same sequence number). If Memtable returns false when receiving such keys, we can benefit from this signal to properly increase the sequence number in the rare cases when we have a duplicate key in the write batch written to DB under WritePrepared transactions.