mssmt: add transactional tree storage interface and sqlite implementation #90
Conversation
There was a problem hiding this comment.
Great work!
Still in a draft state, so did a light pass, mainly focusing on the SQL/DB aspects. I love how elegant the recursive CTE query is here for fetching a sub-tree. I think I grok it mostly, but need to let it marinate in my mind a bit longer. After looking at the query (namely limit 3), I think I see what you mean here about if the "unrolled" join would be faster or not. The query seems pretty flexible as is (can parameterize 3 to allow it to fetch a larger sub tree), and I think if we wanted to get really crazy, we could use SUBSTR to implement a recursive branch fetching to create merkle proofs.
| isLeft := bytes.Equal(row.HashKey, lHashKey) | ||
| isRight := bytes.Equal(row.HashKey, rHashKey) | ||
|
|
||
| if !isLeft && !isRight { |
There was a problem hiding this comment.
Will this actually ever happen given the query limits 3 by so should only ever return the immediate left+right child? Or is this related to the compacted leaf?
There was a problem hiding this comment.
Yeah, so I'll cover this with additional unit test if we approve the current recursive schema. Since we don't store the default nodes in the table (which we may since it's making everything more elegant), we could end up fetching the root, it's child and the child of the child in which case we want to ignore that next level child (and just use the default branch instead).
There was a problem hiding this comment.
This is due to how recursive cte query works, it won't just anchor the two children to the starter query, but will do so recursively too.
There was a problem hiding this comment.
we could end up fetching the root, it's child and the child of the child in which case we want to ignore that next level child (and just use the default branch instead).
Gotcha that makes sense. If this is the case, then I think the idea of explicitly passing in a level accumulator into the CTE would help us have a tighter grasp on the recursion?
| sum BIGINT NOT NULL | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS mssmt_nodes_hash_key_idx ON mssmt_nodes (hash_key); |
There was a problem hiding this comment.
Since the hash key is a PRIMARY KEY, that already creates an index.
There was a problem hiding this comment.
We might want to add indexes for l_hash_key and r_hash_key as they're used in the recursive WHERE clause.
There was a problem hiding this comment.
Yeah, tried adding the indexes for the left/right keys too, but weirdly the query became slower. Still investigating why.
bhandras
force-pushed
the
tree-storage
branch
9 times, most recently
from
3fadc17
to
3633f71
Compare
|
Dear reviewers, the PR is now ready for review 🚀 |
This commit changes the mssmt.Store and mssmt.Tree interfaces and the underlying implementations to allow transactional behavior, meaning all updates and reads on the tree would become atomic and either fail (and roll back state) or succeed.
tarocli: fix deleting universes from federation
This PR changes the
mssmt.Storeinterface to allow transactional updates and reads of underlying trees. We also add an sqlite based implementation.TODO:
- fix transaction contexts, currently all are simplycontext.TODO()- make sure failed transactions roll back the tree roots in all cases- improve test coverage- more experiments with the schema to allow better performance