Skip to content

Commit

Permalink
remove superfluous use of runtime.SetFinalizer on SQLiteRows
Browse files Browse the repository at this point in the history
The commit removes the use of runtime.SetFinalizer to finalize
SQLiteRows since only serves to close the associated SQLiteStmt which
already has a registered finalizer.

It also fixes a race and potential panic in SQLiteRows.Close around the
SQLiteRows.s field (*SQLiteStmt) which is accessed without a mutex being
held, but modified with it held (null'd out). Further the mutex we are
holding is that of the SQLiteStmt so a subsequent call to Close will
cause a panic sine it'll attempt to dereference a nil field. The fix
here is to add a mutex for closing to SQLiteRows.

Since we now also set the s field to nil when closing this commit
removes the "closed" field (since checking if s is nil is the same) and
also changes the type of "nc" (number of columns) to an int32 so that we
can pack the nc and cls fields, and add the close mutex without making
the struct any bigger.

```
goos: darwin
goarch: arm64
pkg: github.com/charlievieth/go-sqlite3
cpu: Apple M4 Pro
                                          │   x1.txt    │               x4.txt                │
                                          │   sec/op    │   sec/op     vs base                │
Suite/BenchmarkExec/Params-14               719.2n ± 2%   716.9n ± 1%        ~ (p=0.897 n=10)
Suite/BenchmarkExec/NoParams-14             506.5n ± 3%   500.1n ± 0%   -1.25% (p=0.002 n=10)
Suite/BenchmarkExecContext/Params-14        1.584µ ± 0%   1.567µ ± 1%   -1.07% (p=0.007 n=10)
Suite/BenchmarkExecContext/NoParams-14      1.524µ ± 1%   1.524µ ± 1%        ~ (p=0.539 n=10)
Suite/BenchmarkExecStep-14                  443.9µ ± 3%   441.4µ ± 0%   -0.55% (p=0.011 n=10)
Suite/BenchmarkExecContextStep-14           447.8µ ± 1%   442.9µ ± 0%   -1.10% (p=0.000 n=10)
Suite/BenchmarkExecTx-14                    1.643µ ± 1%   1.640µ ± 0%        ~ (p=0.642 n=10)
Suite/BenchmarkQuery-14                     1.968µ ± 3%   1.821µ ± 1%   -7.52% (p=0.000 n=10)
Suite/BenchmarkQuerySimple-14               1.207µ ± 2%   1.040µ ± 1%  -13.84% (p=0.000 n=10)
Suite/BenchmarkQueryContext/Background-14   2.400µ ± 1%   2.320µ ± 0%   -3.31% (p=0.000 n=10)
Suite/BenchmarkQueryContext/WithCancel-14   8.847µ ± 5%   8.512µ ± 4%   -3.79% (p=0.007 n=10)
Suite/BenchmarkParams-14                    2.131µ ± 2%   1.967µ ± 1%   -7.70% (p=0.000 n=10)
Suite/BenchmarkStmt-14                      1.444µ ± 1%   1.359µ ± 1%   -5.89% (p=0.000 n=10)
Suite/BenchmarkRows-14                      61.57µ ± 1%   60.24µ ± 1%   -2.16% (p=0.000 n=10)
Suite/BenchmarkStmtRows-14                  60.15µ ± 1%   59.08µ ± 1%   -1.78% (p=0.000 n=10)
Suite/BenchmarkQueryParallel-14             960.9n ± 1%   420.8n ± 2%  -56.21% (p=0.000 n=10)
geomean                                     4.795µ        4.430µ        -7.62%
```
  • Loading branch information
charlievieth authored and mattn committed Dec 9, 2024
1 parent ab13d63 commit c61eeb5
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 23 deletions.
48 changes: 25 additions & 23 deletions sqlite3.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ type SQLiteStmt struct {
s *C.sqlite3_stmt
t string
closed bool
cls bool
cls bool // True if the statement was created by SQLiteConn.Query
}

// SQLiteResult implements sql.Result.
Expand All @@ -393,12 +393,12 @@ type SQLiteResult struct {
// SQLiteRows implements driver.Rows.
type SQLiteRows struct {
s *SQLiteStmt
nc int
nc int32 // Number of columns
cls bool // True if we need to close the parent statement in Close
cols []string
decltype []string
cls bool
closed bool
ctx context.Context // no better alternative to pass context into Next() method
closemu sync.Mutex
}

type functionInfo struct {
Expand Down Expand Up @@ -2008,14 +2008,12 @@ func (s *SQLiteStmt) query(ctx context.Context, args []driver.NamedValue) (drive

rows := &SQLiteRows{
s: s,
nc: int(C.sqlite3_column_count(s.s)),
nc: int32(C.sqlite3_column_count(s.s)),
cls: s.cls,
cols: nil,
decltype: nil,
cls: s.cls,
closed: false,
ctx: ctx,
}
runtime.SetFinalizer(rows, (*SQLiteRows).Close)

return rows, nil
}
Expand Down Expand Up @@ -2112,34 +2110,38 @@ func (s *SQLiteStmt) Readonly() bool {

// Close the rows.
func (rc *SQLiteRows) Close() error {
rc.s.mu.Lock()
if rc.s.closed || rc.closed {
rc.s.mu.Unlock()
rc.closemu.Lock()
defer rc.closemu.Unlock()
s := rc.s
if s == nil {
return nil
}
rc.s = nil // remove reference to SQLiteStmt
s.mu.Lock()
if s.closed {
s.mu.Unlock()
return nil
}
rc.closed = true
if rc.cls {
rc.s.mu.Unlock()
return rc.s.Close()
s.mu.Unlock()
return s.Close()
}
rv := C.sqlite3_reset(rc.s.s)
rv := C.sqlite3_reset(s.s)
if rv != C.SQLITE_OK {
rc.s.mu.Unlock()
return rc.s.c.lastError()
s.mu.Unlock()
return s.c.lastError()
}
rc.s.mu.Unlock()
rc.s = nil
runtime.SetFinalizer(rc, nil)
s.mu.Unlock()
return nil
}

// Columns return column names.
func (rc *SQLiteRows) Columns() []string {
rc.s.mu.Lock()
defer rc.s.mu.Unlock()
if rc.s.s != nil && rc.nc != len(rc.cols) {
if rc.s.s != nil && int(rc.nc) != len(rc.cols) {
rc.cols = make([]string, rc.nc)
for i := 0; i < rc.nc; i++ {
for i := 0; i < int(rc.nc); i++ {
rc.cols[i] = C.GoString(C.sqlite3_column_name(rc.s.s, C.int(i)))
}
}
Expand All @@ -2149,7 +2151,7 @@ func (rc *SQLiteRows) Columns() []string {
func (rc *SQLiteRows) declTypes() []string {
if rc.s.s != nil && rc.decltype == nil {
rc.decltype = make([]string, rc.nc)
for i := 0; i < rc.nc; i++ {
for i := 0; i < int(rc.nc); i++ {
rc.decltype[i] = strings.ToLower(C.GoString(C.sqlite3_column_decltype(rc.s.s, C.int(i))))
}
}
Expand Down
18 changes: 18 additions & 0 deletions sqlite3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2111,6 +2111,7 @@ var benchmarks = []testing.InternalBenchmark{
{Name: "BenchmarkStmt", F: benchmarkStmt},
{Name: "BenchmarkRows", F: benchmarkRows},
{Name: "BenchmarkStmtRows", F: benchmarkStmtRows},
{Name: "BenchmarkQueryParallel", F: benchmarkQueryParallel},
}

func (db *TestDB) mustExec(sql string, args ...any) sql.Result {
Expand Down Expand Up @@ -2568,3 +2569,20 @@ func benchmarkStmtRows(b *testing.B) {
}
}
}

func benchmarkQueryParallel(b *testing.B) {
b.RunParallel(func(pb *testing.PB) {
db, err := sql.Open("sqlite3", ":memory:")
if err != nil {
panic(err)
}
db.SetMaxOpenConns(runtime.NumCPU())
defer db.Close()
var i int64
for pb.Next() {
if err := db.QueryRow("SELECT 1, 2, 3, 4").Scan(&i, &i, &i, &i); err != nil {
panic(err)
}
}
})
}

0 comments on commit c61eeb5

Please sign in to comment.