From e45ed86263f72b1e8c579742d8ebc9d4621c9e18 Mon Sep 17 00:00:00 2001 From: Andras Banki-Horvath Date: Fri, 21 Jun 2024 18:31:43 +0200 Subject: [PATCH 1/3] invoices: fix and correctly cover paginated queries Previously paginated queries offseted the add_index_get, add_index_let, settle_index_get and settle_index_let parameters with the paginators current page offset, however this was incorrect as we can just use SQL's LIMIT/OFFSET to paginate. This commit fixes this issue and adds an optional parameter to the constructor of the invoice SQL store to set page size. This is useful when testing as we can now cover pagination correctly with our existing unit tests. --- invoices/invoices_test.go | 9 +++- invoices/sql_store.go | 91 ++++++++++++++++++++++----------------- 2 files changed, 59 insertions(+), 41 deletions(-) diff --git a/invoices/invoices_test.go b/invoices/invoices_test.go index 71da385714..aa4aa6a709 100644 --- a/invoices/invoices_test.go +++ b/invoices/invoices_test.go @@ -249,7 +249,14 @@ func TestInvoices(t *testing.T) { testClock := clock.NewTestClock(testNow) - return invpkg.NewSQLStore(executor, testClock) + // We'll use a pagination limit of 3 for all tests to ensure + // that we also cover query pagination. + const testPaginationLimit = 3 + + return invpkg.NewSQLStore( + executor, testClock, + invpkg.WithPaginationLimit(testPaginationLimit), + ) } for _, test := range testList { diff --git a/invoices/sql_store.go b/invoices/sql_store.go index bcf3c1f892..21254ddef5 100644 --- a/invoices/sql_store.go +++ b/invoices/sql_store.go @@ -20,9 +20,9 @@ import ( ) const ( - // queryPaginationLimit is used in the LIMIT clause of the SQL queries - // to limit the number of rows returned. - queryPaginationLimit = 100 + // defaultQueryPaginationLimit is used in the LIMIT clause of the SQL + // queries to limit the number of rows returned. + defaultQueryPaginationLimit = 100 ) // SQLInvoiceQueries is an interface that defines the set of operations that can @@ -152,16 +152,47 @@ type BatchedSQLInvoiceQueries interface { type SQLStore struct { db BatchedSQLInvoiceQueries clock clock.Clock + opts SQLStoreOptions +} + +// SQLStoreOptions holds the options for the SQL store. +type SQLStoreOptions struct { + paginationLimit int +} + +// defaultSQLStoreOptions returns the default options for the SQL store. +func defaultSQLStoreOptions() SQLStoreOptions { + return SQLStoreOptions{ + paginationLimit: defaultQueryPaginationLimit, + } +} + +// SQLStoreOption is a functional option that can be used to optionally modify +// the behavior of the SQL store. +type SQLStoreOption func(*SQLStoreOptions) + +// WithPaginationLimit sets the pagination limit for the SQL store queries that +// paginate results. +func WithPaginationLimit(limit int) SQLStoreOption { + return func(o *SQLStoreOptions) { + o.paginationLimit = limit + } } // NewSQLStore creates a new SQLStore instance given a open // BatchedSQLInvoiceQueries storage backend. func NewSQLStore(db BatchedSQLInvoiceQueries, - clock clock.Clock) *SQLStore { + clock clock.Clock, options ...SQLStoreOption) *SQLStore { + + opts := defaultSQLStoreOptions() + for _, applyOption := range options { + applyOption(&opts) + } return &SQLStore{ db: db, clock: clock, + opts: opts, } } @@ -617,13 +648,11 @@ func (i *SQLStore) FetchPendingInvoices(ctx context.Context) ( readTxOpt := NewSQLInvoiceQueryReadTx() err := i.db.ExecTx(ctx, &readTxOpt, func(db SQLInvoiceQueries) error { - limit := queryPaginationLimit - return queryWithLimit(func(offset int) (int, error) { params := sqlc.FilterInvoicesParams{ PendingOnly: true, NumOffset: int32(offset), - NumLimit: int32(limit), + NumLimit: int32(i.opts.paginationLimit), Reverse: false, } @@ -646,7 +675,7 @@ func (i *SQLStore) FetchPendingInvoices(ctx context.Context) ( } return len(rows), nil - }, limit) + }, i.opts.paginationLimit) }, func() { invoices = make(map[lntypes.Hash]Invoice) }) @@ -660,8 +689,7 @@ func (i *SQLStore) FetchPendingInvoices(ctx context.Context) ( // InvoicesSettledSince can be used by callers to catch up any settled invoices // they missed within the settled invoice time series. We'll return all known -// settled invoice that have a settle index higher than the passed -// sinceSettleIndex. +// settled invoice that have a settle index higher than the passed idx. // // NOTE: The index starts from 1. As a result we enforce that specifying a value // below the starting index value is a noop. @@ -676,14 +704,11 @@ func (i *SQLStore) InvoicesSettledSince(ctx context.Context, idx uint64) ( readTxOpt := NewSQLInvoiceQueryReadTx() err := i.db.ExecTx(ctx, &readTxOpt, func(db SQLInvoiceQueries) error { - settleIdx := idx - limit := queryPaginationLimit - err := queryWithLimit(func(offset int) (int, error) { params := sqlc.FilterInvoicesParams{ - SettleIndexGet: sqldb.SQLInt64(settleIdx + 1), - NumLimit: int32(limit), + SettleIndexGet: sqldb.SQLInt64(idx + 1), NumOffset: int32(offset), + NumLimit: int32(i.opts.paginationLimit), Reverse: false, } @@ -707,10 +732,8 @@ func (i *SQLStore) InvoicesSettledSince(ctx context.Context, idx uint64) ( invoices = append(invoices, *invoice) } - settleIdx += uint64(limit) - return len(rows), nil - }, limit) + }, i.opts.paginationLimit) if err != nil { return err } @@ -777,7 +800,7 @@ func (i *SQLStore) InvoicesSettledSince(ctx context.Context, idx uint64) ( // InvoicesAddedSince can be used by callers to seek into the event time series // of all the invoices added in the database. This method will return all -// invoices with an add index greater than the specified sinceAddIndex. +// invoices with an add index greater than the specified idx. // // NOTE: The index starts from 1. As a result we enforce that specifying a value // below the starting index value is a noop. @@ -792,14 +815,11 @@ func (i *SQLStore) InvoicesAddedSince(ctx context.Context, idx uint64) ( readTxOpt := NewSQLInvoiceQueryReadTx() err := i.db.ExecTx(ctx, &readTxOpt, func(db SQLInvoiceQueries) error { - addIdx := idx - limit := queryPaginationLimit - return queryWithLimit(func(offset int) (int, error) { params := sqlc.FilterInvoicesParams{ - AddIndexGet: sqldb.SQLInt64(addIdx + 1), - NumLimit: int32(limit), + AddIndexGet: sqldb.SQLInt64(idx + 1), NumOffset: int32(offset), + NumLimit: int32(i.opts.paginationLimit), Reverse: false, } @@ -821,10 +841,8 @@ func (i *SQLStore) InvoicesAddedSince(ctx context.Context, idx uint64) ( result = append(result, *invoice) } - addIdx += uint64(limit) - return len(rows), nil - }, limit) + }, i.opts.paginationLimit) }, func() { result = nil }) @@ -851,21 +869,18 @@ func (i *SQLStore) QueryInvoices(ctx context.Context, readTxOpt := NewSQLInvoiceQueryReadTx() err := i.db.ExecTx(ctx, &readTxOpt, func(db SQLInvoiceQueries) error { - limit := queryPaginationLimit - return queryWithLimit(func(offset int) (int, error) { params := sqlc.FilterInvoicesParams{ NumOffset: int32(offset), - NumLimit: int32(limit), + NumLimit: int32(i.opts.paginationLimit), PendingOnly: q.PendingOnly, + Reverse: q.Reversed, } if q.Reversed { - idx := int32(q.IndexOffset) - // If the index offset was not set, we want to // fetch from the lastest invoice. - if idx == 0 { + if q.IndexOffset == 0 { params.AddIndexLet = sqldb.SQLInt64( int64(math.MaxInt64), ) @@ -873,19 +888,15 @@ func (i *SQLStore) QueryInvoices(ctx context.Context, // The invoice with index offset id must // not be included in the results. params.AddIndexLet = sqldb.SQLInt64( - idx - int32(offset) - 1, + q.IndexOffset - 1, ) } - - params.Reverse = true } else { // The invoice with index offset id must not be // included in the results. params.AddIndexGet = sqldb.SQLInt64( - q.IndexOffset + uint64(offset) + 1, + q.IndexOffset + 1, ) - - params.Reverse = false } if q.CreationDateStart != 0 { @@ -923,7 +934,7 @@ func (i *SQLStore) QueryInvoices(ctx context.Context, } return len(rows), nil - }, limit) + }, i.opts.paginationLimit) }, func() { invoices = nil }) From 892561f8f032a477f54b87bd45c6a8851fa5d751 Mon Sep 17 00:00:00 2001 From: Andras Banki-Horvath Date: Fri, 21 Jun 2024 18:40:03 +0200 Subject: [PATCH 2/3] sqldb: bump modernc.org/sqlite to 1.29.10 which fixes init data race Tracking issue: https://gitlab.com/cznic/sqlite/-/issues/180 --- go.mod | 5 ++++- go.sum | 8 ++------ invoices/invoiceregistry_test.go | 9 --------- invoices/invoices_test.go | 2 -- sqldb/go.mod | 2 +- sqldb/go.sum | 6 ++---- 6 files changed, 9 insertions(+), 23 deletions(-) diff --git a/go.mod b/go.mod index 60b6c5e61b..e0bcd34502 100644 --- a/go.mod +++ b/go.mod @@ -186,7 +186,7 @@ require ( modernc.org/libc v1.49.3 // indirect modernc.org/mathutil v1.6.0 // indirect modernc.org/memory v1.8.0 // indirect - modernc.org/sqlite v1.29.8 // indirect + modernc.org/sqlite v1.29.10 // indirect modernc.org/strutil v1.2.0 // indirect modernc.org/token v1.1.0 // indirect sigs.k8s.io/yaml v1.2.0 // indirect @@ -203,6 +203,9 @@ replace github.com/gogo/protobuf => github.com/gogo/protobuf v1.3.2 // allows us to specify that as an option. replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.30.0-hex-display +// Temporary replace until the next version of sqldb is tagged. +replace github.com/lightningnetwork/lnd/sqldb => ./sqldb + // If you change this please also update .github/pull_request_template.md and // docs/INSTALL.md. go 1.21.4 diff --git a/go.sum b/go.sum index 280c4f2845..5fe81d3507 100644 --- a/go.sum +++ b/go.sum @@ -456,8 +456,6 @@ github.com/lightningnetwork/lnd/kvdb v1.4.8 h1:xH0a5Vi1yrcZ5BEeF2ba3vlKBRxrL9uYX github.com/lightningnetwork/lnd/kvdb v1.4.8/go.mod h1:J2diNABOoII9UrMnxXS5w7vZwP7CA1CStrl8MnIrb3A= github.com/lightningnetwork/lnd/queue v1.1.1 h1:99ovBlpM9B0FRCGYJo6RSFDlt8/vOkQQZznVb18iNMI= github.com/lightningnetwork/lnd/queue v1.1.1/go.mod h1:7A6nC1Qrm32FHuhx/mi1cieAiBZo5O6l8IBIoQxvkz4= -github.com/lightningnetwork/lnd/sqldb v1.0.2 h1:PfuYzScYMD9/QonKo/QvgsbXfTnH5DfldIimkfdW4Bk= -github.com/lightningnetwork/lnd/sqldb v1.0.2/go.mod h1:V2Xl6JNWLTKE97WJnwfs0d0TYJdIQTqK8/3aAwkd3qI= github.com/lightningnetwork/lnd/ticker v1.1.1 h1:J/b6N2hibFtC7JLV77ULQp++QLtCwT6ijJlbdiZFbSM= github.com/lightningnetwork/lnd/ticker v1.1.1/go.mod h1:waPTRAAcwtu7Ji3+3k+u/xH5GHovTsCoSVpho0KDvdA= github.com/lightningnetwork/lnd/tlv v1.2.3 h1:If5ibokA/UoCBGuCKaY6Vn2SJU0l9uAbehCnhTZjEP8= @@ -479,8 +477,6 @@ github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWE github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-runewidth v0.0.13 h1:lTGmDsbAYt5DmK6OnoV7EuIF1wEIFAcxld6ypU4OSgU= github.com/mattn/go-runewidth v0.0.13/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w= -github.com/mattn/go-sqlite3 v1.14.22 h1:2gZY6PC6kBnID23Tichd1K+Z0oS6nE/XwU+Vz/5o4kU= -github.com/mattn/go-sqlite3 v1.14.22/go.mod h1:Uh1q+B4BYcTPb+yiD3kU8Ct7aC0hY9fxUwlHK0RXw+Y= github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0jegS5sx/RkqARlsWZ6pIwiU= github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= github.com/miekg/dns v1.1.43 h1:JKfpVSCB84vrAmHzyrsxB5NAr5kLoMXZArPSw7Qlgyg= @@ -1064,8 +1060,8 @@ modernc.org/opt v0.1.3 h1:3XOZf2yznlhC+ibLltsDGzABUGVx8J6pnFMS3E4dcq4= modernc.org/opt v0.1.3/go.mod h1:WdSiB5evDcignE70guQKxYUl14mgWtbClRi5wmkkTX0= modernc.org/sortutil v1.2.0 h1:jQiD3PfS2REGJNzNCMMaLSp/wdMNieTbKX920Cqdgqc= modernc.org/sortutil v1.2.0/go.mod h1:TKU2s7kJMf1AE84OoiGppNHJwvB753OYfNl2WRb++Ss= -modernc.org/sqlite v1.29.8 h1:nGKglNx9K5v0As+zF0/Gcl1kMkmaU1XynYyq92PbsC8= -modernc.org/sqlite v1.29.8/go.mod h1:lQPm27iqa4UNZpmr4Aor0MH0HkCLbt1huYDfWylLZFk= +modernc.org/sqlite v1.29.10 h1:3u93dz83myFnMilBGCOLbr+HjklS6+5rJLx4q86RDAg= +modernc.org/sqlite v1.29.10/go.mod h1:ItX2a1OVGgNsFh6Dv60JQvGfJfTPHPVpV6DF59akYOA= modernc.org/strutil v1.2.0 h1:agBi9dp1I+eOnxXeiZawM8F4LawKv4NzGWSaLfyeNZA= modernc.org/strutil v1.2.0/go.mod h1:/mdcBmfOibveCTBxUl5B5l6W+TTH1FXPLHZE6bTosX0= modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y= diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go index 4a76d58aae..b0c0195227 100644 --- a/invoices/invoiceregistry_test.go +++ b/invoices/invoiceregistry_test.go @@ -6,7 +6,6 @@ import ( "database/sql" "fmt" "math" - "sync" "testing" "testing/quick" "time" @@ -24,12 +23,6 @@ import ( "github.com/stretchr/testify/require" ) -// sqliteConstructorMu is used to ensure that only one thread can call the -// sqldb.NewTestSqliteDB constructor at a time. This is a temporary workaround -// that can be removed once this race condition in the sqlite repo is resolved: -// https://gitlab.com/cznic/sqlite/-/issues/180 -var sqliteConstructorMu sync.Mutex - // TestInvoiceRegistry is a master test which encompasses all tests using an // InvoiceDB instance. The purpose of this test is to be able to run all tests // with a custom DB instance, so that we can test the same logic with different @@ -137,9 +130,7 @@ func TestInvoiceRegistry(t *testing.T) { var db *sqldb.BaseDB if sqlite { - sqliteConstructorMu.Lock() db = sqldb.NewTestSqliteDB(t).BaseDB - sqliteConstructorMu.Unlock() } else { db = sqldb.NewTestPostgresDB(t, pgFixture).BaseDB } diff --git a/invoices/invoices_test.go b/invoices/invoices_test.go index aa4aa6a709..4e30b0ac96 100644 --- a/invoices/invoices_test.go +++ b/invoices/invoices_test.go @@ -234,9 +234,7 @@ func TestInvoices(t *testing.T) { makeSQLDB := func(t *testing.T, sqlite bool) invpkg.InvoiceDB { var db *sqldb.BaseDB if sqlite { - sqliteConstructorMu.Lock() db = sqldb.NewTestSqliteDB(t).BaseDB - sqliteConstructorMu.Unlock() } else { db = sqldb.NewTestPostgresDB(t, pgFixture).BaseDB } diff --git a/sqldb/go.mod b/sqldb/go.mod index fcf6c3ee9e..51792d2e0b 100644 --- a/sqldb/go.mod +++ b/sqldb/go.mod @@ -11,7 +11,7 @@ require ( github.com/ory/dockertest/v3 v3.10.0 github.com/stretchr/testify v1.9.0 golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8 - modernc.org/sqlite v1.29.8 + modernc.org/sqlite v1.29.10 ) require ( diff --git a/sqldb/go.sum b/sqldb/go.sum index 2d92b7667f..140eb99967 100644 --- a/sqldb/go.sum +++ b/sqldb/go.sum @@ -92,8 +92,6 @@ github.com/lib/pq v1.10.9 h1:YXG7RB+JIjhP29X+OtkiDnYaXQwpS4JEWq7dtCCRUEw= github.com/lib/pq v1.10.9/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= -github.com/mattn/go-sqlite3 v1.14.22 h1:2gZY6PC6kBnID23Tichd1K+Z0oS6nE/XwU+Vz/5o4kU= -github.com/mattn/go-sqlite3 v1.14.22/go.mod h1:Uh1q+B4BYcTPb+yiD3kU8Ct7aC0hY9fxUwlHK0RXw+Y= github.com/mitchellh/mapstructure v1.4.1 h1:CpVNEelQCZBooIPDn+AR3NpivK/TIKU8bDxdASFVQag= github.com/mitchellh/mapstructure v1.4.1/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= github.com/moby/sys/mountinfo v0.5.0/go.mod h1:3bMD3Rg+zkqx8MRYPi7Pyb0Ie97QEBmdxbhnCLlSvSU= @@ -233,8 +231,8 @@ modernc.org/opt v0.1.3 h1:3XOZf2yznlhC+ibLltsDGzABUGVx8J6pnFMS3E4dcq4= modernc.org/opt v0.1.3/go.mod h1:WdSiB5evDcignE70guQKxYUl14mgWtbClRi5wmkkTX0= modernc.org/sortutil v1.2.0 h1:jQiD3PfS2REGJNzNCMMaLSp/wdMNieTbKX920Cqdgqc= modernc.org/sortutil v1.2.0/go.mod h1:TKU2s7kJMf1AE84OoiGppNHJwvB753OYfNl2WRb++Ss= -modernc.org/sqlite v1.29.8 h1:nGKglNx9K5v0As+zF0/Gcl1kMkmaU1XynYyq92PbsC8= -modernc.org/sqlite v1.29.8/go.mod h1:lQPm27iqa4UNZpmr4Aor0MH0HkCLbt1huYDfWylLZFk= +modernc.org/sqlite v1.29.10 h1:3u93dz83myFnMilBGCOLbr+HjklS6+5rJLx4q86RDAg= +modernc.org/sqlite v1.29.10/go.mod h1:ItX2a1OVGgNsFh6Dv60JQvGfJfTPHPVpV6DF59akYOA= modernc.org/strutil v1.2.0 h1:agBi9dp1I+eOnxXeiZawM8F4LawKv4NzGWSaLfyeNZA= modernc.org/strutil v1.2.0/go.mod h1:/mdcBmfOibveCTBxUl5B5l6W+TTH1FXPLHZE6bTosX0= modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y= From b35f0606ba1edcd2c1b49f21db602ebc4b328fb9 Mon Sep 17 00:00:00 2001 From: Andras Banki-Horvath Date: Thu, 27 Jun 2024 22:37:45 +0200 Subject: [PATCH 3/3] docs: update release notes for 0.18.2-beta --- docs/release-notes/release-notes-0.18.2.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.18.2.md b/docs/release-notes/release-notes-0.18.2.md index e2216bc700..6d44635e4a 100644 --- a/docs/release-notes/release-notes-0.18.2.md +++ b/docs/release-notes/release-notes-0.18.2.md @@ -91,6 +91,10 @@ ## Testing ## Database + +* [Fixed](https://github.com/lightningnetwork/lnd/pull/8854) pagination issues + in SQL invoicedb queries. + ## Code Health ## Tooling and Documentation