[sql-32] accounts: add migration code from kvdb to SQL #1047

ViktorTigerstrom · 2025-04-22T14:59:16Z

This PR introduces the migration logic for transitioning the accounts store from kvdb to SQL.

Note that as of this PR, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

Part of #917

ellemouton

Soooooo excited to see this come through! I couldnt help but take a look even though review not requested - just leaving some drive by comments in the mean time :)

I'll wait till review requested before looking again :)

ellemouton · 2025-04-22T15:34:11Z

accounts/kv_sql_migration_test.go

+
+			t.Run("Postgres", func(t *testing.T) {
+				migrationTest(t, kvStore, testClock, false)
+			})
+
+			t.Run("SQLite", func(t *testing.T) {
+				migrationTest(t, kvStore, testClock, true)


in Lit, we've gone with the build flag approach. So can just use the existing NewTestDB methods no? and then if bbolt backend, just skip the test ?

Updated as discussed offline :)

ellemouton · 2025-04-22T15:34:42Z

accounts/store_sql.go

@@ -732,6 +733,79 @@ func (s *SQLStore) StoreLastIndexes(ctx context.Context, addIndex,
 	})
 }

+func makeInsertAccountParams(account *OffChainBalanceAccount) (


let's keep the migration code in the same file

Ok, yeah agree :)! Had it here previously to mimic lnd's migration implementation.

accounts/kv_sql_migration_test.go

ellemouton · 2025-04-22T15:35:54Z

accounts/kv_sql_migration_test.go

+	pgFixture := db.NewTestPgFixture(
+		t, db.DefaultPostgresFixtureLifetime, true,
+	)
+	t.Cleanup(func() {
+		pgFixture.TearDown(t)
+	})
+
+	makeSQLDB := func(t *testing.T, clock *clock.TestClock,
+		sqlite bool) (*SQLStore, *db.TransactionExecutor[SQLQueries]) {
+
+		var store *SQLStore
+
+		if sqlite {
+			store = NewSQLStore(db.NewTestSqliteDB(t).BaseDB, clock)
+		} else {
+			store = NewSQLStore(
+				db.NewTestPostgresDB(t).BaseDB, clock,
+			)


see other comment about using the existing infrastructure in LiT that uses build flags instead

Thanks, updated!

ellemouton · 2025-04-22T15:40:57Z

accounts/kv_sql_migration_test.go

+			// If the db doesn't have any indices, we can't compare
+			// them.


if the kvstore didnt populate during test set up though, then we will silently exit here. Rather have a test param like "expectLastIndex" so that we know for sure if we are getting the right result here

Ah good catch, I actually forgot to store the indexes when i added invoices, so that caught that 🚀!

ellemouton · 2025-04-22T15:52:39Z

accounts/sql_migration.go

+		overrideAccountTimeZone(kvAccount)
+		overrideAccountTimeZone(migratedAccount)
+
+		if !reflect.DeepEqual(kvAccount, migratedAccount) {


i worry about the direct comparisons here because i think it can fail if the AccountInvoice and AccountPayments maps are large since we cannot guarantee order of the maps.

So adding to this, i think it would be worth it to have a test that generates a large number of invoices & payments (and maybe even many random accounts) - just so that we can really make sure that we arent missing things here. So basically a test case that is not deterministic and does more of a fuzzing style test

Added randomization tests to address this :)!

In preparation for upcoming migration tests from a kvdb to an SQL store, this commit updates the NewTestDB function to return the Store interface rather than a concrete store implementation. This change ensures that migration tests can call NewTestDB under any build tag while receiving a consistent return type.

ViktorTigerstrom · 2025-04-24T09:34:13Z

accounts/kv_sql_migration_test.go

+		{
+			"randomized accounts",
+			true,
+			randomizeAccounts,
+		},
+		{
+			"rapid randomized accounts",
+			true,
+			rapidRandomizeAccounts,
+		},


I added 2 tests here currently, where one uses rapid for the randomisation, and one uses the normal "golang.org/x/exp/rand" testing semi-randomisation.

The drawback with the rapid test, is that we can't really generate too many invoices and and payments per account, as you can't really limit the amount of times the rapid.Check runs in golang (i.e. the number of accounts we populate the db with), without limiting the amount of runs the Check function would run for the entire litd project. I.e. if we limited it for this test, that would impact any other tests in the future that uses the rapid.Check function. There is an open issue for this in the rapid lib, see:
flyingmutant/rapid#38

When running the migration locally, the test migration execution time really goes up if you have a lot of accounts with a lot of payments and invoices, and therefore I opted to not add such a test to not make the make unit execution time take too long locally.

Therefore, I also added a normal test that doesn't use rapid for randomization, so that test adds up to 1000 invoices and payments.

In the end though , I can delete one of the tests if you prefer any test over the other :)!

I think I prefer randomizeAccounts since it's more concise and performant, great you tested out both approaches 👍

Cool, thanks for the input! Would he interested to hear Elle's opinion here as well :)

This commit introduces the migration logic for transitioning the accounts store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

bitromortac

Awesome work 🚀! Will certainly need another pass, since I haven't reviewed many migration PRs

bitromortac · 2025-04-25T07:43:56Z

accounts/kv_sql_migration_test.go

@@ -0,0 +1,348 @@
+package accounts


nit: I think it would be better to name it sql_migration_test.go

bitromortac · 2025-04-25T07:53:53Z

accounts/sql_migration.go

+		AccountID:      sqlID,
+		Hash:           hash[:],
+		Status:         int16(entry.Status),
+		FullAmountMsat: int64(entry.FullAmount),


an overflow is very unrealistic here, right?

Yeah I think we have to make a choice here, and for migrations in general:
We can either:

Assume that since this is a migration, the data we are migrating must have already been sanity checked and rule checked during the initial insertion into the kvdb, and therefore errors like that shouldn't be possible to occur during the migration.

We sanity check and check the rules again for all data during the migration.

I have opted for option 1 here and for upcoming migrations, as I do think that should be the case, and we'd only let the migrations use more resources during the migration if we chose option 2 instead.

But I'd very interested to hear from both of you if you think we should instead go with option 2 here.

bitromortac · 2025-04-25T07:57:48Z

accounts/sql_migration.go

+		migratedAccountID, err := getAccountIDByAlias(
+			ctx, tx, kvAccount.ID,
+		)
+		if err != nil {
+			return fmt.Errorf("unable to fetch migrated "+
+				"account(%v): %w", kvAccount.ID, err)
+		}


you could return the sql id from migrateSingleAccountToSQL perhaps

bitromortac · 2025-04-25T08:16:20Z

accounts/sql_migration.go

+func makeSetAddIndexParams(indexValue uint64) (sqlc.SetAccountIndexParams,
+	error) {
+
+	return makeSetAccountIndexParams(indexValue, addIndexName)
+}
+
+func makeSetSettleIndexParams(indexValue uint64) (sqlc.SetAccountIndexParams,
+	error) {
+
+	return makeSetAccountIndexParams(indexValue, settleIndexName)
+}


nit: I think those aren't really adding a lot and could use makeSetAccountIndexParams directly, but curious about your motivation

bitromortac · 2025-04-25T08:22:45Z

accounts/kv_sql_migration_test.go

+// backed to a SQL database. Note that this test does not attempt to be a
+// complete migration test.


Could you clarify what is missing to be a complete migration test?

What I mean here is that we are not really testing that the accounts are fully functional after the migration has been done, nor that any linked sessions etc. work properly.

That'd be more suitable in an itest instead :)!

bitromortac · 2025-04-25T09:21:36Z

accounts/kv_sql_migration_test.go

+		{
+			"randomized accounts",
+			true,
+			randomizeAccounts,
+		},
+		{
+			"rapid randomized accounts",
+			true,
+			rapidRandomizeAccounts,
+		},


I think I prefer randomizeAccounts since it's more concise and performant, great you tested out both approaches 👍

ViktorTigerstrom added the no-changelog This PR is does not require a release notes entry label Apr 22, 2025

ellemouton reviewed Apr 22, 2025

View reviewed changes

ViktorTigerstrom force-pushed the 2025-04-migrate-accounts branch from 3072e48 to b4cffd8 Compare April 24, 2025 09:12

ViktorTigerstrom commented Apr 24, 2025

View reviewed changes

ViktorTigerstrom requested review from ellemouton and bitromortac April 24, 2025 09:37

levmi assigned ViktorTigerstrom Apr 24, 2025

ViktorTigerstrom added 2 commits April 24, 2025 22:14

accounts: add migration code from kvdb to SQL

db845aa

This commit introduces the migration logic for transitioning the accounts store from kvdb to SQL. Note that as of this commit, the migration is not yet triggered by any production code, i.e. only tests execute the migration logic.

accounts: add randomized accounts migration tests

8cd1631

ViktorTigerstrom force-pushed the 2025-04-migrate-accounts branch from b4cffd8 to 8cd1631 Compare April 24, 2025 15:14

bitromortac reviewed Apr 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sql-32] accounts: add migration code from kvdb to SQL #1047

[sql-32] accounts: add migration code from kvdb to SQL #1047

ViktorTigerstrom commented Apr 22, 2025

ellemouton left a comment

ellemouton Apr 22, 2025

ViktorTigerstrom Apr 24, 2025

ellemouton Apr 22, 2025

ViktorTigerstrom Apr 24, 2025

ellemouton Apr 22, 2025

ViktorTigerstrom Apr 24, 2025

ellemouton Apr 22, 2025

ViktorTigerstrom Apr 24, 2025

ellemouton Apr 22, 2025

ViktorTigerstrom Apr 24, 2025

ViktorTigerstrom Apr 24, 2025

bitromortac Apr 25, 2025

ViktorTigerstrom Apr 25, 2025

bitromortac left a comment

bitromortac Apr 25, 2025

bitromortac Apr 25, 2025

ViktorTigerstrom Apr 25, 2025

bitromortac Apr 25, 2025

bitromortac Apr 25, 2025

bitromortac Apr 25, 2025

ViktorTigerstrom Apr 25, 2025

bitromortac Apr 25, 2025

		// If the db doesn't have any indices, we can't compare
		// them.

		// backed to a SQL database. Note that this test does not attempt to be a
		// complete migration test.

[sql-32] accounts: add migration code from kvdb to SQL #1047

Are you sure you want to change the base?

[sql-32] accounts: add migration code from kvdb to SQL #1047

Conversation

ViktorTigerstrom commented Apr 22, 2025

ellemouton 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac 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

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

Choose a reason for hiding this comment