feat(iavl): add KV data reader & writer, and mmap wrapper #25645
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "fmt" | ||
| "math" | ||
| "os" | ||
| "unsafe" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25645 +/- ##
==========================================
+ Coverage 70.26% 70.37% +0.11%
==========================================
Files 835 842 +7
Lines 54361 54888 +527
==========================================
+ Hits 38196 38628 +432
- Misses 16165 16260 +95
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| if unsafe.Sizeof(ChangesetInfo{}) != sizeChangesetInfo { | ||
| panic(fmt.Sprintf("invalid ChangesetInfo size: got %d, want %d", unsafe.Sizeof(ChangesetInfo{}), sizeChangesetInfo)) | ||
| } | ||
| } |
There was a problem hiding this comment.
This was missing in the previous PR
|
|
||
| // ValueOffset is the offset the value data for this node in the key value data file. | ||
| // The same size considerations apply here as for KeyOffset. | ||
| ValueOffset uint32 |
There was a problem hiding this comment.
In order to efficiently cache keys, we need to allow key and value bytes to be non-contiguous in the data file. Adding a separate value offset allows us to put key and value data wherever we want to. Hopefully, the additional 4 bytes per leaf node is offset by more key caching in the kv data file.
iavl/internal/mmap.go
Outdated
| "io" | ||
| "os" | ||
| ) | ||
| import "github.com/edsrzf/mmap-go" |
There was a problem hiding this comment.
For now I am using this off-the-shelf mmap wrapper which has the highest number of known importers on pkg.go.dev: https://pkg.go.dev/github.com/edsrzf/mmap-go?tab=importedby
In the future, it may be worth considering creating our own mmap wrapper. On linux, it may be possible to apply an optimization where we can resize the mmap without unmapping memory: https://stackoverflow.com/questions/74243583/memory-map-file-with-growing-size
|
@aaronc your pull request is missing a changelog! |
|
@aaronc a few more linter compaints |
| if err != nil { | ||
| return fmt.Errorf("failed to read cached key offset at %d: %w", wr.offset, err) | ||
| } | ||
| wr.offset += 4 |
There was a problem hiding this comment.
Because we've just read a 4-byte uint32
| // KVEntryKeyBlob indicates a standalone key data entry. | ||
| // This should be followed by varint length + raw bytes. | ||
| // Used for compacted (non-WAL) leaf or branch keys not already cached. | ||
| KVEntryKeyBlob KVEntryType = 0x4 | ||
|
|
||
| // KVEntryValueBlob indicates a standalone value data entry. | ||
| // This should be followed by varint length + raw bytes. | ||
| // Used for compacted (non-WAL) leaf values. | ||
| // The main difference between KVEntryKeyBlob and KVEntryValueBlob is that key | ||
| // entries may be cached for faster access, while value entries are not cached. |
There was a problem hiding this comment.
can you explain more about the caching here? this wasn't in the previous version was it?
There was a problem hiding this comment.
Caching wasn't in the previous version, no. Basically in looking at data files, the kv data files are much larger than any of the other files, and my theory is we're storing lots of duplicate key data. There are likely lots of storage locations in the database which are written to repeatedly with the same key. Also branch nodes always share keys with some leaf nodes. So introducing this caching is a very simple form of data compression that hopefully will lead to some reduction in storage. There could be other forms of compression we considered, but this seems pretty straightforward and likely to have some pay off, and all it costs us is a little extra memory to maintain the cache while we're writing a file. My suggestion would be to try this and compare data file sizes between the current version and this one.
|
re: discussion for reviewers i think i'd prefer safety over allowing larger kv. we just need to make sure this design is known in a document somewhere |
So would you prefer that I update this PR to error when key or value size exceed the proposed limits (2^16-1 or 64kb for keys and 2^24-1 or 16mb for values)? I can make sure that this is in go doc comments, but there probably should be some document higher up that states this too - not sure where that should be. |
Yes, i think that is the right direction.
Whenever we add the readme back in, we could probably have a section on limits or key/values in general |
Description
This PR specifies the IAVLX KV data file format for storing the WAL as well as other key-value data (branch node keys and compacted changeset KV data), and implements the
KVDataReader,KVDataWriterandWALReadertypes. It also adds the convenienceFileWriterandMmapwrapper types.One design question for reviewers is whether we should proactively limit key and value size. I would suggest a key limit of 2^16-1 (64KB) and a value limit of 2^24-1 (16MB). Currently, this KV data file uses 32-bit offsets which limits its size to 4gb before we have to roll over. When initially writing changesets, we should probably roll over around 1 or 2gb and then compact up to 4gb. If, however, while writing a version we ran out of space, the node would crash non-deterministically. This is unlikely to happen if we roll over at 1 or 2gb unless someone introduces some really large unexpected KV data. Setting a limit to key and value size would be consensus breaking (unlikely to ever get triggered in practice), but would make such pathological scenarios cause nodes to fail more deterministically based on validation rather than just running out of disk space. We could also explore larger offsets of 40-64bits, but the larger the
kv.datfile is, the more extra disk space we need when doing compaction. And also really large key/value data should probably be considered pathological anyway. Any thoughts on all of this?