feat(storage): store old value in memtable #15301
Conversation
| kv_pairs.push((key, StorageValue::new_put(value))); | ||
| kv_pairs.push((key, SharedBufferValue::Insert(value))); | ||
| if let Some(old_values) = &mut old_values { | ||
| old_values.push(Bytes::new()); |
There was a problem hiding this comment.
Will we have an entry in the old value SST for KeyOp::Insert?
There was a problem hiding this comment.
No. In SharedBufferBatchIterator, when IS_NEW_VALUE is false, at the end of the next call, we will have an extra advance_until_valid_old_value to skip the KeyOp::Insert
wenym1
force-pushed
the
yiming/old-value-memtable
branch
from
ad0e146
to
43c84c3
Compare
| } | ||
| } | ||
| } | ||
| table_change_log |
There was a problem hiding this comment.
I think it is guaranteed that entries in table_change_log should have both non-empty new_value and non-empty old_value. Can we add an assertion to check that?
There was a problem hiding this comment.
Just thought of a corner case, will we generate a old_value sst if there are only INSERTs in new_value sst? Correct me if I am wrong, I think we will generate a old_value sst with no entries in it.
There was a problem hiding this comment.
By design, if there is no data, we will still have an empty EpochNewChangeLog to store the epochs information so that when we replay we won't miss a barrier.
I just checked the code. Currently the sst builder is initialized lazily. So if the payload is append only, the old value imm iterator won't have any output, and then no sst will be built.
| HummockValue::Put(val) => val.len(), | ||
| HummockValue::Delete => 0, | ||
| SharedBufferValue::Insert(val) | SharedBufferValue::Update(val) => { | ||
| val.len() |
There was a problem hiding this comment.
Should we add size_of_val here as well?
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
After #15300 and #15417
In this PR, we will compact and upload the old values of state store when
is_log_storeis enabled.In
SharedBufferBatch, whenis_log_storeis enabled, an extraold_valueof typeVec<Bytes>will be used to stored the old value. Thenew_valueof shared buffer batch now stores a new enumSharedBufferValue. The difference to the previously usedHummockValueis thatSharedBufferValuehas an extra enum caseUpdateto indicate that the operation is an update. When the enum isDeleteandUpdatethe value inold_valuewill be the corresponding old value, and forInsert, it means there is no old value for the current key op, and the value will be an empty bytes. Therefore, theold_valuehas the same length as thenew_value.In
SharedBufferBatchIterator, we add a new generic const boolIS_NEW_VALUEto indicate whether the iterator is iterating over new value or old value. IfIS_NEW_VALUEis true, the logic will be the same as original. IfIS_NEW_VALUEis false, thevaluemethod will take the value from theold_value, and in thenext_call, we will further advance the iterator to skip the key with op asInsert, becauseInserthas no old value.The old value will be compacted to separate SSTs and add to hummock version.
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.