From 010a396b0e15c3a8c8891486492ed223f6253db3 Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Wed, 22 Feb 2023 22:20:19 +0200 Subject: [PATCH] updated dao fail/retry handling --- CHANGELOG.md | 11 +++- daos/base.go | 33 +++++++---- daos/base_retry.go | 26 ++++----- daos/base_retry_test.go | 8 +-- daos/record.go | 100 +++++++++++++++++----------------- tools/search/provider.go | 9 +-- tools/search/provider_test.go | 12 ++-- 7 files changed, 108 insertions(+), 91 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d66d8c2..8b89cddb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,11 @@ - Added new "View" collection type (@todo document) -- Added auto fail/retry for the `SELECT` queries to gracefully handle the `database is locked` errors ([#1795](https://github.com/pocketbase/pocketbase/discussions/1795#discussioncomment-4882169)). +- Added auto fail/retry (default to 8 attempts) for the `SELECT` queries to gracefully handle the `database is locked` errors ([#1795](https://github.com/pocketbase/pocketbase/discussions/1795#discussioncomment-4882169)). + _The default max attempts can be accessed or changed via `Dao.MaxLockRetries`._ -- Added default max query executation timeout (120s). +- Added default max query executation timeout (90s). + _The default timeout can be access or changed via `Dao.ModelQueryTimeout`._ - Added support for `dao.RecordQuery(collection)` to scan directly the `One()` and `All()` results in `*models.Record` or `[]*models.Record` without the need of explicit `NullStringMap`. @@ -14,6 +16,10 @@ - Enabled `process.env` in JS migrations to allow accessing `os.Environ()`. +- Added `UploadedFiles` field to the `RecordCreateEvent` and `RecordUpdateEvent` event structs. + +- **!** Moved file upload after the record persistent to allow custom changing the record id safely from the `OnModelBeforeCreate` hook. + - **!** Changed `System.GetFile()` to return directly `*blob.Reader` instead of the `io.ReadCloser` interface. - **!** Changed `To`, `Cc` and `Bcc` of `mailer.Message` to `[]mail.Address` for consistency and to allow multiple recipients and optional name. @@ -38,6 +44,7 @@ - **!** Removed the previously deprecated `Dao.Block()` and `Dao.Continue()` helpers in favor of `Dao.NonconcurrentDB()`. +- Other minor Admin UI improvements. ## v0.12.3 diff --git a/daos/base.go b/daos/base.go index 08b4e5a9..0f9d1bb4 100644 --- a/daos/base.go +++ b/daos/base.go @@ -5,6 +5,7 @@ package daos import ( "errors" + "time" "github.com/pocketbase/dbx" "github.com/pocketbase/pocketbase/models" @@ -20,8 +21,10 @@ func New(db dbx.Builder) *Dao { // async and sync db builders. func NewMultiDB(concurrentDB, nonconcurrentDB dbx.Builder) *Dao { return &Dao{ - concurrentDB: concurrentDB, - nonconcurrentDB: nonconcurrentDB, + concurrentDB: concurrentDB, + nonconcurrentDB: nonconcurrentDB, + MaxLockRetries: 8, + ModelQueryTimeout: 90 * time.Second, } } @@ -32,6 +35,14 @@ type Dao struct { concurrentDB dbx.Builder nonconcurrentDB dbx.Builder + // MaxLockRetries specifies the default max "database is locked" auto retry attempts. + MaxLockRetries int + + // ModelQueryTimeout is the default max duration of a running ModelQuery(). + // + // This field has no effect if an explicit query context is already specified. + ModelQueryTimeout time.Duration + BeforeCreateFunc func(eventDao *Dao, m models.Model) error AfterCreateFunc func(eventDao *Dao, m models.Model) BeforeUpdateFunc func(eventDao *Dao, m models.Model) error @@ -63,15 +74,17 @@ func (dao *Dao) NonconcurrentDB() dbx.Builder { return dao.nonconcurrentDB } -// ModelQuery creates a new query with preset Select and From fields -// based on the provided model argument. +// ModelQuery creates a new preconfigured select query with preset +// SELECT, FROM and other common fields based on the provided model. func (dao *Dao) ModelQuery(m models.Model) *dbx.SelectQuery { tableName := m.TableName() return dao.DB(). Select("{{" + tableName + "}}.*"). From(tableName). - WithExecHook(onLockErrorRetry) + WithBuildHook(func(query *dbx.Query) { + query.WithExecHook(execLockRetry(dao.ModelQueryTimeout, dao.MaxLockRetries)) + }) } // FindById finds a single db record with the specified id and @@ -189,7 +202,7 @@ func (dao *Dao) Delete(m models.Model) error { } return nil - }, defaultMaxRetries) + }) } // Save upserts (update or create if primary key is not set) the provided model. @@ -197,12 +210,12 @@ func (dao *Dao) Save(m models.Model) error { if m.IsNew() { return dao.lockRetry(func(retryDao *Dao) error { return retryDao.create(m) - }, defaultMaxRetries) + }) } return dao.lockRetry(func(retryDao *Dao) error { return retryDao.update(m) - }, defaultMaxRetries) + }) } func (dao *Dao) update(m models.Model) error { @@ -296,7 +309,7 @@ func (dao *Dao) create(m models.Model) error { return nil } -func (dao *Dao) lockRetry(op func(retryDao *Dao) error, maxRetries int) error { +func (dao *Dao) lockRetry(op func(retryDao *Dao) error) error { retryDao := dao return baseLockRetry(func(attempt int) error { @@ -310,5 +323,5 @@ func (dao *Dao) lockRetry(op func(retryDao *Dao) error, maxRetries int) error { } return op(retryDao) - }, maxRetries) + }, dao.MaxLockRetries) } diff --git a/daos/base_retry.go b/daos/base_retry.go index 9c0f9e51..eca32fa2 100644 --- a/daos/base_retry.go +++ b/daos/base_retry.go @@ -8,26 +8,24 @@ import ( "github.com/pocketbase/dbx" ) -const defaultQueryTimeout time.Duration = 2 * time.Minute +// default retries intervals (in ms) +var defaultRetryIntervals = []int{100, 250, 350, 500, 700, 1000} -const defaultMaxRetries int = 10 - -var defaultRetryIntervals = []int{100, 250, 350, 500, 700, 1000, 1200, 1500} - -func onLockErrorRetry(s *dbx.SelectQuery, op func() error) error { - return baseLockRetry(func(attempt int) error { - // load a default timeout context if not set explicitly - if s.Context() == nil { - ctx, cancel := context.WithTimeout(context.Background(), defaultQueryTimeout) +func execLockRetry(timeout time.Duration, maxRetries int) dbx.ExecHookFunc { + return func(q *dbx.Query, op func() error) error { + if q.Context() == nil { + cancelCtx, cancel := context.WithTimeout(context.Background(), timeout) defer func() { cancel() - s.WithContext(nil) // reset + q.WithContext(nil) // reset }() - s.WithContext(ctx) + q.WithContext(cancelCtx) } - return op() - }, defaultMaxRetries) + return baseLockRetry(func(attempt int) error { + return op() + }, maxRetries) + } } func baseLockRetry(op func(attempt int) error, maxRetries int) error { diff --git a/daos/base_retry_test.go b/daos/base_retry_test.go index e4232f91..ddf84162 100644 --- a/daos/base_retry_test.go +++ b/daos/base_retry_test.go @@ -6,12 +6,12 @@ import ( ) func TestGetDefaultRetryInterval(t *testing.T) { - if i := getDefaultRetryInterval(-1); i.Milliseconds() != 1500 { - t.Fatalf("Expected 1500ms, got %v", i) + if i := getDefaultRetryInterval(-1); i.Milliseconds() != 1000 { + t.Fatalf("Expected 1000ms, got %v", i) } - if i := getDefaultRetryInterval(999); i.Milliseconds() != 1500 { - t.Fatalf("Expected 1500ms, got %v", i) + if i := getDefaultRetryInterval(999); i.Milliseconds() != 1000 { + t.Fatalf("Expected 1000ms, got %v", i) } if i := getDefaultRetryInterval(3); i.Milliseconds() != 500 { diff --git a/daos/record.go b/daos/record.go index a7aea19f..ccf52f21 100644 --- a/daos/record.go +++ b/daos/record.go @@ -23,68 +23,70 @@ func (dao *Dao) RecordQuery(collection *models.Collection) *dbx.SelectQuery { return dao.DB(). Select(selectCols). From(tableName). - WithExecHook(onLockErrorRetry). - WithOneHook(func(s *dbx.SelectQuery, a any, op func(b any) error) error { - switch v := a.(type) { - case *models.Record: - if v == nil { - return op(a) - } + WithBuildHook(func(query *dbx.Query) { + query.WithExecHook(execLockRetry(dao.ModelQueryTimeout, dao.MaxLockRetries)). + WithOneHook(func(q *dbx.Query, a any, op func(b any) error) error { + switch v := a.(type) { + case *models.Record: + if v == nil { + return op(a) + } - row := dbx.NullStringMap{} - if err := op(&row); err != nil { - return err - } + row := dbx.NullStringMap{} + if err := op(&row); err != nil { + return err + } - record := models.NewRecordFromNullStringMap(collection, row) + record := models.NewRecordFromNullStringMap(collection, row) - *v = *record + *v = *record - return nil - default: - return op(a) - } - }). - WithAllHook(func(s *dbx.SelectQuery, sliceA any, op func(sliceB any) error) error { - switch v := sliceA.(type) { - case *[]*models.Record: - if v == nil { - return op(sliceA) - } + return nil + default: + return op(a) + } + }). + WithAllHook(func(q *dbx.Query, sliceA any, op func(sliceB any) error) error { + switch v := sliceA.(type) { + case *[]*models.Record: + if v == nil { + return op(sliceA) + } - rows := []dbx.NullStringMap{} - if err := op(&rows); err != nil { - return err - } + rows := []dbx.NullStringMap{} + if err := op(&rows); err != nil { + return err + } - records := models.NewRecordsFromNullStringMaps(collection, rows) + records := models.NewRecordsFromNullStringMaps(collection, rows) - *v = records + *v = records - return nil - case *[]models.Record: - if v == nil { - return op(sliceA) - } + return nil + case *[]models.Record: + if v == nil { + return op(sliceA) + } - rows := []dbx.NullStringMap{} - if err := op(&rows); err != nil { - return err - } + rows := []dbx.NullStringMap{} + if err := op(&rows); err != nil { + return err + } - records := models.NewRecordsFromNullStringMaps(collection, rows) + records := models.NewRecordsFromNullStringMaps(collection, rows) - nonPointers := make([]models.Record, len(records)) - for i, r := range records { - nonPointers[i] = *r - } + nonPointers := make([]models.Record, len(records)) + for i, r := range records { + nonPointers[i] = *r + } - *v = nonPointers + *v = nonPointers - return nil - default: - return op(sliceA) - } + return nil + default: + return op(sliceA) + } + }) }) } diff --git a/tools/search/provider.go b/tools/search/provider.go index f8d728fb..8a559cb4 100644 --- a/tools/search/provider.go +++ b/tools/search/provider.go @@ -5,7 +5,6 @@ import ( "math" "net/url" "strconv" - "strings" "github.com/pocketbase/dbx" ) @@ -198,11 +197,9 @@ func (s *Provider) Exec(items any) (*Result, error) { if len(queryInfo.From) > 0 { baseTable = queryInfo.From[0] } - countQuery := modelsQuery - rawCountQuery := countQuery.Select(strings.Join([]string{baseTable, "id"}, ".")).OrderBy().Build().SQL() - wrappedCountQuery := queryInfo.Builder.NewQuery("SELECT COUNT(*) FROM (" + rawCountQuery + ")") - wrappedCountQuery.Bind(countQuery.Build().Params()) - if err := wrappedCountQuery.Row(&totalCount); err != nil { + clone := modelsQuery + countQuery := clone.Select("COUNT(DISTINCT {{" + baseTable + ".id}})").OrderBy() + if err := countQuery.Row(&totalCount); err != nil { return nil, err } diff --git a/tools/search/provider_test.go b/tools/search/provider_test.go index 5c8c5ba2..23b5c611 100644 --- a/tools/search/provider_test.go +++ b/tools/search/provider_test.go @@ -228,7 +228,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { false, `{"page":1,"perPage":10,"totalItems":2,"totalPages":1,"items":[{"test1":1,"test2":"test2.1","test3":""},{"test1":2,"test2":"test2.2","test3":""}]}`, []string{ - "SELECT COUNT(*) FROM (SELECT `test`.`id` FROM `test` WHERE NOT (`test1` IS NULL))", + "SELECT COUNT(DISTINCT {{test.id}}) FROM `test` WHERE NOT (`test1` IS NULL)", "SELECT * FROM `test` WHERE NOT (`test1` IS NULL) ORDER BY `test1` ASC LIMIT 10", }, }, @@ -241,7 +241,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { false, `{"page":1,"perPage":30,"totalItems":2,"totalPages":1,"items":[{"test1":1,"test2":"test2.1","test3":""},{"test1":2,"test2":"test2.2","test3":""}]}`, []string{ - "SELECT COUNT(*) FROM (SELECT `test`.`id` FROM `test` WHERE NOT (`test1` IS NULL))", + "SELECT COUNT(DISTINCT {{test.id}}) FROM `test` WHERE NOT (`test1` IS NULL)", "SELECT * FROM `test` WHERE NOT (`test1` IS NULL) ORDER BY `test1` ASC LIMIT 30", }, }, @@ -274,7 +274,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { false, `{"page":1,"perPage":` + fmt.Sprint(MaxPerPage) + `,"totalItems":1,"totalPages":1,"items":[{"test1":2,"test2":"test2.2","test3":""}]}`, []string{ - "SELECT COUNT(*) FROM (SELECT `test`.`id` FROM `test` WHERE ((NOT (`test1` IS NULL)) AND (COALESCE(test2, '') != COALESCE(null, ''))) AND (test1 >= 2))", + "SELECT COUNT(DISTINCT {{test.id}}) FROM `test` WHERE ((NOT (`test1` IS NULL)) AND (COALESCE(test2, '') != COALESCE(null, ''))) AND (test1 >= 2)", "SELECT * FROM `test` WHERE ((NOT (`test1` IS NULL)) AND (COALESCE(test2, '') != COALESCE(null, ''))) AND (test1 >= 2) ORDER BY `test1` ASC, `test2` DESC LIMIT 500", }, }, @@ -287,7 +287,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { false, `{"page":1,"perPage":10,"totalItems":0,"totalPages":0,"items":[]}`, []string{ - "SELECT COUNT(*) FROM (SELECT `test`.`id` FROM `test` WHERE (NOT (`test1` IS NULL)) AND (COALESCE(test3, '') != COALESCE('', '')))", + "SELECT COUNT(DISTINCT {{test.id}}) FROM `test` WHERE (NOT (`test1` IS NULL)) AND (COALESCE(test3, '') != COALESCE('', ''))", "SELECT * FROM `test` WHERE (NOT (`test1` IS NULL)) AND (COALESCE(test3, '') != COALESCE('', '')) ORDER BY `test1` ASC, `test3` ASC LIMIT 10", }, }, @@ -300,7 +300,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { false, `{"page":2,"perPage":1,"totalItems":2,"totalPages":2,"items":[{"test1":2,"test2":"test2.2","test3":""}]}`, []string{ - "SELECT COUNT(*) FROM (SELECT `test`.`id` FROM `test` WHERE NOT (`test1` IS NULL))", + "SELECT COUNT(DISTINCT {{test.id}}) FROM `test` WHERE NOT (`test1` IS NULL)", "SELECT * FROM `test` WHERE NOT (`test1` IS NULL) ORDER BY `test1` ASC LIMIT 1 OFFSET 1", }, }, @@ -345,7 +345,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { for _, q := range testDB.CalledQueries { if !list.ExistInSliceWithRegex(q, s.expectQueries) { - t.Errorf("[%s] Didn't expect query \n%v in \n%v", s.name, q, testDB.CalledQueries) + t.Fatalf("[%s] Didn't expect query \n%v \nin \n%v", s.name, q, s.expectQueries) } } }