-
Notifications
You must be signed in to change notification settings - Fork 2.3k
sqldb: address pre-existing code quality issues in transaction executor and utilities #10698
Description
Background
During review of #10697 (sqldb/v2), Gemini Code Assist flagged several issues.
All of them are pre-existing in sqldb (v1) and were carried over verbatim into
sqldb/v2. They are not regressions introduced by the v2 PR but are worth
addressing in both packages consistently.
Issues to fix
1. defer inside retry loop (interfaces.go)
In ExecuteSQLTransactionWithRetry, a defer tx.Rollback() is placed inside
the retry for loop. Deferred calls accumulate on the stack across retries and
only fire when the outer function returns, not at the end of each iteration.
While not a correctness bug (Rollback on an already-closed tx is a no-op), it
delays resource release under retries.
Fix: Wrap the loop body in a closure so the deferred rollback fires per
iteration.
Affected: sqldb/interfaces.go:296, sqldb/v2/interfaces.go:321
2. Unused randRetryDelay method (interfaces.go)
txExecutorOptions has a randRetryDelay() method that is never called. The
actual retry delay logic uses the package-level randRetryDelay function
instead.
Fix: Remove the unused method.
Affected: sqldb/interfaces.go:139, sqldb/v2/interfaces.go:163
3. No validation of MaxBatchSize in store constructors (paginate.go)
QueryConfig.Validate() guards against MaxBatchSize == 0 (which would cause
an infinite loop in ExecuteBatchedQuery), but neither NewSqliteStore nor
NewPostgresStore call Validate() on the provided config.
Fix: Call cfg.QueryConfig.Validate() inside both store constructors, or
ensure callers always validate before constructing a store.
Affected: sqldb/sqlite.go:58, sqldb/postgres.go:97 (and v2 equivalents)
4. getDatabaseNameFromDSN only handles URL-style DSNs (postgres.go)
The function uses url.Parse and only works with URL-format DSNs
(e.g. postgres://user:pass@host/db). Postgres also supports key=value
connection strings (e.g. host=localhost dbname=mydb), which would silently
return an empty database name.
Fix: Either add support for key=value DSN parsing or document the
URL-only requirement explicitly.
Affected: sqldb/postgres.go:75, sqldb/v2/postgres.go:85
Notes
- All four issues exist in both
sqldb(v1) andsqldb/v2— fixes should be
applied to both packages in the same PR for consistency. - These are low-severity issues; none cause incorrect behaviour under normal
usage.