Skip to content

Update NewPool logic in regards to poolsize > 1 with inmemory set #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions go.work.sum
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
github.com/google/pprof v0.0.0-20221118152302-e6195bd50e26 h1:Xim43kblpZXfIBQsbuBVKCudVG457BR2GZFIz3uw3hQ=
github.com/google/pprof v0.0.0-20221118152302-e6195bd50e26/go.mod h1:dDKJzRmX4S37WGHujM7tX//fmj1uioxKzKxz3lo4HJo=
github.com/google/pprof v0.0.0-20240409012703-83162a5b38cd/go.mod h1:kf6iHlnVGwgKolg33glAes7Yg/8iWP8ukqeldJSO7jw=
github.com/google/pprof v0.0.0-20250317173921-a4b03ec1a45e/go.mod h1:boTsfXsheKC2y+lKOCMpSfarhxDeIzfZG1jqGcPl3cA=
github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8=
github.com/mattn/go-sqlite3 v1.14.16 h1:yOQRA0RpS5PFz/oikGwBEqvAWhWg5ufRz4ETLjwpU1Y=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc=
golang.org/x/crypto v0.25.0/go.mod h1:T+wALwcMOSE0kXgUAnPAHqTLW+XHgcELELW8VaDgm/M=
golang.org/x/crypto v0.38.0/go.mod h1:MvrbAqul58NNYPKnOra203SB9vpuZW0e+RRZV+Ggqjw=
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
golang.org/x/mod v0.16.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
golang.org/x/mod v0.24.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww=
golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg=
golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c=
golang.org/x/net v0.6.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs=
Expand All @@ -24,7 +24,6 @@ golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJ
golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.5.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
golang.org/x/sync v0.14.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
Expand All @@ -37,6 +36,7 @@ golang.org/x/telemetry v0.0.0-20240521205824-bda55230c457/go.mod h1:pRgIJT+bRLFK
golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k=
golang.org/x/term v0.22.0/go.mod h1:F3qCibpT5AMpCRfhfT53vVJwhLtIVHhB9XDjfFvnMI4=
golang.org/x/term v0.32.0/go.mod h1:uZG1FhGx848Sqfsq4/DlJr3xGGsYMu/L5GW4abiaEPQ=
golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4=
Expand All @@ -49,7 +49,6 @@ golang.org/x/tools v0.17.0 h1:FvmRgNOcs3kOa+T20R1uhfP9F6HgG2mfxDv1vrx1Htc=
golang.org/x/tools v0.17.0/go.mod h1:xsh6VxdV005rRVaS6SSAf9oiAqljS7UZUacMZ8Bnsps=
golang.org/x/tools v0.19.0/go.mod h1:qoJWxmGSIBmAeriMx19ogtrEPrGtDbPK634QFIcLAhc=
golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk=
golang.org/x/tools v0.33.0/go.mod h1:CIJMaWEY88juyUfo7UbgPqbC8rU2OqfAV1h2Qp0oMYI=
lukechampine.com/uint128 v1.2.0/go.mod h1:c4eWIwlEGaxC/+H1VguhU4PHXNWDCDMUlWdIWl2j1gk=
modernc.org/cc/v3 v3.41.0/go.mod h1:Ni4zjJYJ04CDOhG7dn640WGfwBzfE0ecX8TyMB0Fv0Y=
modernc.org/ccgo/v3 v3.17.0/go.mod h1:Sg3fwVpmLvCUTaqEUjiBDAvshIaKDB0RXaf+zgqFu8I=
Expand Down
14 changes: 11 additions & 3 deletions sqlitex/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package sqlitex
import (
"context"
"fmt"
"strings"
"sync"

"zombiezen.com/go/sqlite"
Expand Down Expand Up @@ -69,11 +70,18 @@ func Open(uri string, flags sqlite.OpenFlags, poolSize int) (pool *Pool, err err

// NewPool opens a fixed-size pool of SQLite connections.
func NewPool(uri string, opts PoolOptions) (pool *Pool, err error) {
if uri == ":memory:" {
return nil, strerror{msg: `sqlite: ":memory:" does not work with multiple connections, use "file::memory:?mode=memory&cache=shared"`}
poolSize := opts.PoolSize

if poolSize != 1 {
uriLower := strings.ToLower(uri)
inMemory := uri == ":memory:" || opts.Flags&sqlite.OpenMemory != 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode=memory is another way this condition can be triggered.

sharedCache := strings.Contains(uriLower, "cache=shared") || opts.Flags&sqlite.OpenSharedCache != 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although unlikely, the cache=shared string could appear in the file name. I think we need to do some minimal parsing of the URI (and ensure that the URI is, in fact, being parsed as a URI). Off-hand, you might be able to get away with splitting on the ? and doing url.ParseQuery, but double-check the docs to make sure there isn't some subtlety that I haven't thought of.


if inMemory && !sharedCache {
return nil, strerror{msg: `sqlite: uri==":memory:" or flag sqlite.OpenMemory does not work with multiple connections, use "file::memory:?mode=memory&cache=shared"`}
}
}

poolSize := opts.PoolSize
if poolSize < 1 {
poolSize = 10
}
Expand Down
66 changes: 66 additions & 0 deletions sqlitex/pool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,72 @@ func TestPool(t *testing.T) {
stmt.Reset()
}

func TestInvalidPoolOptions(t *testing.T) {
dir, err := os.MkdirTemp("", "sqlite-test-")
if err != nil {
t.Fatal(err)
}
defer os.RemoveAll(dir)

dbFile := filepath.Join(dir, "awal.db")
dbFileShared := filepath.Join(dir, "awal.db?cache=shared")

tests := []struct {
name string
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names aren't being used and aren't helpful here. Remove, please.

uri string
flags sqlite.OpenFlags
wantErr bool
}{
{
name: "Error: uri is set to :memory: with poolsize > 1",
uri: ":memory:",
flags: sqlite.OpenReadWrite | sqlite.OpenCreate | sqlite.OpenURI,
wantErr: true,
},
{
name: "Error: OpenMemory flag set with poolsize > 1",
uri: dbFile,
flags: sqlite.OpenMemory | sqlite.OpenReadWrite | sqlite.OpenCreate | sqlite.OpenURI,
wantErr: true,
},
{
name: "Success: uri is :memory: but cache is shared via flag with poolsize > 1",
uri: ":memory:",
flags: sqlite.OpenSharedCache | sqlite.OpenReadWrite | sqlite.OpenCreate | sqlite.OpenURI,
},
{
name: "Succes: uri has cached=shared and sqlite.OpenMemory flag is set with poolsize > 1",
uri: dbFileShared,
flags: sqlite.OpenMemory | sqlite.OpenReadWrite | sqlite.OpenCreate | sqlite.OpenURI,
},
{
name: "Success: uri is dbFile, flag is set to sqlite.OpenMemory && sqlite.OpenSharedCache with poolsize > 1",
uri: dbFile,
flags: sqlite.OpenMemory | sqlite.OpenReadWrite | sqlite.OpenCreate | sqlite.OpenSharedCache | sqlite.OpenURI,
},
}

for _, test := range tests {
dbpool, err := sqlitex.NewPool(test.uri, sqlitex.PoolOptions{
Flags: test.flags,
PoolSize: 2,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should vary the pool size for some of these tests.

})
if err == nil {
dbpool.Close()
}

switch {
case err == nil && test.wantErr:
t.Errorf("TestInvalidPoolOptions(%s): got err == nil, want err != nil", test.name)
continue
case err != nil && !test.wantErr:
t.Errorf("TestInvalidPoolOptions(%s): got err == %s, want err == nil", test.name, err)
continue
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For error tests like these, I like including a case that logs the error message in successful cases without failing. This allows manual inspection to see whether there's a case that's failing for an unrelated error cause.

}

}

const insertCount = 120

func testInsert(t *testing.T, id string, dbpool *sqlitex.Pool) {
Expand Down