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) and sqldb/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.
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 intosqldb/v2. They are not regressions introduced by the v2 PR but are worthaddressing in both packages consistently.
Issues to fix
1.
deferinside retry loop (interfaces.go)In
ExecuteSQLTransactionWithRetry, adefer tx.Rollback()is placed insidethe retry
forloop. Deferred calls accumulate on the stack across retries andonly 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:3212. Unused
randRetryDelaymethod (interfaces.go)txExecutorOptionshas arandRetryDelay()method that is never called. Theactual retry delay logic uses the package-level
randRetryDelayfunctioninstead.
Fix: Remove the unused method.
Affected:
sqldb/interfaces.go:139,sqldb/v2/interfaces.go:1633. No validation of
MaxBatchSizein store constructors (paginate.go)QueryConfig.Validate()guards againstMaxBatchSize == 0(which would causean infinite loop in
ExecuteBatchedQuery), but neitherNewSqliteStorenorNewPostgresStorecallValidate()on the provided config.Fix: Call
cfg.QueryConfig.Validate()inside both store constructors, orensure callers always validate before constructing a store.
Affected:
sqldb/sqlite.go:58,sqldb/postgres.go:97(and v2 equivalents)4.
getDatabaseNameFromDSNonly handles URL-style DSNs (postgres.go)The function uses
url.Parseand only works with URL-format DSNs(e.g.
postgres://user:pass@host/db). Postgres also supportskey=valueconnection strings (e.g.
host=localhost dbname=mydb), which would silentlyreturn an empty database name.
Fix: Either add support for
key=valueDSN parsing or document theURL-only requirement explicitly.
Affected:
sqldb/postgres.go:75,sqldb/v2/postgres.go:85Notes
sqldb(v1) andsqldb/v2— fixes should beapplied to both packages in the same PR for consistency.
usage.