sqldb/v2: introduce sqldb/v2 module#10697
sqldb/v2: introduce sqldb/v2 module#10697yyforyongyu merged 18 commits intolightningnetwork:masterfrom
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.
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 introduces 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 for SQLite and Postgres backends. It includes comprehensive support for migrations, transaction execution with automatic retry logic, and utilities for paginated queries. The review feedback identifies several important improvements: addressing a potential resource leak caused by using defer inside a retry loop, removing unused code in the transaction executor, preventing potential infinite loops in pagination through better configuration validation, and expanding Postgres DSN parsing to support non-URL connection strings.
| defer func() { | ||
| _ = tx.Rollback() | ||
| }() |
There was a problem hiding this comment.
Using defer inside a loop is discouraged as it can lead to resource exhaustion. Deferred functions are only executed when the surrounding function returns, not at the end of each loop iteration. In this retry loop, if multiple attempts are made due to serialization errors, multiple rollback functions will be queued on the stack. This can cause memory growth and delayed release of database resources. Consider wrapping the loop body in a closure to ensure the deferred rollback runs at the end of each iteration.
There was a problem hiding this comment.
let's fix this in a follow-up
| func (t *txExecutorOptions) randRetryDelay() time.Duration { | ||
| return time.Duration(rand.Int63n(int64(t.maxRetryDelay))) //nolint:gosec | ||
| } |
There was a problem hiding this comment.
The method randRetryDelay on txExecutorOptions appears to be unused within this package. The logic for calculating retry delays is instead implemented in the package-level function randRetryDelay at line 220, which is used by ExecuteSQLTransactionWithRetry. Consider removing this unused method to maintain code clarity.
| } | ||
|
|
||
| // Process items in pages. | ||
| for i := 0; i < len(inputItems); i += int(cfg.MaxBatchSize) { |
There was a problem hiding this comment.
If cfg.MaxBatchSize is zero, this loop will result in an infinite loop. While QueryConfig.Validate checks for this, the NewSqliteStore and NewPostgresStore functions do not currently enforce validation of the provided configuration. It is recommended to ensure MaxBatchSize is positive before entering the loop or enforce validation during store initialization.
| func getDatabaseNameFromDSN(dsn string) (string, error) { | ||
| // Parse the DSN as a URL | ||
| u, err := url.Parse(dsn) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| // The database name is the last segment of the path. Trim leading slash | ||
| // and return the last segment. | ||
| return path.Base(u.Path), nil | ||
| } |
There was a problem hiding this comment.
The getDatabaseNameFromDSN function assumes the DSN is always in URL format. Postgres also supports a 'key=value' connection string format (e.g., host=localhost user=test dbname=mydb). For such DSNs, url.Parse will not correctly extract the database name, which may cause issues during migration initialization. Consider adding support for parsing 'key=value' DSNs or documenting that only URL-style DSNs are supported.
|
replaces #10681 |
3 participants
Summary
This PR introduces the
sqldb/v2module, a standalone Go module extractedfrom the existing
sqldbpackage. It contains only the non-lnd-specificdatabase infrastructure code, making it reusable across lnd and tapd.
This branch is a clean rebase of
origin/sqldb-v2onto currentmaster.It contains exactly the 18 commits from that feature branch and nothing else,
so reviewers can verify no unrelated changes are introduced.
Commits (oldest → newest)
sqldb/v2: add base for sqldb/v2 modulesqldb/v2: move all non lnd-specific v1 code to v2sqldb/v2: introduce MigrationStreamsqldb/v2: Use MigrationStream for migrationssqldb/v2: introduce sqldb/v2 BaseDBsqldb/v2: sync features with tapd's sqldb packagesqldb/v2: rename test db helper filessqldb/v2: clarify no_sqlite SqliteStore intentsqldb/v2: set predictable NewTestPgFixture container namesqldb/v2: move Sqlite test helpers to separate filesqldb/v2: rename ErrRetriesExceeded errorsqldb/v2: rename the postgresErrMsgs listsqldb/v2: use defaultMaxIdleConns in SqliteStoresqldb/v2: ensure SqliteConfig.MaxConnections is usedsqldb/v2: clarify config options docssqldb/v2: add MaxIdleConnections & ConnMaxLifetime sqlite optssqldb/v2: rename MigrationConfig to MigrationDescriptorsqldb/v2: limit MigrationExecutor interfaceTest plan
make buildpassesmake lintpassessqldb/v2package pass