feat(internal/database): badger v3 and memory implementations #3005

qdm12 · 2022-12-13T16:12:33Z

Changes

ℹ️ This is only the standalone database implementation. I need the in-memory implementation for #2831 which in turn unblocks #2981 which will unblock the full replacement of chaindb. This doesn't affect the existing codebase for now.

Tests

go test github.com/ChainSafe/gossamer/internal/database/...

Issues

Related #1973 / will create an issue for replacing the database implementation

Primary Reviewer

@timwu20

internal/database/badger/writebatch.go

qdm12 · 2022-12-13T16:19:54Z

internal/database/memory/database.go

+// Database is an in-memory database implementation.
+type Database struct {


The point of the in-memory database implementation package is to force the internal/database code to be flexible for additional implementations.

~~On top of that, I would suggest us to use this one instead of the in-memory badger database, so the badger database is only on disk.~~ Never mind stupid idea, we should test using the badger code (if badger is the default prod db) to make sure it works with the badger underlying code. Added an InMemory setting field for it.

The point of the in-memory database implementation package is to force the internal/database code to be flexible for additional implementations.

Does that mean it is not getting used anywhere?

It could if we add a config field for the database implementation later. Anyway for now both implementations are not used.

The memory implementation serves as a good example for another database implementation (as in it's a 'dumb' base just using Go std types).

if tests can use the badger in memory version, do we really need this in memory implementation?

I would like to keep it as a base for another future implementation. For example badger uses the badger/v3 batch implementation, but some other implementations might need to implement their own batching, and it's already there in the (zero-dependency) memory implementation. I can create an issue to remove it later on once we have at least another implementation?

internal/database/memory/writebatch.go

internal/database/badger/database.go

internal/database/badger/settings.go

internal/database/badger/database.go

internal/database/badger/helpers.go

kishansagathiya

Looks good overall.

Yet to look at test files.

kishansagathiya · 2022-12-16T11:56:08Z

internal/database/badger/settings.go

+	// It defaults to the current directory if left unset.
+	Path string
+	// InMemory is whether to use an in-memory database.
+	InMemory *bool


doesnt look like it has to be a pointer

Actually the point of this is to detect when it's left unset, since false is not 'unset'. That way we can set defaults as we want with a method.

More details on https://github.com/qdm12/gosettings#settings-struct I have been using this for about a year, and it has been quite scalable and nice to use.

why do we need to check if it is set or unset?

To be able to set defaults if it's left unset (and it extends beyond eventually, such as merging/overriding settings structs).

I don't see why this is necessary. IMO InMemory should default to false.

To be able to set defaults if it's left unset (and it extends beyond eventually, such as merging/overriding settings structs).

I don't think I understand this completely.

it extends beyond eventually, such as merging/overriding settings structs

May be elaborate this part. Do we need to merge or override this setting at some point?

What I am really not able to understand is that why can't we treat false as unset.

Say someone assigned a false value to inMemory, why do we care if it was set or it is there because it is default value?

What I am really not able to understand is that why can't we treat false as unset.

Because false or true are set values. The user can specify false.
Say we want to read from two configuration sources such as flags and toml, and we want flags to take precedence.
The flag specifies --inmemory=false and the toml config specifies inmemory = true. If we consider false as unset, then we would end up with a final value of true whereas it should be false due to precedence.
Same applies with merging/overriding settings.

It's perhaps out of scope and not used yet, but it's good code writing imo and there is no point neglecting this.

If we consider false as unset, then we would end up with a final value of true whereas it should be false due to precedence

If I want flag to take precedence, I would just take flag value and assign it to inMemory. It doesn't matter if it is set or not. So, if the flag says false, the value of inMemory will become false.

The way we deal with such setting is,

initialise with default settings with, say Default().

Override with say toml config (we don't need to know existing values in settings here)

Override with say flag (we don't need existing values here as well)

At no point we need to know existing settings values.

I don't think we need inMemory to be pointer here.

I don't see why we would want to support detecting an unset value for this attribute. I see no reason why the default value for InMemory should not be false.

Read #3005 (comment) again. Empty value should be invalid (and for a boolean, false and true are both 'set values').

If I want flag to take precedence, I would just take flag value and assign it to inMemory. It doesn't matter if it is set or not. So, if the flag says false, the value of inMemory will become false.

Say the flag is --inmemory=false, then your field is set to false. Then your next source (i.e. toml) sets inmemory = true. How does it know if it should override this false with its true?? You can have ugly logic for every field to do this, but it's much more modular to just use a boolean pointer and if it's nil, then just set it to the value read from the settings source, otherwise leave it to its non-nil set value

EDIT: if it's such a blocker for this PR, I can switch it to a boolean but it's kinda the wrong approach for new code imo

internal/database/interfaces.go

kishansagathiya · 2022-12-16T12:11:20Z

internal/database/memory/database.go

+// Database is an in-memory database implementation.
+type Database struct {


The point of the in-memory database implementation package is to force the internal/database code to be flexible for additional implementations.

Does that mean it is not getting used anywhere?

kishansagathiya · 2022-12-16T12:20:29Z

internal/database/memory/database.go

+}
+
+// Close closes the database.
+func (db *Database) Close() (err error) {


Is this getting used anywhere other than tests?

Also, it might make sense to use mutex here to make sure that we don't close it while someone is using the db.

Is this getting used anywhere other than tests?

Both implementations are not used anywhere in this PR. The badger implementation will be used in production code and in tests in the next PR. The in memory implementation could be used in tests, but really we should test using the in-memory badger implementation. So at the end of the day, that memory implementation is really just a base to serve for another implementation (zero dependency, just Go code with what should be expected in terms of implementation)

Also, it might make sense to use mutex here to make sure that we don't close it while someone is using the db.

Totally, added it 😉

It's fully replaced in #3088 now 😉 (based on this PR)

internal/database/memory/table.go

internal/database/memory/writebatch.go

kishansagathiya

Most of the code looks good.

Considering we have two database implementations, we should have the same test testing for both the implementations.
It not nice that none of these functions are not getting used in this PR. Ideally, it will be nice if we have a db interface that is getting used. Then, In the same PR we can implement a db and plug it will rest of the codebase to use it to test if things are working fine. Let me know if that can't be done at the moment.

internal/database/badger/database_test.go

internal/database/badger/race_test.go

kishansagathiya · 2023-01-04T08:39:26Z

internal/database/badger/settings.go

+	// It defaults to the current directory if left unset.
+	Path string
+	// InMemory is whether to use an in-memory database.
+	InMemory *bool


why do we need to check if it is set or unset?

qdm12 · 2023-01-04T14:01:47Z

It not nice that none of these functions are not getting used in this PR. Ideally, it will be nice if we have a db interface that is getting used. Then, In the same PR we can implement a db and plug it will rest of the codebase to use it to test if things are working fine. Let me know if that can't be done at the moment.

Yeah I get your point; using this newer interface/packages is blocked by #2831 which I'm working on now. Once this is merged, I'll rebase this branch on it and make a separate PR based on this one, which we can merge into this PR. Trying to keep this PR and the future interface-replacement PR separate to the amount of deltas in each (for reviewers), but we should merge it as a single commit in development indeed 👍

internal/database/badger/database.go

internal/database/badger/helpers.go

timwu20 · 2023-01-17T02:17:51Z

internal/database/badger/settings.go

+	// It defaults to the current directory if left unset.
+	Path string
+	// InMemory is whether to use an in-memory database.
+	InMemory *bool


I don't see why this is necessary. IMO InMemory should default to false.

internal/database/badger/writebatch.go

timwu20 · 2023-01-17T02:22:34Z

internal/database/memory/database.go

+// Database is an in-memory database implementation.
+type Database struct {


if tests can use the badger in memory version, do we really need this in memory implementation?

timwu20 · 2023-01-17T21:31:00Z

I think we should wait to merge this into development. We should probably do some testing to ensure things are working as expected before merging in. Can you create a release branch that will contain this PR and the PR that utilises this code?

- Path cannot be the empty string, defaults to `.` - Deleting a non-existing key returns nil from badger - Remove `newTable` unneeded constructor - Add `makePrefixedKey` function, `append` is WRONG to use

…ehavior

…y tests

Co-authored-by: Kishan Sagathiya <kishansagathiya@gmail.com>

- Add settings helper method `WithPath` - Add settings helper method `WithInMemory`

qdm12 force-pushed the qdm12/internal/database branch from e3a3f86 to 95a71f1 Compare December 13, 2022 16:26

qdm12 commented Dec 13, 2022

View reviewed changes

qdm12 force-pushed the qdm12/internal/database branch from f6480a0 to 8069eee Compare December 13, 2022 16:28

qdm12 marked this pull request as ready for review December 13, 2022 16:29

qdm12 requested review from noot, edwardmack, timwu20, EclesioMeloJunior, jimjbrettj and kishansagathiya as code owners December 13, 2022 16:29

EclesioMeloJunior requested changes Dec 14, 2022

View reviewed changes

internal/database/badger/database.go Outdated Show resolved Hide resolved

internal/database/badger/database.go Outdated Show resolved Hide resolved

internal/database/badger/settings.go Show resolved Hide resolved

qdm12 force-pushed the qdm12/internal/database branch from 4d2aa37 to f57b5bf Compare December 15, 2022 10:06

qdm12 requested a review from EclesioMeloJunior December 15, 2022 10:06

qdm12 commented Dec 15, 2022

View reviewed changes

internal/database/badger/database.go Show resolved Hide resolved

qdm12 force-pushed the qdm12/internal/database branch 2 times, most recently from d685c56 to f886b62 Compare December 15, 2022 17:29

qdm12 commented Dec 15, 2022

View reviewed changes

internal/database/badger/helpers.go Outdated Show resolved Hide resolved

kishansagathiya reviewed Dec 16, 2022

View reviewed changes

qdm12 force-pushed the qdm12/internal/database branch from 51226ca to 0e4a4cc Compare January 2, 2023 11:12

kishansagathiya requested changes Jan 4, 2023

View reviewed changes

qdm12 force-pushed the qdm12/internal/database branch 2 times, most recently from 50fad42 to 3ea9abf Compare January 9, 2023 15:57

timwu20 reviewed Jan 17, 2023

View reviewed changes

qdm12 mentioned this pull request Jan 24, 2023

chore(state): remove Full online pruner #3063

Merged

qdm12 force-pushed the qdm12/internal/database branch 2 times, most recently from 0f73b2d to be520e3 Compare January 27, 2023 14:13

qdm12 changed the base branch from development to qdm12/dep-inject-db January 27, 2023 14:13

qdm12 mentioned this pull request Jan 30, 2023

chore(dot/state): offline pruner changes #3084

Merged

qdm12 force-pushed the qdm12/internal/database branch 2 times, most recently from 1b91d82 to 5875dc3 Compare March 11, 2023 11:34

qdm12 and others added 25 commits March 30, 2023 17:34

internal/database: shared errors and interfaces

86dbc26

internal/database/memory implementation with tests

f5122f5

internal/database/badger implementation

f52ebc2

Remove DropAll method on tables

eab45c2

Add copyright notices

21a4b79

Shorter error return for txn.Set(key, value)

20bff2d

internal/database/badger tests and fixes

9283017

- Path cannot be the empty string, defaults to `.` - Deleting a non-existing key returns nil from badger - Remove `newTable` unneeded constructor - Add `makePrefixedKey` function, `append` is WRONG to use

internal/database/memory: discard batch once cancel to match badger b…

ca1c1e9

…ehavior

Add badger race test

b098c6d

Add race tests to make test-state-race including other thread safet…

4b39e80

…y tests

Add InMemory badger setting field

940ad6d

Fix typo in package description for internal/database

d1d840f

Co-authored-by: Kishan Sagathiya <kishansagathiya@gmail.com>

internal/database/memory: use mutex for close

99ce8f3

Mutex protect closed boolean

6e06ba2

memory: return database.ErrClosed instead of panic

9981401

Transform badger error in database.ErrClosed

84d92fa

Create blackbox testing database_test package

6809532

Remove commented test code

5256a92

Fix deepsource warnings

db445bf

Alias badger/v3 import with badger

257e2ff

makePrefixedKey -> newPrefixedKey

0e9f18e

Use atomic.Bool for closed field

f020a91

Add Stream method

e44e10c

Badger path must be empty when in-memory

fbd3c74

- Add settings helper method `WithPath` - Add settings helper method `WithInMemory`

Fix test snake cases

1db53a5

qdm12 force-pushed the qdm12/dep-inject-db branch from 67c1c07 to 7d33f1a Compare March 30, 2023 18:34

qdm12 force-pushed the qdm12/internal/database branch from 5875dc3 to 1db53a5 Compare March 30, 2023 18:35

timwu20 closed this Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(internal/database): badger v3 and memory implementations #3005

feat(internal/database): badger v3 and memory implementations #3005

qdm12 commented Dec 13, 2022 •

edited

Loading

qdm12 Dec 13, 2022 •

edited

Loading

kishansagathiya Dec 16, 2022

qdm12 Jan 2, 2023 •

edited

Loading

timwu20 Jan 17, 2023

qdm12 Jan 17, 2023

kishansagathiya left a comment

kishansagathiya Dec 16, 2022

qdm12 Jan 2, 2023

kishansagathiya Jan 4, 2023

qdm12 Jan 9, 2023 •

edited

Loading

timwu20 Jan 17, 2023

kishansagathiya Jan 17, 2023

qdm12 Jan 17, 2023

kishansagathiya Jan 17, 2023 •

edited

Loading

timwu20 Jan 17, 2023

qdm12 Mar 2, 2023 •

edited

Loading

kishansagathiya Dec 16, 2022

kishansagathiya Dec 16, 2022

kishansagathiya Dec 16, 2022

qdm12 Jan 2, 2023 •

edited

Loading

qdm12 Mar 12, 2023

kishansagathiya left a comment •

edited

Loading

kishansagathiya Jan 4, 2023

qdm12 commented Jan 4, 2023

timwu20 Jan 17, 2023

timwu20 Jan 17, 2023

timwu20 commented Jan 17, 2023

		// Database is an in-memory database implementation.
		type Database struct {

feat(internal/database): badger v3 and memory implementations #3005

feat(internal/database): badger v3 and memory implementations #3005

Conversation

qdm12 commented Dec 13, 2022 • edited Loading

Changes

Tests

Issues

Primary Reviewer

qdm12 Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 Jan 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya 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

qdm12 Jan 9, 2023 • edited Loading

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

kishansagathiya Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 Mar 2, 2023 • edited Loading

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

qdm12 Jan 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timwu20 commented Jan 17, 2023

qdm12 commented Dec 13, 2022 •

edited

Loading

qdm12 Dec 13, 2022 •

edited

Loading

qdm12 Jan 2, 2023 •

edited

Loading

qdm12 Jan 9, 2023 •

edited

Loading

kishansagathiya Jan 17, 2023 •

edited

Loading

qdm12 Mar 2, 2023 •

edited

Loading

qdm12 Jan 2, 2023 •

edited

Loading

kishansagathiya left a comment •

edited

Loading