-
Notifications
You must be signed in to change notification settings - Fork 106
[sql-46] trigger kvdb to sql migration using sqldb/v2
#1114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[sql-46] trigger kvdb to sql migration using sqldb/v2
#1114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @ViktorTigerstrom, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request represents a significant architectural shift, laying the groundwork for litd
to transition its data persistence from BoltDB to a SQL backend. By integrating sqldb/v2
and implementing code migrations, it enables the seamless transfer of existing data for key components like accounts, sessions, and firewall rules within dev
environments. This change facilitates future development on a more scalable and flexible data layer, while ensuring data integrity during the transition.
Highlights
- KVDB to SQL Migration: I've implemented the core logic to migrate existing BoltDB (KVDB) data for
accounts
,sessions
, andfirewalldb
stores to a SQL database. This is a 'code migration' that runs as a post-migration step, ensuring existing data is transferred to the new schema. sqldb/v2
Integration: This PR integrates the newsqldb/v2
library for database management. This includes adopting its robust transaction execution pattern and migration framework across theaccounts
,session
, andfirewalldb
modules.- Separate Dev Migrations: I've introduced a mechanism for
dev
builds to have their own SQL migration streams. This allows for experimental or in-progress migrations, such as the KVDB to SQL data migration, to be tested and developed without affecting production builds. - Database Schema Adaptation: Various database interaction layers, including SQLC generated code, store interfaces, and test helpers, have been updated. This aligns them with the new
sqldb/v2
patterns and introducessqlcmig6
types specifically for handling the migration's intermediate schema.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is a substantial pull request that migrates the database backend from kvdb
to sqldb/v2
and introduces the necessary migration logic. The changes are well-structured, particularly the use of a separate sqlcmig6
package for migration-specific queries and the centralized migration stream handling. I've identified one critical issue that will prevent compilation, and a couple of medium-severity issues related to error message formatting. After addressing these points, the code should be in good shape.
for i := range len(query) { | ||
if query[i] != '?' { | ||
sb.WriteByte(query[i]) | ||
continue | ||
} | ||
|
||
sb.WriteString("$") | ||
sb.WriteString(strconv.Itoa(argNum)) | ||
argNum++ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop for i := range len(query)
is not valid Go syntax and will cause a compilation error. range
cannot be used on an integer value.
A direct fix is to use a standard C-style for loop: for i := 0; i < len(query); i++
.
However, a more idiomatic way to iterate over a string in Go is for _, r := range query
, which correctly handles Unicode characters. Since you are using WriteByte
, the C-style loop is a safe and direct replacement.
for i := range len(query) { | |
if query[i] != '?' { | |
sb.WriteByte(query[i]) | |
continue | |
} | |
sb.WriteString("$") | |
sb.WriteString(strconv.Itoa(argNum)) | |
argNum++ | |
} | |
for i := 0; i < len(query); i++ { | |
if query[i] != '?' { | |
sb.WriteByte(query[i]) | |
continue | |
} | |
sb.WriteString("$") | |
sb.WriteString(strconv.Itoa(argNum)) | |
argNum++ | |
} |
config_dev.go
Outdated
return stores, fmt.Errorf("error applying "+ | ||
"migrations to SQLlite store: %w", err, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return stores, fmt.Errorf("error applying "+ | ||
"migrations to Postgres store: %w", err, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
78d4b40
to
01695d5
Compare
In upcoming commits, we will introduce support for sqldb/v2 in Lightning Terminal, in order to support code migrations. This is needed, so that we can ensure that the kvdb to sql migration is run exactly after an exact sql migration version, and not after all sql migrations have been executed. The reason why that's important is that sql table definition may change with future sql migrations, while the kvdb to sql migration will remain the same and always expect the same table definition. Therefore, if the kvdb to sql migration is run after all sql migrations, it may fail due to a table definition mismatch. This commit adds the sqldb/v2 dependency to `litd`, so that we can implement it's usage and execution of the kvdb to sql migration correctly in the upcoming commits. NOTE: currently the sqldb/v2 dependency hasn't been shipped in any lnd release, so this dependency currently a forked version of sqldb/v2. Therefore to use the sqldb/v2 dependency, you need to link the dependency to your local lnd fork, which has the sqldb/v2 code in it.
A core component of `sqldb/v2` useage, is that the package allows and requires that the callsite defines a `sqldb.MigrationStream` that will be run during the initialization of the `sqldb/v2 database instance. The `sqldb.MigrationStream` defines the exact sql migrations to run, as well as additional code migrations will be run after each individual migration version. This commit introduces the core definition of the migration stream for `litd`, and will be further exteded in upcoming commits that will introduce kvdb to sql code migration. Note that as of this commit, as no part of the `litd` codebase uses the `sqldb/v2` database instances, this migration stream is not yet used.
The `sqldb/v2` package now provides a definition for the `BackendType` type. As useage of `sqldb/v2` requires useage of that type, we update `litd` to use the `BackendType` definition from `sqldb/v2`, instead of it's own definition.
The upcoming implementation of `sqldb/v2` will extensively create a `Queries` object on the fly. To make more intuitive how to create the queries object for specific database types, we introduce a `NewForType` helper method. This also mimics how `tapd` creates the `Queries` object, and in order not let `litd` have it's own definition of how `Queries` object are created on the fly, the upcoming `sqldb/v2` usage will utilize this helper method.
As we will change the `accounts`, `session` & `firewalldb` packages to use the `sqldb/v2` package, we need to make those packages use the `sqldb/v2` `PostgresStore` when setting up their test postgres databases, instead of `litd`'s own `PostgresStore` version. In order to enable that functionality, we add a new helper function that creates a `PostgresStore` using the `sqldb/v2` package, in addition to helper function that creates a `PostgresStore` using the `litd` version. Once we have shifted all of `litd`'s code to use the `sqldb/v2` definition, we will remove the `litd` version.
Update the accounts package to use the `sqldb/v2` package instead of the older version.
Update the session package to use the `sqldb/v2` package instead of the older version.
Previous commits had forgotten to add the `ListAllKVStoresRecords` query to the `firewalldb.SQLKVStoreQueries` interface. As that is required to make the query useable when defining the `sqldb/v2` `TransactionExecutor` for the `firewalldb` package, this commit adds it to the interface.
rename `sqlStore` to `store` in the firewalldb sql migration test file, to make the name shorted. This is done in preparation for future commits which will lengthen the lines where `sqlStore` is used, which otherwise would make the lines exceed the 80 character limit.
Update the firewalldb package to use the `sqldb/v2` package instead of the older version.
As we've now switched over to using sqldb v2 for most of the db objects, we can remove a lot of deprecated code that's no longer used in the litd project. This commit removes that code.
As the legacy `NewTestPostgresDB` function is no longer used and has been removed, it no longer makes sense to have a `V2` suffix on the `NewTestPostgresV2DB` function. This commit renames it to `NewTestPostgresDB`, to indicate that this function now replaces the legacy function.
This commit introduces the `sqlcmig6` package, which at the time of this commit contains the same queries and models as `sqlc` package. Importantly though, once the kvdb to sql migration is made available in production, the `sqlcmig6` package will not change, as it is intended to represent the sql db as it was at the time of the migration. The sqlcmig6 package is therefore intended to be used in the kvdb to sql migration code, as it is will always be compatible with the sql database when all sql migrations prior to the kvdb to sql migration are applied. When additional sql migrations are added in the future, they may effect the `sqlc` package in such a way that the standard `sqlc` queries and models aren't compatible with kvdb to sql migration code any longer. By preserving the `sqlcmig6` package, we ensure that the kvdb to sql migration code can always use the same queries and models that were available at the time of the migration, even if the `sqlc` package changes in the future. Note that the `sqlcmig6` package have not been generated by `sqlc` (the queries and models are copied from the `sqlc` package), as it is not intended to be changed in the future.
Add the `sqlcmig6` defintion of the the accounts queries to the accounts package, along with a transaction executor that uses those queries. Note that while the standard Queiries interface, that use the standard sqlc queries, may change, the `SQLMig6Queries` interface is intended to remain the same.
This commit updates the accounts package to use the new `sqlcmig6` package for kvdb to SQL migration.
Add the `sqlcmig6` defintion of the the session queries to the session package, along with a transaction executor that uses those queries. Note that while the standard Queiries interface, that use the standard sqlc queries, may change, the `SQLMig6Queries` interface is intended to remain the same.
This commit updates the session package to use the new `sqlcmig6` package for kvdb to SQL migration.
Add the `sqlcmig6` defintion of the the firewalldb queries to the firewalldb package, along with a transaction executor that uses those queries. Note that while the standard Queiries interface, that use the standard sqlc queries, may change, the `SQLMig6Queries` interface is intended to remain the same.
As the firewalldb package kvdb to sql migration tests creates `sqlc` models to assert the migration results, we will need to update those call sites to instead use the `sqlcmig6` models instead, in order to be compatible with the `SQLMig6Queries` queries. However, since we can't update the `SQLDB` methods to use `sqlcmig6` models as params, we need to update the test code assertion to instead use the `SQLQueries` object directly instead of the `SQLDB` object. This makes it easy to swap that `SQLQueries` object to a `SQLMig6Queries` object in the commit that updates the firewalldb package to use the `sqlcmig6` package for the kvdb to sql migration.
This commit updates the firewalldb package to use the new `sqlcmig6` package for kvdb to SQL migration.
In upcoming commits, other packages than the `db` package will need to be able to access the `SqlSchemas`. This commit exports it in preparation for those changes
In upcoming commits, we will introduce a new migration stream package that will need to reference the db package, as well as the accounts, session and firewalldb package in future commits. To avoid circular dependencies, we therefore introduce a new migration stream that unit tests can use, in order to avoid having to import the new migration stream package.
The upcoming kvdb to sql code migration will be added to as part of the `sqldb/v2` migration stream. However, since the kvdb to sql migration will need to use the migration functions present in the `accounts`, `firewalldb`, and `session` packages, the migration will need to referernce those packages. That would lead to a circular dependency though if the migration stream was defined in the `db` package, as those packages need to import the `db` package. To avoid this, we introduce a new `migrationstreams` package that contains the migration streams, and ensure that the `db` package doesn't import the `migrationstreams` package.
When the kvdb to sql migration is initially introduced, we will want to ensure that it is only run under dev builds during the testing phase. We therefore introduce the functionality to have separate dev migrations, which are only included in a separate migration stream that is used only in dev builds. Note that these dev migrations are currently not included in the `sqlc.yaml` file, to ensure that the main sqlc models doesn't include the dev migrations.
The next commit that adds the kvdb to sql code migration will need to initialize the accounts kvdb DB. This commit exports that struct so that it can be referenced outside of the accounts package.
Add the necessary code to trigger the kvdb to sql code migration in dev builds.
01695d5
to
a12d714
Compare
Based on and blocked by #1113
Implements step 3. of "Phase 3" in #917.
This PR implements the functionality to use
sqldb/v2
to trigger the kvdb to sql migration under thedev
build-taggedlitd
builds, using code migrations whichsqldb/v2
adds support for. This PR also adds support for having separatedev
sql migrations, which are only run under build-taggedlitd
builds.Note that this is only a draft PR for now, as the sqldb/v2 is not released, and we therefore need to add forked+local dependencies in order to run this. Once we have an official release of the sqldb/v2 package, I'll update this PR to use that dependency.