Conversation
In the upcoming commits, we will introduce a new sqldb module, sqldb version 2. The intention of the new sqldb module, is to make it generalizable so that it contains no `lnd` specific code, to ensure that it can be reused in other projects. This commit adds the base of the new module, but does not include any implementation yet, as that will be done in the upcoming commits.
This commit moves all non lnd-specific code of sqldb/v1 to the new sqldb/v2 module. Note however, that without additional changes, this package still needs to reference lnd, as references to the lnd `sqlc` package is required without further changes. Those changes will be introduced in the upcoming commits, to fully decouple the new sqldb/v2 module from lnd.
This commit introduces a new struct named `MigrationStream`, which defines a structure for migrations SQL migrations. The `MigrationStream` struct contains the SQL migrations which will be applied, as well as corresponding post-migration code migrations which will be executed afterwards. The struct also contains fields which define how the execution of the migrations are tracked. Importantly, it is also possible to define multiple different `MigrationStream`s which are executed, to for example define one `prod` and one `dev` migration stream.
This commit updates the `sqldb/v2` package to utilize the new `MigrationStream` type for executing migrations, instead of passing `[]MigrationConfig`'s directly.
This commit updates the definition of the `BaseDB` struct to decouple it from lnd`s `sqlc` package. We also introduce new fields to the struct to make it possible to track the database type used at runtime.
In order to make it possible to replace `tapd`'s internal `sqldb` package with the new generic `sqldb/v2` package, we need to make sure that all features and functionality that currently exist in the `tapd` package are also present in the new `sqldb/v2` package. This commit adds such additional missing features to the `sqldb/v2` package.
Previously, the test db helper files were suffixed with "_test", which would indicate that the files specifically contained tests. However, these files actually contain helper functions to be used in tests, and are not tests themselves. To better reflect their purpose, the files have been renamed to instead be prefixed with "test_".
The docs of the `SqliteStore` for no sqlite build environments previously didn't clarify that the actual `SqliteStore` implementation under such build tag environments, didn't actually implement a real sqlite store. This commit clarifies that in the docs.
rename the `ErrRetriesExceeded` error to `ErrTxRetriesExceeded`.
Rename the `postgresErrMsgs` list to `postgresRetriableErrMsgs`, in order to clarify its usage.
The code previously used `defaultMaxOpenConns` for both the maximum number of open connections and the maximum number of idle connections in the `SqliteStore`. This commit updates the code to use `defaultMaxIdleConns` for the maximum number of idle connections.
The documentation for the `SqliteConfig.MaxConnections`, `PostgresConfig.MaxOpenConnections` and `PostgresConfig.MaxIdleConnections` previously stated that an unlimited number was used when the value was set to 0. This is not the case however, as setting the value to 0 will result in the default values being used.
This commit limits the MigrationExecutor interface due to the following reasoning: 1. SkipMigrations() and DefaultTarget() should not be on the interface Both are only used by ApplyAllMigrations, which immediately passes the results back into the same executor. They are internal implementation details and should be folded into ExecuteMigrations itself. 2. SetSchemaVersion and GetSchemaVersion are test-only but on the production interface Every caller of these in sqldb/v2 is in test files. The SetSchemaVersion comment even says "USE WITH CAUTION" — dangerous test utilities should not be on an interface that every real consumer must implement. They should be accessible on the concrete types only and used directly in tests without going through the interface. 3. ExecuteMigrations should not take a MigrationTarget parameter for the normal path On the normal startup path, callers just do executor.ExecuteMigrations(executor.DefaultTarget(), stream) — asking the executor for its default and handing it straight back. The method should run to latest by default; a version override for tests can live on the concrete type instead.
…ackage `sqldb/v2` as separate package
Summary of ChangesHello, 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 merges the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new version of the sqldb package, providing a database-agnostic abstraction layer for SQLite and Postgres. It features a transaction executor with retry logic, a migration framework supporting SQL and programmatic migrations, and utilities for paginated and batch queries. The feedback suggests improving test performance by using in-memory SQLite databases for unit tests instead of writing to disk.
| // TODO(roasbeef): if we pass :memory: for the file name, then we get | ||
| // an in mem version to speed up tests |
There was a problem hiding this comment.
Consider using :memory: for the database file name in tests to create an in-memory SQLite database. This can significantly speed up test execution by avoiding disk I/O. The current TODO comment acknowledges this, and it would be a valuable improvement.
| // TODO(roasbeef): if we pass :memory: for the file name, then we get | |
| // an in mem version to speed up tests | |
| dbFileName := ":memory:" |
| // TODO(roasbeef): if we pass :memory: for the file name, then we get | ||
| // an in mem version to speed up tests |
There was a problem hiding this comment.
Similar to NewTestSqliteDB, using :memory: for the database file name in NewTestSqliteDBWithVersion can improve test performance by utilizing an in-memory SQLite database. This aligns with the existing TODO comment.
| // TODO(roasbeef): if we pass :memory: for the file name, then we get | |
| // an in mem version to speed up tests | |
| dbFileName := ":memory:" |
2 participants
This PR merges the
sqldb-v2branch which includes the newsqldb/v2package intolnd's master branch.Note that
sqldb/v2is not yet used anywhere withinlnd, but has already been implemented into other repositories such as:https://github.com/lightninglabs/lightning-terminal
Ideally, this PR is merged prior to the next official
lndrelease.