sqldb: new package supporting sql backends by positiveblue · Pull Request #7343 · lightningnetwork/lnd

positiveblue · 2023-01-19T16:37:05Z

This is the second PR for #6288

Only the latest commit applies to this PR

The previous PR we added the sql schema and queries for invoices.

In this PR provide the scaffolding for using the new sql stores.

The new interfaces, structs and methods are in sync with other projects like Taproot Assets.

Transactional Queries: the sqldb package defines the interfaces required to execute transactional queries to our storage interface.
Embed Migration Files: the migration files are embedded into the binary.
Database Migrations: I kept the use of 'golang-migrate' to ensure our codebase remains in sync with the other projects, but it can be changed. (@joostjager provided some feedback about golang-migrate pitfalls and how sql-migrate can be a good subtitute)
Build Flags for Conditional DB Target: flexibility to specify our database target at compile-time based on the build flags in the same way we do with our kv stores.
SQL utils: generic functions used for marshal/unmarshal db data. <= I will fix the linter later
Update modules: ran go mod tidy.

NOTE: the structure of this PR has changed so some of the old comments don't apply or apply to other PRs.

joostjager · 2023-01-20T07:33:25Z

Maybe you saw this comment already, but there's a gotcha with golang-migrate: #7251 (comment) / #6176 (comment)

This is totally on point, thanks for sharing

bitromortac

Very cool to see what's needed to operate on two different backends, great work 🚀. Do you have plans to unify the backend creation with what's present in the kvdb package? Will also take a look at the follow-up PRs to build more context.

bitromortac · 2023-01-31T12:12:05Z

There is already some fixture functionality in the kvdb_postgres, perhaps it can be reused?

I think using the dockertest vs embedded postgres has some advantages. We can always extend the docker based one with more backends if needed and also can easily change versions at test time.

bitromortac · 2023-02-01T13:44:48Z

nit: postgres

saubyk · 2023-06-13T05:01:45Z

Concept Ack

bhandras

Looks very good to me, just a few comments/questions added. 🥇

bhandras · 2023-07-12T07:40:06Z

I think using the dockertest vs embedded postgres has some advantages. We can always extend the docker based one with more backends if needed and also can easily change versions at test time.

bhandras · 2023-07-12T07:42:54Z

+func parseSqliteError(sqliteErr *sqlite.Error) error {
+	switch sqliteErr.Code() {
+	// Handle unique constraint violation error.
+	case sqlite3.SQLITE_CONSTRAINT_UNIQUE:


I wonder if there's added value from mapping the unique constraint error at all? Apart from logging of course.

IIRC, we ended up doing this in the tap repo so we could get better logging from the DB, otherwise it's some like single integer return error from sqlite.

bhandras · 2023-07-12T07:44:20Z

+	defaultMaxConns = 25
+
+	// connIdleLifetime is the amount of time a connection can be idle.
+	connIdleLifetime = 5 * time.Minute


Will this result in the connection pool shrinking? If so will the pool be repopulated automatically?

IIUC, this lets a connection be idle for 5 mins at a time before being garbage colelcted. These "connections" re really just in memory structs (for sqlite) managed by the database/sql package. FWIW, these values haven't been tuned a ton other than things like some of the pragma directives we realized we needed.

bhandras · 2023-07-12T07:45:44Z

+			value: "full",
+		},
+		{
+			// This is used to ensure proper durability for users


While this is true, iirc mac users will suffer greatly when it comes to performance. Some itests may even fail too. I think the mac philosophy is that writes are durable as long as there's battery remaining and even then the OS is capable to putting the system to sleep when needed.

FWIW this is only active on mac. It is the case that this dramatically improves perf if removed. IIRC, for sqlite elsewhere in the project, we move these directives up one level and allow users to override them.

Roasbeef · 2023-07-13T00:36:55Z

Can be rebased now!

Roasbeef · 2023-07-14T00:24:46Z

+// struct.
+func (s *BaseDB) BeginTx(ctx context.Context, opts TxOptions) (*sql.Tx, error) {
+	sqlOptions := sql.TxOptions{
+		ReadOnly: opts.ReadOnly(),


We should also inherit the strongest serializability guarntees we can here as well: https://github.com/lightninglabs/taproot-assets/blob/main/tapdb/interfaces.go#L251

Note that this only actually matters for postgres.

We'll also want to revisit that mutex we added as well, as with this guarantee, we ensure that all transactions are serializable, and they're bailed if not. Hence the need for that retry logic I linked above.

Roasbeef · 2023-07-14T00:26:11Z

+func parseSqliteError(sqliteErr *sqlite.Error) error {
+	switch sqliteErr.Code() {
+	// Handle unique constraint violation error.
+	case sqlite3.SQLITE_CONSTRAINT_UNIQUE:


IIRC, we ended up doing this in the tap repo so we could get better logging from the DB, otherwise it's some like single integer return error from sqlite.

Roasbeef · 2023-07-14T00:27:04Z

+	defaultMaxConns = 25
+
+	// connIdleLifetime is the amount of time a connection can be idle.
+	connIdleLifetime = 5 * time.Minute


IIUC, this lets a connection be idle for 5 mins at a time before being garbage colelcted. These "connections" re really just in memory structs (for sqlite) managed by the database/sql package. FWIW, these values haven't been tuned a ton other than things like some of the pragma directives we realized we needed.

Roasbeef · 2023-07-14T00:27:59Z

+			value: "full",
+		},
+		{
+			// This is used to ensure proper durability for users


FWIW this is only active on mac. It is the case that this dramatically improves perf if removed. IIRC, for sqlite elsewhere in the project, we move these directives up one level and allow users to override them.

lightninglabs-deploy · 2023-07-21T01:14:52Z

@positiveblue, remember to re-request review from reviewers when ready

Roasbeef · 2023-07-23T13:54:50Z

Failing with a linter error atm:

no changes added to commit (use "git add" and/or "git commit -a")
make: *** [Makefile:239: fmt-check] Error 1
diff --git a/sqldb/postgres.go b/sqldb/postgres.go
index 72f31c9e7..bc[34](https://github.com/lightningnetwork/lnd/actions/runs/5597978693/job/15163270504?pr=7343#step:4:35)ecb43 100644
--- a/sqldb/postgres.go
+++ b/sqldb/postgres.go
@@ -7,7 +7,6 @@ import (
 	"time"
 
 	postgres_migrate "github.com/golang-migrate/migrate/v4/database/postgres"
-	// Read migrations from files.
 	_ "github.com/golang-migrate/migrate/v4/source/file"
 	"github.com/lightningnetwork/lnd/sqldb/sqlc"
 	"github.com/stretchr/testify/require"
diff --git a/sqldb/postgres_fixture.go b/sqldb/postgres_fixture.go
index f8dc2dd3d..becf3007d 1006[44](https://github.com/lightningnetwork/lnd/actions/runs/5597978693/job/15163270504?pr=7343#step:4:45)
--- a/sqldb/postgres_fixture.go
+++ b/sqldb/postgres_fixture.go
@@ -9,7 +9,6 @@ import (
 	"testing"
 	"time"
 
-	// Import the postgres driver.
 	_ "github.com/lib/pq"
 	"github.com/ory/dockertest/v3"
 	"github.com/ory/dockertest/v3/docker"

This commit provides the scaffolding for using the new sql stores. The new interfaces, structs and methods are in sync with other projects like Taproot Assets. - Transactional Queries: the sqldb package defines the interfaces required to execute transactional queries to our storage interface. - Migration Files Embedded: the migration files are embedded into the binary. - Database Migrations: I kept the use of 'golang-migrate' to ensure our codebase remains in sync with the other projects, but can be changed. - Build Flags for Conditional DB Target: flexibility to specify our database target at compile-time based on the build flags in the same way we do with our kv stores. - Update modules: ran `go mod tidy`.

Roasbeef

LGTM 🍨

Roasbeef · 2023-07-27T18:05:45Z

+// randRetryDelay returns a random retry delay between 0 and the configured max
+// delay.
+func (t *txExecutorOptions) randRetryDelay() time.Duration {
+	return time.Duration(prand.Int63n(int64(t.retryDelay))) //nolint:gosec


One thing we want to fix in the repo this originated was to make this actually have a min back off, as is right now, it can use a rather small value (say 1 ns) which causes the retries to bleed out pretty quickly.

So I think we want to also have a min value here, then offset from that, 50 ms for a min value seems good I think? Then we can add a TODO here to circle back on doing proper exponential backoff as well.

Or perhaps we can also try and do an exponential ramp up?

bhandras

Awesome work @positiveblue, LGTM 🥇

bhandras · 2023-07-27T18:11:11Z

+// randRetryDelay returns a random retry delay between 0 and the configured max
+// delay.
+func (t *txExecutorOptions) randRetryDelay() time.Duration {
+	return time.Duration(prand.Int63n(int64(t.retryDelay))) //nolint:gosec


Or perhaps we can also try and do an exponential ramp up?

bhandras · 2023-07-27T18:12:27Z

+
+		// Before we try again, we'll wait with a random backoff based
+		// on the retry delay.
+		time.Sleep(retryDelay)


If the retry delay can take seconds (in the extreme case) perhaps worth using a select and time.After() channel instead so we can quickly return if the passed in context is canceled.

bhandras · 2023-07-27T18:13:35Z

+	}
+
+	if stat.IsDir() {
+		return f, err


nit: s/err/nil

bhandras · 2023-07-27T18:15:29Z

+	}
+
+	rawDB.SetMaxOpenConns(maxConns)
+	rawDB.SetMaxIdleConns(maxConns)


I wonder if it's worth setting the idle conns to something lower? Perhaps defaultMaxConns/2?

ziggie1984

Very interesting PR, design still over my head but getting grip on it. Had some nits regarding the comments apart from that consider my review as a novice in this field.

ziggie1984 · 2023-07-14T16:36:52Z

+)
+
+// TxOptions represents a set of options one can use to control what type of
+// database transaction is created. Transaction can wither be read or write.


nit: wither=>either

ziggie1984 · 2023-07-14T17:21:16Z

+// TransactionExecutor is a generic struct that abstracts away from the type of
+// query a type needs to run under a database transaction, and also the set of


nit: not sure if I understand the sentence correctly but shouldn't this be:

TransactionExecutor is a generic struct that abstracts away from the type of query which type needs ...

ziggie1984 · 2023-07-14T17:30:02Z

+}
+
+// NewTestPgFixture constructs a new TestPgFixture starting up a docker
+// container running Postgres 11. The started container will expire in after


nit: s/in after/after?

ziggie1984 · 2023-07-30T21:56:59Z

+	return newReplacerFile(f, t.replaces)
+}
+
+type replacerFile struct {


Nit: Should this type also have a comment ?

positiveblue force-pushed the sqldb branch 2 times, most recently from 041c7e3 to de45917 Compare January 19, 2023 17:51

joostjager reviewed Jan 20, 2023

View reviewed changes

positiveblue force-pushed the sqldb branch 2 times, most recently from d49fab0 to e6eff7c Compare January 24, 2023 14:35

positiveblue mentioned this pull request Jan 24, 2023

sqldb: add invoice schema and sql queries #7354

Merged

saubyk assigned positiveblue Jan 26, 2023

saubyk added invoices sql database Related to the database/storage of LND labels Jan 26, 2023

saubyk added this to the v0.16.0 milestone Jan 26, 2023

saubyk requested a review from bitromortac January 26, 2023 18:22

bitromortac reviewed Feb 1, 2023

View reviewed changes

saubyk modified the milestones: v0.16.0, v0.16.1 Feb 14, 2023

Roasbeef requested a review from sputn1ck February 24, 2023 04:47

saubyk modified the milestones: v0.16.1, v0.17.0 Mar 13, 2023

positiveblue mentioned this pull request May 11, 2023

[Epic] channeldb+invoices: create native SQL schema for invoice storage #6288

Closed

5 tasks

positiveblue force-pushed the sqldb branch from e6eff7c to 3972d08 Compare May 30, 2023 15:25

positiveblue force-pushed the sqldb branch 2 times, most recently from 57cb5eb to ddd9560 Compare June 8, 2023 00:08

positiveblue mentioned this pull request Jun 9, 2023

[WIP] Invocies: refactor InvoiceDB methods #7542

Closed

positiveblue force-pushed the sqldb branch 2 times, most recently from 5b0c990 to 8f47937 Compare June 10, 2023 00:11

positiveblue marked this pull request as ready for review June 10, 2023 01:19

positiveblue force-pushed the sqldb branch from 8f47937 to 71ea76a Compare June 11, 2023 20:30

positiveblue requested a review from bhandras June 21, 2023 22:54

positiveblue force-pushed the sqldb branch from 71ea76a to 2cb5b73 Compare June 24, 2023 16:53

positiveblue force-pushed the sqldb branch from 2cb5b73 to 5072c3c Compare June 24, 2023 17:30

bhandras reviewed Jul 12, 2023

View reviewed changes

Roasbeef requested changes Jul 14, 2023

View reviewed changes

positiveblue force-pushed the sqldb branch from 5072c3c to 6e79624 Compare July 19, 2023 10:27

Roasbeef requested changes Jul 23, 2023

View reviewed changes

Comment thread sqldb/interfaces.go Outdated

positiveblue force-pushed the sqldb branch 2 times, most recently from 0062060 to 8fd28e1 Compare July 27, 2023 09:45

positiveblue added 2 commits July 27, 2023 03:31

docs: add release note for 7343

099907c

positiveblue force-pushed the sqldb branch from 8fd28e1 to 099907c Compare July 27, 2023 10:31

saubyk requested review from Roasbeef and bhandras July 27, 2023 15:18

Roasbeef approved these changes Jul 27, 2023

View reviewed changes

bhandras approved these changes Jul 27, 2023

View reviewed changes

ziggie1984 reviewed Jul 30, 2023

View reviewed changes

Roasbeef merged commit 1871970 into lightningnetwork:master Jul 31, 2023

saubyk moved this from High Priority to Merged in PR Review Priority May 15, 2025

		// TransactionExecutor is a generic struct that abstracts away from the type of
		// query a type needs to run under a database transaction, and also the set of

Conversation

positiveblue commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saubyk commented Jun 13, 2023

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented Jul 13, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Jul 21, 2023

Uh oh!

Roasbeef commented Jul 23, 2023

Uh oh!

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ziggie1984 left a comment

Choose a reason for hiding this comment

Uh oh!

positiveblue commented Jan 19, 2023 •

edited

Loading

joostjager Jan 20, 2023 •

edited

Loading