feat(storage): implement its own GC method

[ailisp]

implement #2732
remove cache and update gc count. because gc count is in batch update, cache it in ChainStoreUpdate to implement semantic: column.gc_count += 1.

Test Plan

Test it still pass existing gc test and gc count of each column correctly update when remove_old_data
This PR is also introduce the first storage migration, tested by near init, near run with master binary, then use this branch binary to run, observe not crash and new column created.

[gitpod-io]

[Kouprin]

This is awesome. I approve the structure.
@frol any your suggestions at this point?

[frol]

I like it!

[ailisp]

Call gc method on all gcing now. test failed due to:

debug_assert!(
            self.transaction.ops.len()
                == self
                    .transaction
                    .ops
                    .iter()
                    .map(|op| match op {
                        DBOp::Insert { col, key, .. } => {
                            (*col as u8, key)
                        }
                        DBOp::Delete { col, key } => {
                            (*col as u8, key)
                        }
                    })
                    .collect::<std::collections::HashSet<_>>()
                    .len(),
            "Transaction overwrites itself: {:?}",
            self
        );

Trying to figure out a way to write "update times of gc on col during this batch of update" only once, to pass this assert

[ailisp]

@Kouprin Tests failed quite weirdly. I debug the basic test clear_old_data, was failed at assert block 1 not removed (therefore probably everything is not removed). I added some debug print to trace chain_store_update, store_update merge and commit, and how get_block behaves. It turns out to be block is removed in storage, but still exists in sizedcache (know this by tracing read_with_cache, and if comment read_with_cache "read_without_cache" all gc tests passing),

And right after self.chain_store.blocks.cache_remove(key) do a print self.chain_store.blocks.cache_get(key) shows None so it's indeed removed. Then it's added back by self.chain_store.blocks.cache_set in ChainStoreUpdate.commit, and cause the fail. I still can't figure out why this happen with ChainStoreUpdate.gc_col approach, while not happen in the DBCol.gc(store_update, key, cache) approach. Will continue debug tomorrow. (Appreciate if you encounter similar issue before and has a solution!)

[Kouprin]

@Kouprin Tests failed quite weirdly. I debug the basic test clear_old_data, was failed at assert block 1 not removed (therefore probably everything is not removed). I added some debug print to trace chain_store_update, store_update merge and commit, and how get_block behaves. It turns out to be block is removed in storage, but still exists in sizedcache (know this by tracing read_with_cache, and if comment read_with_cache "read_without_cache" all gc tests passing),

And right after self.chain_store.blocks.cache_remove(key) do a print self.chain_store.blocks.cache_get(key) shows None so it's indeed removed. Then it's added back by self.chain_store.blocks.cache_set in ChainStoreUpdate.commit, and cause the fail. I still can't figure out why this happen with ChainStoreUpdate.gc_col approach, while not happen in the DBCol.gc(store_update, key, cache) approach. Will continue debug tomorrow. (Appreciate if you encounter similar issue before and has a solution!)

The root of evil is in calling self.gc_col(ColBlockPerHeight, &block_hash_vec)? after self.gc_col(ColBlock, &block_hash_vec)?. This is definitely a bug because inside gc_col() for ColBlockPerHeight you're trying to call get_block() which is absent.

Hope it helps.

[bowenwang1996]

A couple things:

• Can you run nightly tests to make sure that things are not trivially broken?
• There seems to be no check on whether all columns are gc'ed. For example, it seems that we still don't gc ColTransactionResult

[ailisp]

Can you run nightly tests to make sure that things are not trivially broken?

Good point, running.

There seems to be no check on whether all columns are gc'ed. For example, it seems that we still don't gc ColTransactionResult

I think this was supposed to check externally in store validator, based on discussion in #2732 ?

[bowenwang1996]

@Kouprin looks like there are some columns that are not garbage collected. Can you fix it?

[Kouprin]

@Kouprin looks like there are some columns that are not garbage collected. Can you fix it?

#2860

[Kouprin]

Overall, this is the great job. We need to do the following:

• apply some comments
• get @frol's approve
• get @mikhailOK's approve
• run nightly and make sure it works (https://nightly.neartest.com/s3/db-col-gc-method_8776ca8e324283a1794a17def57c9da40697c1d3_20200617_074640)

[bowenwang1996]

@mikhailOK please review this PR

[frol]

Overall, it is a great improvement! I agree with @Kouprin's suggestions, and here are some comments from me.

We still don't have GC enforcement (when someone adds a new column, we currently can only enforce the GC implementation, but then we should call the method for the column, and that can still be missed), but I don't have a suggestion on how to improve this.

[ailisp]

We still don't have GC enforcement (when someone adds a new column, we currently can only enforce the GC implementation, but then we should call the method for the column, and that can still be missed), but I don't have a suggestion on how to improve this.

This answer from @Kouprin : this'll be checked in StorageValidator, any col that in SHOULD_COL_GC should have non 0 gc count.

[bowenwang1996]

What's the purpose of this per column gc count?

[ailisp]

What's the purpose of this per column gc count?

it's for being checked in storevalidator so that all columns should be GCed should > 0 in per column gc count. This would prevent a column is marked as "should gc" but forgot gc it.

[bowenwang1996]

it's for being checked in storevalidator so that all columns should be GCed should > 0 in per column gc count. This would prevent a column is marked as "should gc" but forgot gc it.

What's the difference between 1 and 10 then? Is the count itself actually used?

[ailisp]

What's the difference between 1 and 10 then? Is the count itself actually used?

From a use case mentioned from @Kouprin somewhere in #2732: Would be useful in test, such as if ColBlock has been gc count over 100, then some other columns should also be at least 1.
Also I think it's potentially helpful for us to understand how many GC operations happen in a production setting and future optimization