Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ADR 040: New DB interface #9573

Merged
merged 10 commits into from
Aug 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#9540](https://github.com/cosmos/cosmos-sdk/pull/9540) Add output flag for query txs command.
* (errors) [\#8845](https://github.com/cosmos/cosmos-sdk/pull/8845) Add `Error.Wrap` handy method
* [\#8518](https://github.com/cosmos/cosmos-sdk/pull/8518) Help users of multisig wallets debug signature issues.
* [\#9573](https://github.com/cosmos/cosmos-sdk/pull/9573) ADR 040 implementation: New DB interface


### Client Breaking Changes
Expand Down
7 changes: 7 additions & 0 deletions db/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
go 1.15

module github.com/cosmos/cosmos-sdk/db

require (
github.com/stretchr/testify v1.7.0
)
11 changes: 11 additions & 0 deletions db/go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
195 changes: 195 additions & 0 deletions db/types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
package db

import "errors"

var (
// ErrBatchClosed is returned when a closed or written batch is used.
ErrBatchClosed = errors.New("batch has been written or closed")

// ErrKeyEmpty is returned when attempting to use an empty or nil key.
ErrKeyEmpty = errors.New("key cannot be empty")

// ErrValueNil is returned when attempting to set a nil value.
ErrValueNil = errors.New("value cannot be nil")

// ErrVersionDoesNotExist is returned when a DB version does not exist.
ErrVersionDoesNotExist = errors.New("version does not exist")

// ErrOpenTransactions is returned when open transactions exist which must
// be discarded/committed before an operation can complete.
ErrOpenTransactions = errors.New("open transactions exist")

// ErrReadOnly is returned when a write operation is attempted on a read-only transaction.
ErrReadOnly = errors.New("cannot modify read-only transaction")

// ErrInvalidVersion is returned when an operation attempts to use an invalid version ID.
ErrInvalidVersion = errors.New("invalid version")
)

// DBConnection represents a connection to a versioned database.
// Records are accessed via transaction objects, and must be safe for concurrent creation
// and read and write access.
// Past versions are only accessible read-only.
type DBConnection interface {
// Opens a read-only transaction at the current working version.
Reader() DBReader

// Opens a read-only transaction at a specified version.
// Returns ErrVersionDoesNotExist for invalid versions.
ReaderAt(uint64) (DBReader, error)

// Opens a read-write transaction at the current version.
ReadWriter() DBReadWriter

// Opens a write-only transaction at the current version.
Writer() DBWriter

// Returns all saved versions
Versions() (VersionSet, error)

// Saves the current contents of the database and returns the next version ID, which will be
// `Versions().Last()+1`.
// Returns an error if any open DBWriter transactions exist.
// TODO: rename to something more descriptive?
SaveNextVersion() (uint64, error)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved

// Attempts to save database at a specific version ID, which must be greater than or equal to
// what would be returned by `SaveNextVersion`.
// Returns an error if any open DBWriter transactions exist.
SaveVersion(uint64) error
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be needed for ABCI state sync, since snapshots need to be imported at a specific height.


// Deletes a saved version. Returns ErrVersionDoesNotExist for invalid versions.
DeleteVersion(uint64) error

// Close closes the database connection.
Close() error
}

// DBReader is a read-only transaction interface. It is safe for concurrent access.
// Callers must call Discard when done with the transaction.
//
// Keys cannot be nil or empty, while values cannot be nil. Keys and values should be considered
// read-only, both when returned and when given, and must be copied before they are modified.
type DBReader interface {
roysc marked this conversation as resolved.
Show resolved Hide resolved
// Get fetches the value of the given key, or nil if it does not exist.
// CONTRACT: key, value readonly []byte
Get([]byte) ([]byte, error)

// Has checks if a key exists.
// CONTRACT: key, value readonly []byte
Has(key []byte) (bool, error)

// Iterator returns an iterator over a domain of keys, in ascending order. The caller must call
// Close when done. End is exclusive, and start must be less than end. A nil start iterates
// from the first key, and a nil end iterates to the last key (inclusive). Empty keys are not
// valid.
// CONTRACT: No writes may happen within a domain while an iterator exists over it.
// CONTRACT: start, end readonly []byte
Iterator(start, end []byte) (Iterator, error)

// ReverseIterator returns an iterator over a domain of keys, in descending order. The caller
// must call Close when done. End is exclusive, and start must be less than end. A nil end
// iterates from the last key (inclusive), and a nil start iterates to the first key (inclusive).
// Empty keys are not valid.
// CONTRACT: No writes may happen within a domain while an iterator exists over it.
// CONTRACT: start, end readonly []byte
// TODO: replace with an extra argument to Iterator()?
ReverseIterator(start, end []byte) (Iterator, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my inclination is that you'd specify iteration direction as an option to the method that produes the iterator and not require duplicate methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a trivial change. Any other opinions on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference, but tend to have the opposite inclination 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeping the interface minimal is a desired design. However it shouldn't on a cost of overloading the functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't have a strong opinion here, but we use different methods in the SDK currently. Being as we're only adding one parameter, I don't see why not.


// Discards the transaction, invalidating any future operations on it.
Discard()
roysc marked this conversation as resolved.
Show resolved Hide resolved
}

// DBWriter is a write-only transaction interface.
// It is safe for concurrent writes, following an optimistic (OCC) strategy, detecting any write
// conflicts and returning an error on commit, rather than locking the DB.
// This can be used to wrap a write-optimized batch object if provided by the backend implementation.
type DBWriter interface {
roysc marked this conversation as resolved.
Show resolved Hide resolved
// Set sets the value for the given key, replacing it if it already exists.
// CONTRACT: key, value readonly []byte
Set([]byte, []byte) error

// Delete deletes the key, or does nothing if the key does not exist.
// CONTRACT: key readonly []byte
Delete([]byte) error

// Flushes pending writes and discards the transaction.
Commit() error
roysc marked this conversation as resolved.
Show resolved Hide resolved

// Discards the transaction, invalidating any future operations on it.
Discard()
}

// DBReadWriter is a transaction interface that allows both reading and writing.
type DBReadWriter interface {
roysc marked this conversation as resolved.
Show resolved Hide resolved
DBReader
DBWriter
}

// Iterator represents an iterator over a domain of keys. Callers must call Close when done.
// No writes can happen to a domain while there exists an iterator over it, some backends may take
// out database locks to ensure this will not happen.
//
// Callers must make sure the iterator is valid before calling any methods on it, otherwise
// these methods will panic. This is in part caused by most backend databases using this convention.
//
// As with DBReader, keys and values should be considered read-only, and must be copied before they are
// modified.
//
// Typical usage:
//
// var itr Iterator = ...
// defer itr.Close()
//
// for ; itr.Valid(); itr.Next() {
// k, v := itr.Key(); itr.Value()
// ...
// }
// if err := itr.Error(); err != nil {
// ...
// }
type Iterator interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written/used iterators with the pattern:

var iter Iterator

for iter.Next() {

}

if err := iter.Err(); err != nil {
    return err
}
if err := iter.Close(); err != nil {
    return err 
}

Which collapses Valid/Next,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, the only odd thing about this is it requires the iterator to start in an invalid state

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 - had similar opinion about it above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking a bit more on this, it may not be ideal as it violates the single-responsibility principle. The interface is smaller but you can't check the iterator's state without mutating it.

That said, I have pushed the change to the interface.

// Domain returns the start (inclusive) and end (exclusive) limits of the iterator.
// CONTRACT: start, end readonly []byte
Domain() (start []byte, end []byte)

// Next moves the iterator to the next key in the database, as defined by order of iteration;
// returns whether the iterator is valid. Once invalid, it remains invalid forever.
Next() bool

// Key returns the key at the current position. Panics if the iterator is invalid.
// CONTRACT: key readonly []byte
Key() (key []byte)

// Value returns the value at the current position. Panics if the iterator is invalid.
// CONTRACT: value readonly []byte
Value() (value []byte)

// Error returns the last error encountered by the iterator, if any.
Error() error

// Close closes the iterator, relasing any allocated resources.
Close() error
}

// VersionSet specifies a set of existing versions
type VersionSet interface {
// Last returns the most recent saved version, or 0 if none.
Last() uint64
roysc marked this conversation as resolved.
Show resolved Hide resolved
// Count returns the number of saved versions.
Count() int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods should probably return an error, for forward compatibility with other potential implementations. E.g. if we change All() to using an iterator, the iterator may hit errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true for all the interfaces, not just this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to push back a bit on this. The VersionSet should ideally represent the version history of a DB at a specific point in time, and should always be in a valid state, so Count and All should not fail. The iterator to replace All will have a Valid method, but should also not encounter an error.

If a DB can't return a valid version history, it should return an error from the Versions method.

Also, TMU this interface won't be exposed to SDK users. If the behavior changes, adding error return values should be a non-breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you cashing versions when calling Versions()? If not, then Count can error if a connection to the DB will get broken in the meantime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, versions are cached so that a VersionSet can refer to a self-contained version history state. If the number of saved versions is expected to be very large, we could change that, but then there's a race hazard - so we must

  • lock the version history until the VersionSet is closed, or
  • merge the version access methods into the DBConnection interface, so they can be individually synchronized, or
  • do nothing and add a caveat that the data is not thread-safe, leaving sync to the caller

I don't see another option yet.

// Iterator returns an iterator over all saved versions.
Iterator() VersionIterator
// Equal returns true iff this set is identical to another.
Equal(VersionSet) bool
// Exists returns true if a saved version exists.
Exists(uint64) bool
}

type VersionIterator interface {
// Next advances the iterator to the next element.
// Returns whether the iterator is valid; once invalid, it remains invalid forever.
Next() bool
// Value returns the version ID at the current position.
Value() uint64
}