Add an alternative to DeleteVersions that leaves no garbage by klim0v · Pull Request #324 · cosmos/iavl

klim0v · 2020-11-04T08:14:26Z

The beginning of this issue is that the DeleteVersions method leaves behind a lot of dirty information, which leads to an increase in storage used by the storage.

I first added a method to remove versions up to a certain height, and now I have expanded it to allow removal within a given range.

DeleteVersionsTo deletes all versions below the given height

tree_test.go

erikgrinaker · 2020-11-05T15:13:10Z

Thanks for contributing! I see you're still making changes to this, and that there are a few linter issues. Let me know when it's ready for review! Also, maybe convert this to a draft until it's ready.

klim0v · 2020-11-05T16:46:29Z

Thanks for contributing! I see you're still making changes to this, and that there are a few linter issues. Let me know when it's ready for review! Also, maybe convert this to a draft until it's ready.

I see linter crashing in CI / CD config, updated to 1.28 and added nosec tag to ignore
Error: Failed to run: Error: requested golangci-lint version 'v1.27' isn't supported: we support only v1.28.3 and later versions, Error: requested golangci-lint version 'v1.27' isn't supported: we support only v1.28.3 and later versions

I finished making changes, you can proceed to check! @erikgrinaker

klim0v · 2020-11-05T16:53:44Z

Finished!

klim0v · 2020-11-05T23:49:28Z

@erikgrinaker, I added an alternative, can you check both of them? klim0v/iavl@6933e53...82251d8

klim0v · 2020-11-06T00:35:50Z

I also suggest to mark the DeleteVersions method as deprecated and try to pass sorted versions from it to the DeleteVersionsInterval.

klim0v · 2020-11-06T00:37:29Z

I also suggest to mark the DeleteVersions method as deprecated and try to pass sorted versions from it to the DeleteVersionsInterval.

@alexanderbez, how do you like it?

erikgrinaker · 2020-11-06T11:56:59Z

I've backported a bunch of linter fixes from master, so you can rebase to avoid all of the linter stuff.

I'd be interested to find out exactly what this garbage you see left behind by DeleteVersions actually is. We have a test TestRandomOperations that runs randomized operations against a tree and checks invariants, including calling DeleteVersion on all versions in random order, and in #326 I've added a test to make sure there are no stray database entries left behind at the end. It doesn't leave any garbage.

Since DeleteVersions basically just iterates over all versions and calls DeleteVersion, this should be equivalent to what this test does, and it doesn't produce any garbage in the database. Can you elaborate?

Checks that we don't leave behind any junk in the database, as reported in #324.

erikgrinaker · 2020-11-09T11:34:21Z

Ok, great - did it help with the performance though? I suspect using Range and calling Seek() are basically the same thing, thus it shouldn't have an effect on performance, but would be good to verify.

klim0v · 2020-11-09T11:42:57Z

Ok, great - did it help with the performance though? I suspect using Range and calling Seek() are basically the same thing, thus it shouldn't have an effect on performance, but would be good to verify.

Yes, it helped! Now I'm looking for an explanation of why this happened.

klim0v · 2020-11-09T12:05:22Z

I suppose Seek() could be slow due to the need to go through the entire tree to make sure there is no such key.

erikgrinaker · 2020-11-09T12:46:58Z

Great! It may be that Seek() actually iterates. Either way, want to submit a PR for tm-db?

klim0v · 2020-11-09T13:04:20Z

Great! It may be that Seek() actually iterates. Either way, want to submit a PR for tm-db?

Yes, but I can't yet confirm my words with performance tests to describe this problem...

erikgrinaker

This PR includes a bunch of noisy formatting changes, it would be nice to avoid these since they're unrelated to the main change.

Also, I've released new versions of tm-db and updated the 0.15 and 0.14 branches of IAVL.

mutable_tree.go

tree_random_test.go

…to delete them.

klim0v · 2020-11-10T16:34:45Z

This PR includes a bunch of noisy formatting changes, it would be nice to avoid these since they're unrelated to the main change.

Also, I've released new versions of tm-db and updated the 0.15 and 0.14 branches of IAVL.

Done ✅

erikgrinaker

Great stuff, thank you for this! Suggested a few minor nitpicks, otherwise LGTM!

mutable_tree.go

tree_random_test.go

CHANGELOG.md

klim0v · 2020-11-11T21:08:52Z

I have added your recommendations, thanks!

* fix: constant overflow when compiling for 32bit machines (cosmos#318) on Go 1.x(x) all constants are ints by default which causing build for 32bit machines to fail due to overflow golang/go#23086 Signed-off-by: Artur Troian <troian.ap@gmail.com> * changelog: release 0.14.2 * lint: update golangci-lint to 1.29 (cosmos#297) * ci: freeze golangci action version (cosmos#303) * github: bump golangci-lint and fix linter issues (cosmos#325) * test: check stray database entries in TestRandomOperations (cosmos#326) Checks that we don't leave behind any junk in the database, as reported in cosmos#324. * revert Protobuf methods accidentally cherry-picked in a41e36b * go.mod: bump tm-db to 0.5.2 * fix DeleteVersions stray orphans, and add DeleteVersionsRange (cosmos#324) * test: add TestRandomOperations case for DeleteVersions * test: run slow tests without race detector in CI (cosmos#338) * changelog: release 0.14.3 (cosmos#333) Co-authored-by: Artur Troian <troian@users.noreply.github.com> Co-authored-by: Erik Grinaker <erik@interchain.berlin> Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Klimov Sergey <klim0v-sergey@yandex.ru>

add MutableTree#DeleteVersionsTo feature

5ac16c5

DeleteVersionsTo deletes all versions below the given height

klim0v requested a review from erikgrinaker as a code owner November 4, 2020 08:14

klim0v added 7 commits November 4, 2020 12:07

fix

ede3c03

optimization

f152119

fix

adf34a3

optimization

3774cfe

optimization

1d611a1

fix delete last version

bcd3237

refactoring

f45b4df

klim0v commented Nov 4, 2020

View reviewed changes

tree_test.go Show resolved Hide resolved

klim0v changed the title ~~add MutableTree#DeleteVersionsTo feature~~ Add an alternative to DeleteVersions that leaves no garbage Nov 4, 2020

klim0v added 2 commits November 4, 2020 20:05

edit error message

e4f96eb

rename DeleteVersionsInterval

7da8d73

klim0v added 2 commits November 5, 2020 19:29

update golangci-lint version

e55f6df

add nosec tag to ignoring golangci-lint

3be5021

klim0v added 2 commits November 5, 2020 19:48

raplace /* #nosec */ to //nolint:gosec

6a59e9a

rollback: replace /* #nosec */ to //nolint:gosec

7761d0e

klim0v added 3 commits November 6, 2020 01:55

rename test

a759e1d

fix

6933e53

alternative variant

69e2130

rename variables

82251d8

erikgrinaker mentioned this pull request Nov 6, 2020

test: check stray database entries in TestRandomOperations #326

Merged

mergify bot pushed a commit that referenced this pull request Nov 6, 2020

test: check stray database entries in TestRandomOperations (#326)

65a3b9c

Checks that we don't leave behind any junk in the database, as reported in #324.

add random DeleteVersionsRange calls to TestRandomOperations

7d3a16f

klim0v requested a review from erikgrinaker November 9, 2020 12:52

klim0v mentioned this pull request Nov 9, 2020

add Range to NewIterator tendermint/tm-db#134

Merged

erikgrinaker reviewed Nov 10, 2020

View reviewed changes

mutable_tree.go Show resolved Hide resolved

tree_random_test.go Outdated Show resolved Hide resolved

klim0v added 5 commits November 10, 2020 18:55

split DeleteVersions into two loops: one to find the ranges, and one …

8120f63

…to delete them.

upgrade tm-db from 0.5.1 to 0.5.2

a60e4b1

refactor TestRandomOperations

9f48411

rollback formatting

1f801ba

rollback formatting

41bcf66

Update CHANGELOG.md

796d4a9

klim0v requested a review from erikgrinaker November 10, 2020 16:47

erikgrinaker self-assigned this Nov 11, 2020

erikgrinaker approved these changes Nov 11, 2020

View reviewed changes

mutable_tree.go Outdated Show resolved Hide resolved

tree_random_test.go Outdated Show resolved Hide resolved

tree_random_test.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

small refactor

402d269

erikgrinaker merged commit 9af2c83 into cosmos:0.14.x Nov 12, 2020

erikgrinaker pushed a commit that referenced this pull request Nov 12, 2020

fix DeleteVersions stray orphans, and add DeleteVersionsRange (#324)

6f2989f

erikgrinaker mentioned this pull request Nov 12, 2020

port DeleteVersions and DeleteVersionsRange #329

Merged

erikgrinaker pushed a commit that referenced this pull request Nov 12, 2020

fix DeleteVersions stray orphans, and add DeleteVersionsRange (#324)

c4e854d

erikgrinaker pushed a commit that referenced this pull request Nov 12, 2020

fix DeleteVersions stray orphans, and add DeleteVersionsRange (#324)

078f48a

This was referenced Nov 24, 2020

feat: bump up iavl 0.4.3 Finschia/finschia-sdk#36

Merged

feat: update go version and bump up iavl Finschia/finschia-sdk#37

Merged

KamiD mentioned this pull request Apr 10, 2021

bump version to v0.14.3 okblockchainlab/iavl#5

Closed

Conversation

klim0v commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

erikgrinaker commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klim0v commented Nov 5, 2020

Uh oh!

klim0v commented Nov 5, 2020

Uh oh!

klim0v commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klim0v commented Nov 6, 2020

Uh oh!

klim0v commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikgrinaker commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikgrinaker commented Nov 9, 2020

Uh oh!

klim0v commented Nov 9, 2020

Uh oh!

klim0v commented Nov 9, 2020

Uh oh!

erikgrinaker commented Nov 9, 2020

Uh oh!

klim0v commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikgrinaker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

klim0v commented Nov 10, 2020

Uh oh!

erikgrinaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klim0v commented Nov 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

klim0v commented Nov 4, 2020 •

edited

Loading

erikgrinaker commented Nov 5, 2020 •

edited

Loading

klim0v commented Nov 5, 2020 •

edited

Loading

klim0v commented Nov 6, 2020 •

edited

Loading

erikgrinaker commented Nov 6, 2020 •

edited

Loading

klim0v commented Nov 9, 2020 •

edited

Loading

erikgrinaker left a comment •

edited

Loading