Open poppindouble opened 4 years ago
Historical index ("When does id=1
every has age = 70
") is an interesting feature, but the storage & speed penalty can be very severe.
Can we make this optional? By default, indices are newest-only, but if the user requires historical index, we can give them that as well. What's the implementation cost of that?
@poppindouble
I think here is the solution that can achieve both historical index and newest-only.
We will adopt solution 1 in the above proposal. Let's have a look about the expensive part of this solution:
really heavy work load at the execute_create_index period, it go through all the historical data in the whole grouping.
a huge problem when we do
SelectCondition::NameProperty
on some "hot" data, since everytime we query byNameProperty
, for each existedunit_id
andchian_height
, we involve one inspect operation, which is really costly.
For the first point, I don't think we have a better solution, since we want to support index historical data, we have to read through all the historical data, even it is optional.
For the second point, we can make it optional, we will add another parameter when we do SelectCondition::NameProperty
, let's say the parameter's name is include_history
, it is a boolean.
If include_history
is false
, we just need to use unit_id
part in our result list. we get all the "newest" value of for each unit_id
. Which cost is exactly like before, no penalty here.
If include_history
is true
, here might be some penalty, in the above proposal, I mentioned that for each unit_id
and chian_height
, we involve one inspect operation. I mentioned inspection
because we don't have a proper API for only for "index" feature, but we do have a function in revert
feature,
fn get_value_after_height_recurse(
&self,
key: &StoreKey,
requested_height: &ChainHeight,
recurse_time: u16,
) -> ImmuxResult<StoreValue> {
if recurse_time > MAX_RECURSION {
return Err(VkvError::TooManyRecursionInFindingValue.into());
}
match self.get_journal(key) {
Err(error) => return Err(error),
Ok(journal) => {
let possible_heights: Vec<_> = journal
.update_heights
.iter()
.take_while(|h| h <= requested_height)
.collect();
for height in possible_heights.into_iter().rev() {
let record = self.load_instruction_record(&height)?;
let instruction = &record.instruction;
match instruction {
Instruction::DataAccess(DataInstruction::Write(write)) => match write {
DataWriteInstruction::SetMany(set_many) => {
for target in &set_many.targets {
if target.key == *key {
return Ok(target.value.clone());
}
}
}
DataWriteInstruction::RevertMany(revert_many) => {
for target in &revert_many.targets {
if target.key == *key {
return Ok(self.get_value_after_height_recurse(
key,
&target.height,
recurse_time + 1,
)?);
}
}
return Err(VkvError::CannotFindSuitableVersion.into());
}
DataWriteInstruction::RevertAll(revert_all) => {
return Ok(self.get_value_after_height_recurse(
key,
&revert_all.target_height,
recurse_time + 1,
)?);
}
},
_ => return Err(VkvError::UnexpectedInstruction.into()),
}
}
return Err(VkvError::CannotFindSuitableVersion.into());
}
}
}
I think one optimization is that we can reuse this function in our scenario as well, so we don't need to go through all the chain_height
regarding to one unit_id
, we just need to query those chain_height
regarding to that unit_id
which matters to the index.
I have another suggestion here, it is an implementation detail thing, when I was going through the code of get_value_after_height_recurse
, for our revert_instruction
, we use recursive method to get its original value, here is the definition of revert_instruction
.
#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct RevertManyInstruction {
pub targets: Vec<RevertTargetSpec>,
}
and
#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct RevertTargetSpec {
pub key: StoreKey,
pub height: ChainHeight,
}
we only record down the key
and height
, and this height
is the chain_height
which we revert from, so I think a better way to do it is that we also record down the value, so it act like a buffer, then we don't need to do the recursion style until we hit the DataWriteInstruction::SetMany
condition. it is like a memorize search thinking. I have this idea is because, I think this will also help in index feature. It is an optimization for query those chain_height
regarding to that unit_id
which matters to the index.
To achieve the above implementation detail thing, let's have a look in this part of our code base:
fn revert_one(
&mut self,
key: &StoreKey,
target_height: ChainHeight,
next_height: ChainHeight,
) -> ImmuxResult<()>
// We can return the revert value here.
DataWriteInstruction::RevertMany(revert) => {
for target in revert.targets.iter() {
self.revert_one(&target.key, target.height, next_height)?;
}
let record: InstructionRecord = instruction.to_owned().into();
// before we save the revert instruction, we buffer its return value here.
if let Err(_) = self.save_instruction_record(&next_height, &record) {
return Err(VkvError::SaveInstructionFail.into());
}
return Ok(Answer::DataAccess(DataAnswer::Write(
DataWriteAnswer::RevertOk(RevertOkAnswer {}),
)));
}
We can give a return value from revert_one
, and save this return value into our instruction. In this way, we can get rid of the recursive way, also accelerate index feature.
@blaesus
Our database has some index out of sync bugs.
What is the issue
Steps to reproduce the bug
Let's assume the
id
of these twoUnit
are1
and2
.We call
execute_create_index
to create index for currentgrouping
.Till so far, our index works as expected. We can query our db with
select
byage=70
, orselect
byage=80
, we will get the expected result.We are changing the age from 70 to 100.
select
byage=70
, orselect
byage=100
, both of the query will get the result:Which is not the expected result of
select
byage=70
.This bug will also lead to the problem of versioning, similar to the above example, after we call
execute_create_index
, if we do arevert
operation, the index will have the result like above, which is out of sync.The reason behind this bug
execute_create_index
, we are actually only creating index for current db, let's have a look in ourexecute_create_index
function.We are using
prefix
here to extract all theunit
in current grouping, which is the "newest"unit
for eachunit_id
, our history data are stored in theInstructionRecord
, which are not being "indexed".unit
which has the sameunit_id
as the previous unit (step 3 in the above example), filedage
is already being recorded inindexed_names
, theupdates_for_index
will be called:if the new inserted
unit
has the sameunit_id
as before, which will leads to both oldid_list_key
and the newid_list_key
will point to the sameunit_id
.The versioning problem also comes out from this reason, when we do
revert
operation, essentialy, we just insert a newunit
with some existedunit_id
.but we didn't update our index properly in the following code:
How to fix it.
The essential problem of this bug is that the index is out of sync, only the "newest" unit are indexed when we call
"execute_create_index"
, and then when "new"unit
with the sameunit_id
are inserted, our index system is not updated properly, this particularunit_id
are existed in bothid_list
(correspondingid_list_key
areget_store_key_of_indexed_id_list(&insert.grouping, &property_name, &old_unit_content);
andget_store_key_of_indexed_id_list(&insert.grouping, &property_name, &new_unit_content);
).I have two ideas how to solve this problem:
Solution 1
When we call
"execute_create_index"
, we create index for everything including history data. This solution requires that we need to scan all history data as well. This can be achieved by following steps:units
in the specificgrouping
(We can achieve this byprefix
).unit_id
from step 1, and for eachunit_id
, we doinspect
.inspect
is the historical data regarding to that specificunit_id
, at this point, we can know these informations:grouping
,unit_id
,unit_content
,NameProperty
, as well as itschain_height
.unit_id
only, we should also pass inchain_height
as well.So, we are not only saving
unit_id
but also saving its correspondingchain_height
together. I don't have a proper name for the combination of (unit_id
,chain_height
) for now, if you can help with naming, that would be great.When we are query our DB with
SelectCondition::NameProperty
, what we get will be a list of the combination of (unit_id
,chain_height
), so, the following problem is how to get theunit
when we know itsunit_id
andchain_height
. We don't have a specific API for this kind of query yet, but we can useinspect
to get it.For this solution, we also need to remember to update our index when we do
revert
operation.Solution 2
We only keep the "newest" index, for each corresponding
unit_id
, we don't supportSelectCondition::NameProperty
for its historical data. In the above example, when we query our db withselect
byage=70
, orselect
byage=100
, both of the query will get the result:but with solution 2, we will only get the above result if we do
select
byage=100
. We don't get the above result if we query our db withselect
byage=70
. At least our db is correct in this way.To achieve this, we need to keep our index update to date, which means we can keep our
create_index
as it is for now, but every time when we insert a new unit, or we revert unit or units, we need to update our db's index.To update our index, we need to do a read operation regarding to this
unit_id
first, find out what is theunit_content
s of thisunit_id
in current db, then, we go through all theNameProperty
, we need to do a querySelectCondition::NameProperty
, and delete itsunit_id
in the correspondingindexed_id_list
.After we call
execute_create_index
, We need to do thisupdate_index
operation in several places in our codebase.unit
or multiunits
.unit
or multiunits
.Comparison
Solution 1:
Pros:
SelectCondition::NameProperty
as well. the cost ininsert
andrevert
is lower then solution 2.Cons:
execute_create_index
period, it go through all the historical data in the whole grouping.SelectCondition::NameProperty
on some "hot" data, since everytime we query byNameProperty
, for each existedunit_id
andchian_height
, we involve oneinspect
operation, which is really costly.Solution 2:
Pros:
execute_create_index
period.SelectCondition::NameProperty
is not costly.Cons:
indexed_id_list
, everytime when we insert a newunit
orunits
, or revert aunit
orunits
, before we can actually perform these operations, we need to do following steps:(I) A read operation regarding to this
unit_id
. (II) Go through all theNameProperty
, for eachNameProperty
, do a querySelectCondition::NameProperty
. (III) Delete itsunit_id
in the correspondingindexed_id_list
insert
andrevert
operation in solution 2 is costly then solution 1.