DBCol should implement its own GC method

[Kouprin]

We missed at least two columns: ColTransactionResult, ColBlockMerkleTree. Each DBCol must implement its own GC method to avoid forgetting about GCing data.

Discussed with @frol, it seems we need to define Trait DBCol with method gc(store_update: StoreUpdate, key: &[u8]) and implement it for each column where execute store_update.delete(column, key).

[bowenwang1996]

FYI for ColBlockMerkleTree it is intentional that we don't garbage collect them as they are needed for header verification.

[Kouprin]

FYI for ColBlockMerkleTree it is intentional that we don't garbage collect them as they are needed for header verification.

It should be explicit.

[SkidanovAlex]

Is my understanding correct that the sheer existence of gc method doesn't guarantee that the method is called?
How does the trait help ensure that the collection is actually garbage collected somewhere?

[bowenwang1996]

@SkidanovAlex my understanding is that we iterate through all columns and call the method to do gc so that we don't forget some columns.

[SkidanovAlex]

@bowenwang1996 that doesn't work. Order in which columns are GCed is important, and sometimes to GC one column we need to iterate another. E.g. to GC state, we can't just iterate over the state collections.

[Kouprin]

We can count a number of calls and then check it in Store Validator.

[ailisp]

Does a simple attribute-like trait work, something like

impl GC for Col1{
gc(&self) {
return  GCCol::GC; // or GCCol::Keep
}
}

fn delete(col, key) {
  if col.gc() == GC::KEEP { return; }
  // original delete(col, key) here
}

or each column gc trait includes column specific logic to determine whether key should be GCed and we're reorganizing gc logics into column gc trait?

[Kouprin]

@frol WDYT? ^^^
Usually, removing from any column look like clearing the cache + removing a single line from the column, i.e.:

            store_update.delete(ColChunkPerHeightShard, {key=} &get_height_shard_id(height, shard_id));
            self.chain_store
                .chunk_hash_per_height_shard
                .cache_remove(&get_height_shard_id(height, shard_id));

Ideally, it would be awesome to call something like ColChunkPerHeightShard.gc(store_update, key, cache) and put GC implementation inside the gc() method for each column, however I'm not sure does it possible in Rust.

[bowenwang1996]

@Kouprin why is it not possible?

[Kouprin]

@bowenwang1996 I didn't say it's not possible. My doubts are about mutating data a lot which may be complicated to handle in Rust.

[ailisp]

@frol WDYT? ^^^
Usually, removing from any column look like clearing the cache + removing a single line from the column, i.e.:

            store_update.delete(ColChunkPerHeightShard, {key=} &get_height_shard_id(height, shard_id));
            self.chain_store
                .chunk_hash_per_height_shard
                .cache_remove(&get_height_shard_id(height, shard_id));

Ideally, it would be awesome to call something like ColChunkPerHeightShard.gc(store_update, key, cache) and put GC implementation inside the gc() method for each column, however I'm not sure does it possible in Rust.

I see, yes i found this pattern a lot and in the internal notion gc spec I did put this together in the pseudo python code :)

So your proposal looks like:
for every column needs gc, because you can't calculated "chunk_hash_per_height_shard" and call from "ColChunkPerHeightShard" :

fn gc(store_update, key, cache) {
            store_update.delete(ColChunkPerHeightShard, {key=} &get_height_shard_id(height, shard_id));
            self.chain_store
                .chunk_hash_per_height_shard
                .cache_remove(&get_height_shard_id(height, shard_id));
}

and for columns want to skip gc it's a noop or conditional noop in gc?

[Kouprin]

Before:

            store_update.delete(ColChunkPerHeightShard, &get_height_shard_id(height, shard_id));
            self.chain_store
                .chunk_hash_per_height_shard
                .cache_remove(&get_height_shard_id(height, shard_id));

After:

ColChunkPerHeightShard.gc(store_update,
                          key: &get_height_shard_id(height, shard_id),
                          cache: self.chain_store.chunk_hash_per_height_shard);

ColChunkPerHeightShard.gc() inside looks like:

fn gc(store_update, key, cache: Option<T>) {
            store_update.delete(ColChunkPerHeightShard, key);
            cache.unwrap().cache_remove(key);
            self.gc_calls += 1;
}

For noop columns they still should be executed somewhere in GC to make sure we don't skip any columns:

fn gc(_store_update, _key, _cache) {
            self.gc_calls += 1;
}

Then I think we'll decide what to do with gc_calls. Now I can only suggest to put all gc_calls into separate column (column_name, u64 - number of gc_calls) and then update Store Validator to check that all values of gc_calls must grow.

[bowenwang1996]

Now I can only suggest to put all gc_calls into separate column

Why do you need to do that? Couldn't you check it right after the gc is done in the current iteration?

[ailisp]

Why do you need to do that? Couldn't you check it right after the gc is done in the current iteration?

@Kouprin sent me offline, it's checking in store-validator, my understanding is checking offline for performance reason?

[Kouprin]

Why do you need to do that? Couldn't you check it right after the gc is done in the current iteration?

No guarantees that all columns must be cleared at each iteration. For example, Chunks are cleared after Blocks. (#2716)

[ailisp]

Worked on this today. It seems rust doesn't let me

impl trait GCCol for DBCol::ColBlockMisc

error: expected type, found variant DBCol::ColBlockMisc not a type
Looks i can only impl trait for the entire enum DBCol. But in that case do we still need a trait? Seems just

impl DBCol {
    fn gc(...) {
        match(&self) {
        DBCol::ColBlockMisc => { gc_for_db_col_block_misc },
        ... => ...
        }
    }
}

would work. and match has better property that enforce you fill cases for all column.

[frol]

@ailisp I thought that we had independent constants, that is why I suggested using a trait. Enum is even better exactly for the enforcement rule of covering all the cases.