feat(store/v2): Removing old store keys by pruning #20927
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@cool-develope your pull request is missing a changelog! |
cool-develope
changed the title
cool-develope
changed the title
store/v2/commitment/iavl/tree.go
Outdated
| @@ -107,6 +108,7 @@ func (t *IavlTree) SetInitialVersion(version uint64) error { | |||
|
|
|||
| // Prune prunes all versions up to and including the provided version. | |||
| func (t *IavlTree) Prune(version uint64) error { | |||
| // latestVersion := t.tree.Version() | |||
There was a problem hiding this comment.
nit: dead code, please remove
store/v2/commitment/metadata.go
Outdated
| } | ||
| }() | ||
|
|
||
| end := []byte(fmt.Sprintf("%s%020d/", removedStoreKeyPrefix, version+1)) |
There was a problem hiding this comment.
personal preference: "%s%020d/", is used in multiple places. Would it make sense to introduce a helper method like buildRemovedStoreKeysPrefix(key, version)[]byte
There was a problem hiding this comment.
The version element is encoded into a 20 byte representation. Why not use big endian encoding instead so that you have only 8 bytes?
store/v2/commitment/metadata.go
Outdated
| }() | ||
|
|
||
| for ; iter.Valid(); iter.Next() { | ||
| storeKey := string(iter.Key()[len(end):]) |
There was a problem hiding this comment.
nit: move len(end) before the loop
| } | ||
| }() | ||
|
|
||
| for ; iter.Valid(); iter.Next() { |
There was a problem hiding this comment.
🤔 surprised to see this. Is it more efficient to iterate over the DB than to use the storeKeys and version parameter?
store/v2/storage/pebbledb/db.go
Outdated
|
|
||
| var storeKeys []string | ||
| for storeKeyIter.First(); storeKeyIter.Valid(); storeKeyIter.Next() { | ||
| storeKey := string(storeKeyIter.Key()[len(end):]) |
There was a problem hiding this comment.
nit: move len() before the loop
store/v2/storage/sqlite/db.go
Outdated
| ); | ||
| ` | ||
|
|
||
| if _, err = tx.Exec(pruneRemovedStoreKeysStmt, reservedStoreKey, keyRemovedStore, version); err != nil { |
There was a problem hiding this comment.
keyRemovedStore is not set in PruneStoreKeys
store/v2/storage/sqlite/db.go
Outdated
| ` | ||
|
|
||
| for _, key := range storeKeys { | ||
| _, err = tx.Exec(flushRemovedStoreKeysStmt, reservedStoreKey, key, version) |
There was a problem hiding this comment.
you are passing only 3 parameters to the statement
store/v2/storage/sqlite/db.go
Outdated
|
|
||
| // prune removed store keys | ||
| pruneRemovedStoreKeysStmt := `DELETE FROM state_storage | ||
| WHERE store_key in ( |
There was a problem hiding this comment.
not fully sure if I understood the DB schema but I think that you also have to define the version to not delete all for a storage key
There was a problem hiding this comment.
good catch, I've fixed and added the testcases to ensure that.
store/v1 shares this behavior right? i.e. cosmos-sdk/store/rootmulti/store.go Lines 504 to 513 in 6708818 Ostensibly light clients (like used in IBC) have been accounting for this across store key removals, but it does seem like a long standing bug, is there an issue for it? |
yes exactly, but not able to find the related issue, maybe removing storekey is not used or at least not frequently |
store/v2/commitment/metadata.go
Outdated
| }() | ||
|
|
||
| for _, storeKey := range storeKeys { | ||
| key := []byte(fmt.Sprintf("%s%s", buildRemovedStoreKeyPrefix(version), storeKey)) |
There was a problem hiding this comment.
here and others: let's move away from strings:
| key := []byte(fmt.Sprintf("%s%s", buildRemovedStoreKeyPrefix(version), storeKey)) | |
| key := append(buildRemovedStoreKeyPrefix(version), storeKey...) |
There was a problem hiding this comment.
you mentioned, fmt.Sprintf is more safe in terms of array expanding 🤔
store/v2/commitment/metadata.go
Outdated
| func buildRemovedStoreKeyPrefix(version uint64) []byte { | ||
| buf := make([]byte, 8) | ||
| binary.BigEndian.PutUint64(buf, version) | ||
| return []byte(fmt.Sprintf("%s%x/", removedStoreKeyPrefix, buf)) |
There was a problem hiding this comment.
The %x would encode it into hex representation which is 4 chars per byte. Why not work with bytes only, like:
const separator byte = '/'
func buildRemovedStoreKeyPrefix(version uint64) []byte {
n := len(removedStoreKeyPrefix)
buf := make([]byte, n+8+1)
copy(buf[0:], removedStoreKeyPrefix)
binary.BigEndian.PutUint64(buf[n:], version)
buf[n+8]=separator
return buf
}(untested)
| @@ -109,8 +116,7 @@ func (c *CommitStore) LoadVersionAndUpgrade(targetVersion uint64, upgrades *core | |||
| for storeKey := range c.multiTrees { | |||
| storeKeys = append(storeKeys, storeKey) | |||
| } | |||
| // deterministic iteration order for upgrades | |||
| // (as the underlying store may change and | |||
| // deterministic iteration order for upgrades (as the underlying store may change and | |||
There was a problem hiding this comment.
It was not stdlib but x/exp for maps. My mistake. 🙈
Feel free to revert to avoid the dependency if you prefer.
store/v2/storage/sqlite/db.go
Outdated
| @@ -203,36 +195,30 @@ func (db *Database) Prune(version uint64) error { | |||
| t2.version <= ? | |||
| ) AND store_key != ?; | |||
| ` | |||
|
|
|||
| if _, err = tx.Exec(pruneStmt, version, reservedStoreKey); err != nil { | |||
| if err := db.executeTx(pruneStmt, version, reservedStoreKey); err != nil { | |||
There was a problem hiding this comment.
With executeTx you split the DB transaction into multiple smaller ones. Unless there is a technical reason, it should be avoided. DB transactions should cover the whole (business) process instead to have atomic behaviour.
store/v2/storage/sqlite/db.go
Outdated
| VALUES(?, ?, ?, ?); | ||
| ` | ||
| flushRemovedStoreKeysStmt := fmt.Sprintf(`INSERT INTO state_storage(store_key, key, value, version) | ||
| VALUES %s`, strings.Repeat("(?, ?, ?, ?),", len(storeKeys))) |
There was a problem hiding this comment.
🤔 I have never seen an insert with repeated placeholders. What are the benefits? With dynamic values possible, the DB can not optimize internally for the prepared statement AFAIK.
You also need to check for empty storeKeys slice or the statement would be incorrect.
The old version looks more common and trustworthy ;-)
store/v2/storage/sqlite/db.go
Outdated
| WHERE store_key in ( | ||
| SELECT value FROM state_storage | ||
| WHERE store_key = ? AND key = ? AND version <= ? | ||
| WHERE store_key IN ( |
There was a problem hiding this comment.
Would it make sense to limit the delete to version <= ? ?
I think about odd edge cases where store foo is renamed to bar and back?
There was a problem hiding this comment.
so we are checking version <= ? in SELECT, do you think it is not fully covered?
There was a problem hiding this comment.
The when the delete does not contain a version condition as well, it will delete everything with a matching store key.
For example
height 10: add store foo + some data
height 20: delete store foo
height 30: add store foo + some data
When prune is is run on height 31, it would find the store marked for delete (at height 20) and deletes everything although it is in use again.
This opens another question, if the store is not pruned, would it contain dirty data?
Given the adoption of IBC and light clients generally, I'd say this is not a problem, perhaps just expected behavior. This PR adds a lot of complicated code to metadata store and the pruning process, which is already quite complex. What led you to address removed store keys at this level, were there user complaints? |
I agree it introduces the extra complexity, one violated case is the archive node if remove old keys instantly. |
store/v2/commitment/metadata.go
Outdated
| return batch.WriteSync() | ||
| } | ||
|
|
||
| func (m *MetadataStore) deleteRemovedStoreKeys(version uint64, fn func(storeKey []byte) error) (err error) { |
There was a problem hiding this comment.
nit: is fn a filter function? if so can it have a more descriptive name? kind of hard to read
| ) | ||
|
|
||
| type MountTreeFn func(storeKey string) (Tree, error) |
There was a problem hiding this comment.
needs a godoc if to be included
There was a problem hiding this comment.
is there an example usage of this in tests? trying to wrap my head around how it will work in GetProof. would multiple different SC still be supported? the current has a homogenous mounTreeFn for heterogeneous map[string]Tree
There was a problem hiding this comment.
yes, it is tested in store_test_suite.go
| realStoreKeys := []string{"renamedStore1", storeKey2, "newStore1", "newStore2"} | ||
| commitStore, err = s.NewStore(commitDB, newStoreKeys, log.NewNopLogger()) | ||
| s.Require().NoError(err) | ||
| err = commitStore.LoadVersionAndUpgrade(latestVersion, upgrades) | ||
| s.Require().NoError(err) | ||
|
|
||
| // verify removed stores | ||
| // GetProof should work for the old stores |
There was a problem hiding this comment.
@kocubinski MountFn is being tested here
|
|
||
| for _, tc := range testcases { | ||
| got := BuildPrefixWithVersion(tc.prefix, tc.version) | ||
| if len(got) != len(tc.want) { |
There was a problem hiding this comment.
Good test cases.
You can simplify the assert with:
if !bytes.Equal(tc.want, got) {
t.Fatalf("expected %X, got %X", tc.want, got)
}
store/v2/storage/sqlite/db.go
Outdated
| WHERE store_key in ( | ||
| SELECT value FROM state_storage | ||
| WHERE store_key = ? AND key = ? AND version <= ? | ||
| WHERE store_key IN ( |
There was a problem hiding this comment.
The when the delete does not contain a version condition as well, it will delete everything with a matching store key.
For example
height 10: add store foo + some data
height 20: delete store foo
height 30: add store foo + some data
When prune is is run on height 31, it would find the store marked for delete (at height 20) and deletes everything although it is in use again.
This opens another question, if the store is not pruned, would it contain dirty data?
store/v2/storage/sqlite/db.go
Outdated
| @@ -329,6 +327,25 @@ func (db *Database) PrintRowsDebug() { | |||
| fmt.Println(strings.TrimSpace(sb.String())) | |||
| } | |||
|
|
|||
| func (db *Database) executeTx(stmt string, args ...interface{}) (err error) { | |||
There was a problem hiding this comment.
personal preference: It is called by one place only. You can inline the method or add some docs please that it is used for a single operation TX only. should be avoided for atomic behaviour on multiple statements
store/v2/storage/pebbledb/db.go
Outdated
| if err := batch.Delete(storeKeyIter.Key(), nil); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| for _, key := range storeKeys { | ||
| for _, s := range storeKeys { |
There was a problem hiding this comment.
would it make sense to search for max version per storeKey?
store/v2/storage/pebbledb/db.go
Outdated
| itr, err := db.storage.NewIter(&pebble.IterOptions{LowerBound: storePrefix(storeKey), UpperBound: storePrefix(util.CopyIncr(storeKey))}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer itr.Close() | ||
|
|
||
| for itr.First(); itr.Valid(); itr.Next() { | ||
| itrKey := itr.Key() | ||
| _, verBz, _ := SplitMVCCKey(itrKey) |
There was a problem hiding this comment.
Why skipping the sanity check on ok result?
store/v2/storage/pebbledb/db.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if keyVersion > s.version { |
store/v2/storage/sqlite/db.go
Outdated
| WHERE store_key = ? AND value = ? AND version <= ? | ||
| GROUP BY key | ||
| ) AS t | ||
| WHERE s.store_key = t.key AND s.version <= t.max_version |
There was a problem hiding this comment.
I have tested this locally. The EXISTS operator feels a bit strange but it works with the where clause 👍
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Description
ref: #19784 #20453
In the original PR, we removed old store keys instantly when upgrading store keys. It will break the
GetProoffor the history on IBC.The new design removes old store keys by pruning rather than instant deleting.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...