invoices: migrate KV invoices to native SQL for users of KV SQL backends #8831
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
bhandras
force-pushed
the
sql-invoice-migration
branch
3 times, most recently
from
6682b50 to
338e1f0
Compare
bhandras
force-pushed
the
sql-invoice-migration
branch
3 times, most recently
from
d2a329f to
6379a8b
Compare
bhandras
force-pushed
the
sql-invoice-migration
branch
2 times, most recently
from
5fe92e2 to
a7bf598
Compare
bhandras
force-pushed
the
sql-invoice-migration
branch
5 times, most recently
from
b6f0ac8 to
b983851
Compare
bhandras
changed the title
bhandras
force-pushed
the
sql-invoice-migration
branch
from
b983851 to
706b444
Compare
bhandras
force-pushed
the
sql-invoice-migration
branch
7 times, most recently
from
96f0cbe to
bfe4ad5
Compare
bhandras
force-pushed
the
sql-invoice-migration
branch
from
3955d21 to
4c09270
Compare
| CREATE TABLE IF NOT EXISTS migration_tracker ( | ||
| -- version is the global version of the migration. Note that we | ||
| -- intentionally don't set it as PRIMARY KEY as it'd auto increment on | ||
| -- SQLite and our sqlc workflow will replace it with an auto incementing |
There was a problem hiding this comment.
s/incementing/incrementing
| Version: 6, | ||
| SchemaVersion: 6, |
There was a problem hiding this comment.
yeah gotcha re how it is working.
Just personal preference would be 1 logical migration per config since as is, it is possible to have "Version 6" run but then actually only the schema update succeeds and then the code-level mig fails and then when talking about it, we would need to say "the second part of the migration 6 (ie, the code part) failed" but part of the migration succeeded. I think it's nicer to say "mig 6 completed, mig 7 failed"
but yeah - defs a subjective opinion i guess. Happy to let second reviewer weigh in
| Version: 6, | ||
| SchemaVersion: 6, |
There was a problem hiding this comment.
another point for the suggestion is: if we are are reading the code later on and want to see where a migration was introduced and we go back to this commit: I rate we want to see the entire migration in the commit. Currently this commit only tells half the story of mig 6
| // invoice into the SQL database. The parameters are derived from the original | ||
| // invoice insert parameters. | ||
| func toInsertMigratedInvoiceParams(params sqlc.InsertInvoiceParams, | ||
| ) sqlc.InsertMigratedInvoiceParams { |
There was a problem hiding this comment.
nit: formatting 🤓 (i know there is a war going on re what looks best but current guidelines suggest
func toInsertMigratedInvoiceParams(
params sqlc.InsertInvoiceParams) sqlc.InsertMigratedInvoiceParams {
invoices/testdata/channel.db
Outdated
There was a problem hiding this comment.
cc @bhandras - thoughts?
| // state DB. Let's query the database to see if we have any | ||
| // invoices there. If we do, we won't allow the user to start | ||
| // lnd with native SQL enabled, as we don't currently migrate | ||
| // the invoices to the new database schema. |
There was a problem hiding this comment.
ah yes i see: if the user was pointing to a bbolt store and then changes config to sql store, there really isnt any way for us to know this so it is essentially a new node
| log.Debugf("Created SQL invoice hash index in %v", time.Since(t0)) | ||
|
|
||
| total := 0 | ||
| // Now we can start migrating the invoices. We'll do this in |
There was a problem hiding this comment.
just one more question that came to mind post-review:
does this actually reduce a bunch of memory usage since on the SQL DB side, it will still take a bunch of memory due to this all being in a single tx.. right?
iirc, we've done previous migrations where we do 1 tx per batch and then persist a check mark in case things go wrong. If anything goes wrong, we can just clear the tables and start again on the next attempt.
There was a problem hiding this comment.
the other thing is: if the migration succeeds successfully, should we not clear the "invoices" bucket in the kv store?
| sanitizedDSN, err := replacePasswordInDSN(cfg.Dsn) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| log.Infof("Using SQL database '%s'", sanitizedDSN) | ||
|
|
||
| rawDB, err := sql.Open("pgx", cfg.Dsn) | ||
| db, err := sql.Open("pgx", cfg.Dsn) |
There was a problem hiding this comment.
I'm curious about the reason for renaming the variable from rawDb to db.
There was a problem hiding this comment.
db, err := sql.Open("sqlite", dsn)
if err != nil { if err != nil {
return nil, err return nil, err
} }I've just noticed the var name here; consistency is indeed beneficial. 👍
| return s.BaseDB | ||
| } | ||
|
|
||
| // ApplyAllMigrations applices both the SQLC and custom in-code migrations to |
There was a problem hiding this comment.
spelling: applices -> applies, and 2 other instances where this function was implemented
| params := sqlc.InsertInvoiceParams{ | ||
| Hash: paymentHash[:], | ||
| AmountMsat: int64(invoice.Terms.Value), | ||
| CltvDelta: sqldb.SQLInt32( |
There was a problem hiding this comment.
Just noticed the comment: // Note: BOLT12 invoices don't have a final cltv delta. is no longer there
| // which is a monotonically increasing uint32. | ||
| invoiceBucket = []byte("invoices") | ||
|
|
||
| // paymentHashIndexBucket is the name of the sub-bucket within the |
There was a problem hiding this comment.
The comment references paymentHashIndexBucket, while the variable is actually named invoiceIndexBucket. Is this discrepancy intentional?
|
@ziggie1984: review reminder |
bhandras
force-pushed
the
sql-invoice-migration
branch
2 times, most recently
from
d2ab959 to
2ba414d
Compare
This commit adds the migration_tracker table which we'll use to track if a custom migration has already been done.
This commit introduces support for custom, in-code migrations, allowing a specific Go function to be executed at a designated database version during sqlc migrations. If the current database version surpasses the specified version, the migration will be skipped.
This commit separates the execution of SQL and in-code migrations from their construction. This change is necessary because, currently, the SQL schema is migrated during the construction phase in the lncfg package. However, migrations are typically executed when individual stores are constructed within the configuration builder.
Previously we intentially did not set settled_at and settle_index when inserting a new invoice as those fields are set when we settle an invoice through the usual invoice update. As migration requires that we set these nullable fields, we can safely add them.
Certain invoices may not have a deterministic payment hash. For such invoices we still store the payment hashes in our KV database, but we do not have a sufficient index to retrieve them. This PR adds such index to the SQL database that will be used during migration to retrieve payment hashes.
…a hash The current sqlc GetInvoice query experiences incremental slowdowns during the migration of large invoice databases, primarily due to its complex predicate set. For this specific use case, a streamlined GetInvoiceByHash function provides a more efficient solution, maintaining near-constant lookup times even with extensive table sizes.
This commit runs the invoice migration if the user has a KV SQL backend configured.
Previously we'd recalculate the paid amount by summing amounts of settled HTLCs. This approach while correct would stop the SQL migration process as some KV invoices may have incorrectly stored paid amounts.
bhandras
force-pushed
the
sql-invoice-migration
branch
from
2ba414d to
33b9112
Compare
| -- only run once. It tracks a global database version that encompasses both | ||
| -- schema migrations handled by golang-migrate and custom in-code migrations | ||
| -- for more complex data conversions that cannot be expressed in pure SQL. | ||
| CREATE TABLE IF NOT EXISTS migration_tracker ( |
There was a problem hiding this comment.
does this mean, this migration_tracker table is always greater or equal to the schema migration table of the sql table ? Because the in-code migration will lead to the version being greater but the version in this table cannot be lower than the version that the schema migration correct ?
| // version. If a migration includes a non-nil MigrationFn, it is executed after | ||
| // the SQL schema has been migrated to the corresponding schema version. |
There was a problem hiding this comment.
Does this still hold true if for every code migration will be done within a separate migration so that we do not have schema-migration and normal version in the same version ?
| type MigrationExecutor interface { | ||
| // CurrentSchemaVersion returns the current schema version of the | ||
| // database. | ||
| CurrentSchemaVersion() (int, error) |
There was a problem hiding this comment.
haven't seen a code part where this function is used ?
|
|
||
| // ExecuteMigrations runs migrations for the database, depending on the | ||
| // target given, either all migrations or up to a given version. | ||
| ExecuteMigrations(target MigrationTarget) error |
There was a problem hiding this comment.
Nit: Maybe add here in the comment that this function handles the schema and in-code migration, and maybe recommend that migrations schema or in-code should be done separately ?
| migrationConfig = []MigrationConfig{ | ||
| { | ||
| Name: "000001_invoices", | ||
| Version: 1, |
There was a problem hiding this comment.
does the schema version always correspond to the filename here, meaning that would we start with 000002 in the firt place the database version would start at 2 ?
|
|
||
| offset := uint64(0) | ||
| t0 := time.Now() | ||
| // Create the hash index which we will use to look up invoice |
invoices/testdata/channel.db
Outdated
There was a problem hiding this comment.
would it be difficult to inject in at runtime @bhandras
| // disabled, the regular native SQL store migrations will still | ||
| // run. If the database version is already above this custom | ||
| // migration's version (6), it will be skipped permanently, | ||
| // regardless of the flag. |
There was a problem hiding this comment.
Hmm so if the user disables this, and we add a new migration in the future (version 7) he will not be able to migrate the invoice ?
| // testNativeSQLNoMigration tests that nodes that have invoices would not start | ||
| // up with native SQL enabled, as we don't currently support migration of KV | ||
| // invoices to the new SQL schema. | ||
| func testNativeSQLNoMigration(ht *lntest.HarnessTest) { |
There was a problem hiding this comment.
Create an issue for this because this will be done in a followu up ?
| @@ -1576,13 +1576,6 @@ func fetchInvoiceData(ctx context.Context, db SQLInvoiceQueries, | |||
|
|
|||
| if len(htlcs) > 0 { | |||
| invoice.Htlcs = htlcs | |||
| var amountPaid lnwire.MilliSatoshi | |||
There was a problem hiding this comment.
Open an issue to investigate this behavior ?
| @@ -1576,13 +1576,6 @@ func fetchInvoiceData(ctx context.Context, db SQLInvoiceQueries, | |||
|
|
|||
| if len(htlcs) > 0 { | |||
| invoice.Htlcs = htlcs | |||
| var amountPaid lnwire.MilliSatoshi | |||
| for _, htlc := range htlcs { | |||
| if htlc.State == HtlcStateSettled { | |||
There was a problem hiding this comment.
I think we just need to adopt it and also add it if the htlc is in the accepted state, rather then removing it?
lnd/invoices/update_invoice.go
Lines 370 to 379 in 1b0f41d
or maybe remove it because it might be error pruning and we haven't tested every case.
There was a problem hiding this comment.
tested it locally it does work with this change:
for _, htlc := range htlcs {
if htlc.State == HtlcStateSettled ||
htlc.State == HtlcStateAccepted {
amountPaid += htlc.Amt
}
}
invoice.AmtPaid = amountPaid
Change Description
This pull request adds the migration of old key-value (KV) invoices to the new native SQL schema when the --db.use-native-sql flag is set, unless the --db.skip-sql-invoice-migration flag is also specified.
Please note that since we currently do not support running on mixed database backends for users of
bboltoretcd, an additional step is required to migrate their KV database to SQL first. For more context, please see lightninglabs/lndinit#21.