Define GCDatastore/CheckedDatastore interfaces

[magik6k]

Both are needed for some badger functionality and may be useful for other future datastores.

CheckedDatastore:

• repo fsck doesn't remove badger lockfile kubo#4301

GCDatastore:

• Garbage collection doesn't free up space when using badger kubo#4300
• GC is slower with badger (with syncwrites enabled) kubo#4298 (comment)

[Stebalien]

Maybe Scrub()? We also want to support datastores that can do error correction (where an error is returned iff the datastore is unrecoverable).

An open question is how do deal with partial failures. It would nice to be able to recover as much data as possible.

[magik6k]

Check would be invoked by ipfs repo fsck, which for now only removes lock files.

Scrub() does make sense (it could be wired up to ipfs repo verify, to keep repo fsck fast). Does putting this into separate interface SGTY (keeping check for stuff like lock-files)?

[Stebalien]

Got it. Yes, having a separate interface sounds good.

Just to make sure, I assume Check() will find and fix any errors related to daemon crashes (i.e., it doesn't just blindly remove the lock file)?

---

Side note: we should get rid of ipfs repo fsck; IPFS should do any recovery necessary on startup automatically.

[whyrusleeping]

Be careful, adding methods to these interfaces generally means every wrapper datastore has to implement them as well.

[whyrusleeping]

We will need to find new interesting ways of abusing the type system

[magik6k]

That's why I made those into separate interfaces - it can only be implemented where it's needed

[whyrusleeping]

@magik6k But heres the problem, the default construction of the go-ipfs badgerds is a measure datastore wrapping the badger datastore. So the Datastore we have in the repo is of type measureds.Datastore. In order to call the GarbageCollect method on that, we need to either have that method implemented on the measureds, or be able to access its child directly.

This gets even more complicated in cases like the default flatfs datastore setup, with multiple mounts and measure datastore wrappings.

[magik6k]

I know that, I started wiring it up to these DS-es locally, but I want interfaces defined first

[Stebalien]

In order to call the GarbageCollect method on that, we need to either have that method implemented on the measureds, or be able to access its child directly.

We can have things like measureds always implement these interfaces and then try to call the appropriate methods on their wrapped types. We can even provide helper functions that do this:

func Scrub(ds *Datastore) error {
    if s, ok := ds.(*ScrubbedDatastore); ok {
        return s.Scrub()
    }
    return nil // error?
}

[Kubuxu]

If we go this path, we could implement a WrapperDatastore struct that would do all the forwarding (implementing all the interfaces).
Then Wrapping datastores can override selected functions. Example:

type MeasureDatastore  struct {
    WrappingDatastore
    
    a int
}

func (md *MeastureDatastore) Get.... {
    md++
    return md.WrappingDatastore.Get(k)
}

I am not sure it is the best path to move forward. Something better might be capability system. Just putting it out here.

[Kubuxu]

As I said, I am not 100% about the solution.

(cleaning out my Review queue).

[whyrusleeping]

@Stebalien

We can have things like measureds always implement these interfaces and then try to call the appropriate methods on their wrapped types.

I'm not saying this is wrong, but it generally just leads to "everybody implements this interface always"

[Stebalien]

@whyrusleeping

It's more that wrappers that may potentially wrap a datastore implementing that interface should implement the interface. That's why I suggested providing convenience functions for using these interfaces when available.

[whyrusleeping]

It might make sense to think about a capability system as @Kubuxu mentions, but these interfaces need work soon-ish anyways, so i'm fine deferring some of that until later.

[Stebalien]

This system should be forwards compatible so LGTM.

[Stebalien]

A bit after the fact, but shouldn't these take the lock?

[magik6k]

Yeah, they should. Will push a fix in a bit