Skip to content

sqldb: address pre-existing code quality issues in transaction executor and utilities #10698

@ziggie1984

Description

@ziggie1984

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugUnintended code behavioursql

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions