fix(storage): do not fetch all sst meta when create iterator #9517

Little-Wallace · 2023-04-27T11:51:23Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Checklist For Contributors

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

Documentation

My PR DOES NOT contain user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

Incorrect.

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

Signed-off-by: Little-Wallace <[email protected]>

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

seems unnecessary

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

incorrect

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

Perhaps we can improve.

soundOfDestiny · 2023-04-28T07:40:45Z

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

+        self.current.as_ref().unwrap().current_epoch()
+    }
+
+    fn next(&mut self) -> Self::NextFuture<'_> {


In current implementation, after a next call, next_extended_user_key may stay invariant. Is this bad-taste?

But everywhere we call next_extended_user_key would always collect all keys whose user-key equals

But everywhere we call next_extended_user_key would always collect all keys whose user-key equals

In MergeIterator, everywhere we call next_extended_user_key would always collect all keys IN OTHER ITERATORS whose user-key equals, but except the iterator itself

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

I'm ok if self.current.take() is in seek, but it should then be in rewind as well.

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

soundOfDestiny

Incorrect.

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

duplicate seek

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

typo

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

soundOfDestiny

as in comment

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

because we have checked self.sstables[self.idx + 1].range_tombstone_count > 0 before

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

soundOfDestiny

incorrect

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

the semantic of next_range_epoch() does not match

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

incorrect

soundOfDestiny · 2023-05-05T13:44:59Z

src/storage/src/hummock/iterator/concat_inner.rs


                        ord == Less || ord == Equal
                    }
                    DirectionEnum::Backward => {
-                        let ord = FullKey::decode(table.largest_key()).cmp(&key);
-
+                        let ord = FullKey::decode(largest_key(table)).cmp(&key);


If right key range is excluded, we will skip the key itself unexpectedly.

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny

LGTM

Li0k

Thanks for the effort

Li0k · 2023-05-06T06:28:14Z

src/storage/src/hummock/store/version.rs

-                    // We would fill block to high priority cache for level-0
-                    self.sstable_store
-                        .sstable_syncable(sstable_info, &local_stats)
+                if sstables.len() > 1 {


IMO， we need some doc to explain that we remove hit_sstable_bloom_filter when non_overlapping sstable.size() > 1. It may result in better code readability

IMO， we need some doc to explain that we remove hit_sstable_bloom_filter when non_overlapping sstable.size() > 1. It may result in better code readability

+1

Li0k · 2023-05-06T06:35:43Z

src/storage/src/hummock/iterator/concat_inner.rs

-                        ord == Greater || ord == Equal
+                        let ord = FullKey::decode(largest_key(table)).cmp(&key);
+                        ord == Greater
+                            || (ord == Equal && !table.key_range.as_ref().unwrap().right_exclusive)


This logic is easy to use wrongly, encapsulate it into a function or add more comments to explain it?

hzxa21 · 2023-05-06T08:29:53Z

src/storage/src/hummock/store/version.rs

-                    // We would fill block to high priority cache for level-0
-                    self.sstable_store
-                        .sstable_syncable(sstable_info, &local_stats)
+                if sstables.len() > 1 {


IMO， we need some doc to explain that we remove hit_sstable_bloom_filter when non_overlapping sstable.size() > 1. It may result in better code readability

+1

hzxa21 · 2023-05-06T08:49:47Z

src/storage/src/hummock/iterator/concat_inner.rs

-                .prefetch(&self.sstable_store, &mut self.stats)
+            let table = self
+                .sstable_store
+                .sstable(&self.tables[idx], &mut self.stats)


random thought: we can prefetch the next sstable meta similar to the block prefetch in sstable iterator. not sure how much improvement we can get though

random thought: we can prefetch the next sstable meta similar to the block prefetch in sstable iterator. not sure how much improvement we can get though

We can get more memory :rolling_on_the_floor_laughing:

random thought: we can prefetch the next sstable meta similar to the block prefetch in sstable iterator. not sure how much improvement we can get though

We can get more memory :rolling_on_the_floor_laughing:

True. But it looks acceptable because the memory usage is proportional to number of levels, not number of SSTs, and we have very few levels.

But after we support meta-cache refill after compaction, the meta-cache miss rate is very low.
So it is not necessary to prefetch the next TableHolder for delete-range (

support delete range with concat

66018c5

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed Apr 27, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Outdated Show resolved Hide resolved

refactor iterator

6131819

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed Apr 27, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Outdated Show resolved Hide resolved

fix warn

0d52c65

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed Apr 28, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Show resolved Hide resolved

remove unnecessary code

815481a

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed Apr 28, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Outdated Show resolved Hide resolved

fix valid

65c7603

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed Apr 28, 2023

View reviewed changes

TennyZhuang changed the title ~~fix(storage:): do not fetch all sst meta when create iterator~~ fix(storage): do not fetch all sst meta when create iterator Apr 28, 2023

Little-Wallace added 3 commits May 4, 2023 11:28

Merge branch 'main' into fix-read-much-meta

11b1280

fix test

5ffc25d

Signed-off-by: Little-Wallace <[email protected]>

refactor

61331e4

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed May 5, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Outdated Show resolved Hide resolved

soundOfDestiny mentioned this pull request May 5, 2023

bug: limit 1 batch query caused OOM #8721

Closed

soundOfDestiny reviewed May 5, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Outdated Show resolved Hide resolved

fix equal case

a238b97

Signed-off-by: Little-Wallace <[email protected]>

Little-Wallace force-pushed the fix-read-much-meta branch from 4e52aa1 to a238b97 Compare May 5, 2023 09:21

soundOfDestiny reviewed May 5, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Show resolved Hide resolved

Little-Wallace added 3 commits May 5, 2023 18:37

fix format

442ae11

Signed-off-by: Little-Wallace <[email protected]>

Merge branch 'main' into fix-read-much-meta

4da1a3d

remove useless take

862a7fa

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed May 5, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Show resolved Hide resolved

soundOfDestiny reviewed May 5, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Outdated Show resolved Hide resolved

fix seek bug

cc6d70c

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed May 5, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Outdated Show resolved Hide resolved

soundOfDestiny reviewed May 5, 2023

View reviewed changes

assert valid

8b9c8a7

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed May 5, 2023

View reviewed changes

src/storage/src/hummock/iterator/concat_delete_range_iterator.rs Outdated Show resolved Hide resolved

Little-Wallace added 3 commits May 5, 2023 21:20

remove trait next range epoch

3c277fa

Signed-off-by: Little-Wallace <[email protected]>

check is last range

6115d27

Signed-off-by: Little-Wallace <[email protected]>

fix format

62eaec9

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny reviewed May 5, 2023

View reviewed changes

check right_exclusive

c289bbc

Signed-off-by: Little-Wallace <[email protected]>

soundOfDestiny approved these changes May 5, 2023

View reviewed changes

soundOfDestiny enabled auto-merge May 5, 2023 15:19

soundOfDestiny added this pull request to the merge queue May 5, 2023

Merged via the queue into risingwavelabs:main with commit 844175e May 5, 2023

Li0k reviewed May 6, 2023

View reviewed changes

hzxa21 reviewed May 6, 2023

View reviewed changes

soundOfDestiny mentioned this pull request May 9, 2023

feat(concat delete iterator): prefetch for concat delete range iterator #9693

Closed

4 tasks

soundOfDestiny mentioned this pull request May 17, 2023

feat(sst meta): rollback fetch meta strategy #8917

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(storage): do not fetch all sst meta when create iterator #9517

fix(storage): do not fetch all sst meta when create iterator #9517

Little-Wallace commented Apr 27, 2023 •

edited

Loading

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny Apr 28, 2023

Little-Wallace May 4, 2023

soundOfDestiny May 4, 2023 •

edited

Loading

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny left a comment

soundOfDestiny May 5, 2023

soundOfDestiny left a comment

Li0k left a comment

Li0k May 6, 2023

hzxa21 May 6, 2023

Li0k May 6, 2023

hzxa21 May 6, 2023

hzxa21 May 6, 2023

soundOfDestiny May 6, 2023

hzxa21 May 7, 2023

Little-Wallace May 10, 2023

fix(storage): do not fetch all sst meta when create iterator #9517

fix(storage): do not fetch all sst meta when create iterator #9517

Conversation

Little-Wallace commented Apr 27, 2023 • edited Loading

What's changed and what's your intention?

Checklist For Contributors

Checklist For Reviewers

Documentation

Types of user-facing changes

Release note

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soundOfDestiny May 4, 2023 • edited Loading

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soundOfDestiny left a comment

Choose a reason for hiding this comment

Li0k 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Little-Wallace commented Apr 27, 2023 •

edited

Loading

soundOfDestiny May 4, 2023 •

edited

Loading