Delete by prefix operator in kvdb #360

cheme · 2020-03-13T09:52:13Z

This PR add a delete by prefix operator in kvdb.

This is something that I want to use in a PR that bulk delete a substrate child trie (substrate child trie got all their keys prefixed by a unique id, so a deletion by prefix at rocksdb level can be use).

Since it is currently only related to this future child trie deletion PR (in progress), it should not be merge until really needed (unless it seems good to have either way).
I am still creating the PR for review, feedback and visibility.

Writing this PR description, I realize that we could also use an operator that delete a range (start inclusive, end exclusive), which could allow more use cases but is not really needed.

…_del_range Block on liftime for column familly.

probably not an issue, dbiterator is one as it will fail on close db).

patch.

ordian · 2020-03-26T14:21:42Z

@cheme could you rebase on master? #313 is merged

ordian · 2020-03-26T15:11:21Z

kvdb/src/lib.rs

@@ -129,3 +149,20 @@ pub trait KeyValueDB: Sync + Send + parity_util_mem::MallocSizeOf {
 		IoStats::empty()
 	}
 }
+
+/// Return for a start inclusive prefix, the non inclusive end.
+pub fn end_prefix(prefix: &[u8]) -> Vec<u8> {


this function can be simplified

fn end_prefix(mut prefix: Vec<u8>) -> Vec<u8> { while let Some(0xff) = prefix.last() { prefix.pop(); } if let Some(byte) = prefix.last_mut() { *byte += 1; } prefix }

I'm not sure we want to expose it kvdb, I see it's used as a utility function in kvdb-rocksdb and kvdb-memorydb, but for example, we don't expose other_io_error

Also if the prefix is empty or 0xfffffff...ff, will it delete the whole db?

And we could reuse this function for iter_from_prefix by setting https://docs.rs/rocksdb/0.13.0/rocksdb/struct.ReadOptions.html#method.set_iterate_upper_bound

for empty prefix it does indeed delete every values (not in a very efficient way).

What about 0xff prefix? I think we should add some tests and document the difference between 0xff and 0xff00 as it's not intuitive.

I am not sure how to document that, I will add a test it may make thing more explicit.

oups, I actually meant 0xff and 0x00ff

I am not sure how to document that,

Let's add a few examples and include interesting cases such as 0xff and 0xff00 among the examples?

kvdb-rocksdb/src/lib.rs

kvdb-memorydb/src/lib.rs

ordian · 2020-03-26T15:18:02Z

kvdb-rocksdb/src/lib.rs

+							} else {
+								// delete the whole column
+								let end_range = &[255u8];
+								batch.delete_range_cf(cf, &prefix[..], &end_range[..]).map_err(other_io_err)?;


why do we need to delete a range, if we're deleting the whole column later?

I did not delete the whole column (at first I tried to but it requires mutable access and some changes overall). So I end up thinking that if we use the delete range to delete the column we are doing something wrong, so using delete range for it felt more consistent.
(I will change the comment)

Suggested change

batch.delete_range_cf(cf, &prefix[..], &end_range[..]).map_err(other_io_err)?;

batch.delete_range_cf(cf, &[], &end_range[..]).map_err(other_io_err)?;

kvdb-shared-tests/src/lib.rs

kvdb/src/lib.rs

ordian · 2020-03-26T16:00:03Z

(the CI can be fixed with cargo fmt btw)

Co-Authored-By: Andronik Ordian <write@reusable.software>

…n into upgraded_del_range_pr

kvdb-rocksdb/src/lib.rs

ordian · 2020-03-26T18:13:33Z

kvdb-memorydb/src/lib.rs

+							let end_range = Bound::Excluded(kvdb::end_prefix(&prefix[..]));
+							let keys: Vec<_> = col.range((start_range, end_range)).map(|(k, _)| k.clone()).collect();
+							for key in keys.into_iter() {
+								col.remove(&key[..]);


this could be optimized by using split_off and append, but currently append is slow: rust-lang/rust#34666 and I think memorydb is only used for tests and kvdb-web, so it's fine for now

ordian

LGTM overall (modulo a couple of unresolved comments).

dvdplm

A few nits.

dvdplm · 2020-03-26T19:47:32Z

kvdb-rocksdb/src/lib.rs

+								let end_range = kvdb::end_prefix(&prefix[..]);
+								batch.delete_range_cf(cf, &prefix[..], &end_range[..]).map_err(other_io_err)?;
+							} else {
+								// delete every values in the column


Suggested change

// delete every values in the column

// deletes all values in the column

dvdplm · 2020-03-26T19:52:39Z

kvdb/src/lib.rs

@@ -80,6 +83,11 @@ impl DBTransaction {
 	pub fn delete(&mut self, col: u32, key: &[u8]) {
 		self.ops.push(DBOp::Delete { col, key: DBKey::from_slice(key) });
 	}
+
+	/// Delete all values with the given key prefix.


Can we document what happens if called with an empty prefix (deletes all values)?

kvdb/src/lib.rs

dvdplm · 2020-03-26T20:05:19Z

kvdb/src/lib.rs

@@ -129,3 +149,20 @@ pub trait KeyValueDB: Sync + Send + parity_util_mem::MallocSizeOf {
 		IoStats::empty()
 	}
 }
+
+/// Return for a start inclusive prefix, the non inclusive end.
+pub fn end_prefix(prefix: &[u8]) -> Vec<u8> {


I am not sure how to document that,

Let's add a few examples and include interesting cases such as 0xff and 0xff00 among the examples?

dvdplm · 2020-03-26T20:06:29Z

kvdb/src/lib.rs

+	fn end_prefix_test() {
+		assert_eq!(end_prefix(&[5, 6, 7]), vec![5, 6, 8]);
+		assert_eq!(end_prefix(&[5, 6, 255]), vec![5, 7]);
+		// this is incorrect as the result is before start


In what sense is it incorrect? Is it "misusage"? Or is the end_prefix() handling this incorrectly?

I also thought this comment was a bit confusing, but the test below uses assert not equal, which maybe redundant in the presence of assert_eq below that.

Yes it is redundant, it is mostly documentation (at some point I was not remembering why we can recurse on popping the 0xff).

ordian · 2020-03-27T06:38:23Z

Writing this PR description, I realize that we could also use an operator that delete a range (start inclusive, end exclusive), which could allow more use cases but is not really needed.

Which use-cases of generic range deletion can you think of?
Unless really needed, I'd like to keep it simple for now.

Co-Authored-By: David <dvdplm@gmail.com>

cheme · 2020-03-27T08:28:47Z

Which use-cases of generic range deletion can you think of?

I do not have any, I was just considering that having range in operation could make things more extendable (it is doable with all three implementations, and probably most db key value impl will get more efficient implementation for range (not radix tree)).

So not 'needed'.

* master: kvdb-rocksdb: optimize and rename iter_from_prefix (#365) bump parity-util-mem (#376) parity-util-mem: fix for windows (#375) keccak-hash: fix bench and add one for range (#372) [parity-crypto] Release 0.6.1 (#373) keccak-hash: bump version to 0.5.1 (#371) keccak-hash: add keccak256_range and keccak512_range functions (#370) Allow pubkey recovery for all-zero messages (#369) Delete by prefix operator in kvdb (#360) kvdb: no overlay (#313) Ban duplicates of parity-uil-mem from being linked into the same program (#363) Use correct license ID (#362) Memtest example for Rocksdb (#349) Prep for release (#361) parity-util-mem: prepare release for 0.5.2 (#359) travis: test parity-util-mem on android (#358) parity-util-mem: update mimalloc feature (#352) kvdb: remove parity-bytes dependency (#351) parity-util-mem: use malloc for usable_size on android (#355) CI: troubleshoot macOS build (#356)

* master: (56 commits) primitive-types: add no_std support for serde feature (#385) Add Rocksdb Secondary Instance Api (#384) kvdb-rocksdb: update rocksdb to 0.14 (#379) prepare releases for a few crates (#382) uint: fix UB in uint::from_big_endian (#381) Fix limit prefix delete case (#368) Add arbitrary trait implementation (#378) kvdb-rocksdb: optimize and rename iter_from_prefix (#365) bump parity-util-mem (#376) parity-util-mem: fix for windows (#375) keccak-hash: fix bench and add one for range (#372) [parity-crypto] Release 0.6.1 (#373) keccak-hash: bump version to 0.5.1 (#371) keccak-hash: add keccak256_range and keccak512_range functions (#370) Allow pubkey recovery for all-zero messages (#369) Delete by prefix operator in kvdb (#360) kvdb: no overlay (#313) Ban duplicates of parity-uil-mem from being linked into the same program (#363) Use correct license ID (#362) Memtest example for Rocksdb (#349) ...

bkchr and others added 11 commits December 4, 2018 16:18

Switch from parity-rocksdb to upstream rust-rocksdb

616b401

Update to latest rocksdb

b0317f6

Delete prefix as range exept when start is full of 255 (corner case).

b6cf86b

Merge commit 'b0317f649ab2c665b7987b8475878fc4d2e1f81d' into upgraded…

9ce83b0

…_del_range Block on liftime for column familly.

forcing ignore lifetime (not more unsafe than before, columnfamily is

6e4f390

probably not an issue, dbiterator is one as it will fail on close db).

Merge branch 'master' into upgraded_del_range, largely update previous

168b103

patch.

remove temporarilly path deps for easier patch usage.

6495423

being a bit more exhaustive

dac8aa1

Tests for delete prefix, fix rocksdb full column delete.

a3a2a54

revert util-mem

a2b481c

update versionning.

81e460e

cheme added the A0-pleasereview label Mar 13, 2020

cheme mentioned this pull request Mar 17, 2020

bulk child trie deletion paritytech/substrate#5280

Closed

Merge branch 'master' into upgraded_del_range_pr

2018880

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb-rocksdb/src/lib.rs Outdated Show resolved Hide resolved

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb-memorydb/src/lib.rs Outdated Show resolved Hide resolved

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb-shared-tests/src/lib.rs Outdated Show resolved Hide resolved

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb/src/lib.rs Outdated Show resolved Hide resolved

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb/src/lib.rs Outdated Show resolved Hide resolved

better end prefix from review

27a3828

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb/src/lib.rs Outdated Show resolved Hide resolved

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb/src/lib.rs Outdated Show resolved Hide resolved

cheme and others added 4 commits March 26, 2020 17:04

revert test error check

1f60726

Update kvdb-memorydb/src/lib.rs

2eb1fd8

Co-Authored-By: Andronik Ordian <write@reusable.software>

Update kvdb-shared-tests/src/lib.rs

ca2269f

Co-Authored-By: Andronik Ordian <write@reusable.software>

Update kvdb/src/lib.rs

fcc4e58

Co-Authored-By: Andronik Ordian <write@reusable.software>

cheme and others added 4 commits March 26, 2020 17:25

Update kvdb/src/lib.rs

34b1e0b

Co-Authored-By: Andronik Ordian <write@reusable.software>

applying suggestions, and remove delete by prefix method.

5a892f8

Merge branch 'upgraded_del_range_pr' of github.com:cheme/parity-commo…

1cedb14

…n into upgraded_del_range_pr

io stats num column

da49663

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb-rocksdb/src/lib.rs Outdated Show resolved Hide resolved

ordian reviewed Mar 26, 2020

View reviewed changes

kvdb-rocksdb/src/lib.rs Outdated Show resolved Hide resolved

cheme added 2 commits March 26, 2020 18:28

end_prefix test

05d9469

format

20bac79

ordian reviewed Mar 26, 2020

View reviewed changes

ordian approved these changes Mar 26, 2020

View reviewed changes

ordian added A8-looksgood and removed A0-pleasereview labels Mar 26, 2020

Redundant delete.

815d6ca

dvdplm approved these changes Mar 26, 2020

View reviewed changes

cheme and others added 3 commits March 27, 2020 09:10

Update kvdb/src/lib.rs

1564f5f

Co-Authored-By: David <dvdplm@gmail.com>

Update kvdb/src/lib.rs

904ebae

Co-Authored-By: David <dvdplm@gmail.com>

Documentation fix and additional test case in end_prefix_test

2456135

cheme added 2 commits March 27, 2020 09:34

Doc.

1f18d0f

doc

9e33a73

ordian merged commit dd89c9a into paritytech:master Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete by prefix operator in kvdb #360

Delete by prefix operator in kvdb #360

cheme commented Mar 13, 2020 •

edited

Loading

ordian commented Mar 26, 2020

ordian Mar 26, 2020

ordian Mar 26, 2020

ordian Mar 26, 2020

cheme Mar 26, 2020

ordian Mar 26, 2020

cheme Mar 26, 2020

ordian Mar 26, 2020

dvdplm Mar 26, 2020

ordian Mar 26, 2020

cheme Mar 26, 2020

ordian Mar 26, 2020

ordian commented Mar 26, 2020

ordian Mar 26, 2020

ordian left a comment

dvdplm left a comment

dvdplm Mar 26, 2020

dvdplm Mar 26, 2020

dvdplm Mar 26, 2020

dvdplm Mar 26, 2020

ordian Mar 26, 2020

cheme Mar 27, 2020

ordian commented Mar 27, 2020

cheme commented Mar 27, 2020

	batch.delete_range_cf(cf, &prefix[..], &end_range[..]).map_err(other_io_err)?;
	batch.delete_range_cf(cf, &[], &end_range[..]).map_err(other_io_err)?;

	// delete every values in the column
	// deletes all values in the column

Delete by prefix operator in kvdb #360

Delete by prefix operator in kvdb #360

Conversation

cheme commented Mar 13, 2020 • edited Loading

ordian commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian commented Mar 26, 2020

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian commented Mar 27, 2020

cheme commented Mar 27, 2020

cheme commented Mar 13, 2020 •

edited

Loading