From bf48c5acb657ca98750d8268b352dc9126397554 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 15 Oct 2022 22:15:43 +0800 Subject: [PATCH 1/4] fix res index --- models/db/index.go | 128 ++++++++++++++++--------------- models/issues/issue.go | 14 ++-- models/issues/issue_index.go | 8 +- models/issues/issue_xref_test.go | 9 ++- models/issues/pull.go | 14 ++-- models/repo.go | 2 +- 6 files changed, 89 insertions(+), 86 deletions(-) diff --git a/models/db/index.go b/models/db/index.go index 673c382b27fcf..df9a5431699f7 100644 --- a/models/db/index.go +++ b/models/db/index.go @@ -8,45 +8,15 @@ import ( "context" "errors" "fmt" - - "code.gitea.io/gitea/modules/setting" ) // ResourceIndex represents a resource index which could be used as issue/release and others -// We can create different tables i.e. issue_index, release_index and etc. +// We can create different tables i.e. issue_index, release_index, etc. type ResourceIndex struct { GroupID int64 `xorm:"pk"` MaxIndex int64 `xorm:"index"` } -// UpsertResourceIndex the function will not return until it acquires the lock or receives an error. -func UpsertResourceIndex(ctx context.Context, tableName string, groupID int64) (err error) { - // An atomic UPSERT operation (INSERT/UPDATE) is the only operation - // that ensures that the key is actually locked. - switch { - case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL: - _, err = Exec(ctx, fmt.Sprintf("INSERT INTO %s (group_id, max_index) "+ - "VALUES (?,1) ON CONFLICT (group_id) DO UPDATE SET max_index = %s.max_index+1", - tableName, tableName), groupID) - case setting.Database.UseMySQL: - _, err = Exec(ctx, fmt.Sprintf("INSERT INTO %s (group_id, max_index) "+ - "VALUES (?,1) ON DUPLICATE KEY UPDATE max_index = max_index+1", tableName), - groupID) - case setting.Database.UseMSSQL: - // https://weblogs.sqlteam.com/dang/2009/01/31/upsert-race-condition-with-merge/ - _, err = Exec(ctx, fmt.Sprintf("MERGE %s WITH (HOLDLOCK) as target "+ - "USING (SELECT ? AS group_id) AS src "+ - "ON src.group_id = target.group_id "+ - "WHEN MATCHED THEN UPDATE SET target.max_index = target.max_index+1 "+ - "WHEN NOT MATCHED THEN INSERT (group_id, max_index) "+ - "VALUES (src.group_id, 1);", tableName), - groupID) - default: - return fmt.Errorf("database type not supported") - } - return err -} - var ( // ErrResouceOutdated represents an error when request resource outdated ErrResouceOutdated = errors.New("resource outdated") @@ -59,53 +29,89 @@ const ( MaxDupIndexAttempts = 3 ) -// GetNextResourceIndex retried 3 times to generate a resource index -func GetNextResourceIndex(tableName string, groupID int64) (int64, error) { - for i := 0; i < MaxDupIndexAttempts; i++ { - idx, err := getNextResourceIndex(tableName, groupID) - if err == ErrResouceOutdated { - continue +// SyncMaxResourceIndex sync the max index with the resource +func SyncMaxResourceIndex(ctx context.Context, tableName string, groupID, maxIndex int64) (err error) { + e := GetEngine(ctx) + + // try to update the max_index and acquire the write-lock for the record + res, err := e.Exec(fmt.Sprintf("UPDATE %s SET max_index=? WHERE group_id=? AND max_index Date: Sun, 16 Oct 2022 09:05:26 +0800 Subject: [PATCH 2/4] add tests --- models/db/index_test.go | 127 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 models/db/index_test.go diff --git a/models/db/index_test.go b/models/db/index_test.go new file mode 100644 index 0000000000000..1ea30e2b60c34 --- /dev/null +++ b/models/db/index_test.go @@ -0,0 +1,127 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package db_test + +import ( + "context" + "errors" + "fmt" + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/unittest" + + "github.com/stretchr/testify/assert" +) + +type TestIndex db.ResourceIndex + +func getCurrentResourceIndex(ctx context.Context, tableName string, groupID int64) (int64, error) { + e := db.GetEngine(ctx) + var idx int64 + has, err := e.SQL(fmt.Sprintf("SELECT max_index FROM %s WHERE group_id=?", tableName), groupID).Get(&idx) + if err != nil { + return 0, err + } + if !has { + return 0, errors.New("no record") + } + return idx, nil +} + +func TestSyncMaxResourceIndex(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + xe := unittest.GetXORMEngine() + assert.NoError(t, xe.Sync(&TestIndex{})) + + err := db.SyncMaxResourceIndex(db.DefaultContext, "test_index", 10, 51) + assert.NoError(t, err) + + // sync new max index + maxIndex, err := getCurrentResourceIndex(db.DefaultContext, "test_index", 10) + assert.NoError(t, err) + assert.EqualValues(t, 51, maxIndex) + + // smaller index doesn't change + err = db.SyncMaxResourceIndex(db.DefaultContext, "test_index", 10, 30) + assert.NoError(t, err) + maxIndex, err = getCurrentResourceIndex(db.DefaultContext, "test_index", 10) + assert.NoError(t, err) + assert.EqualValues(t, 51, maxIndex) + + // larger index changes + err = db.SyncMaxResourceIndex(db.DefaultContext, "test_index", 10, 62) + assert.NoError(t, err) + maxIndex, err = getCurrentResourceIndex(db.DefaultContext, "test_index", 10) + assert.NoError(t, err) + assert.EqualValues(t, 62, maxIndex) + + // commit transaction + err = db.WithTx(func(ctx context.Context) error { + err = db.SyncMaxResourceIndex(ctx, "test_index", 10, 73) + assert.NoError(t, err) + maxIndex, err = getCurrentResourceIndex(ctx, "test_index", 10) + assert.NoError(t, err) + assert.EqualValues(t, 73, maxIndex) + return nil + }) + assert.NoError(t, err) + maxIndex, err = getCurrentResourceIndex(db.DefaultContext, "test_index", 10) + assert.NoError(t, err) + assert.EqualValues(t, 73, maxIndex) + + // rollback transaction + err = db.WithTx(func(ctx context.Context) error { + err = db.SyncMaxResourceIndex(ctx, "test_index", 10, 84) + maxIndex, err = getCurrentResourceIndex(ctx, "test_index", 10) + assert.NoError(t, err) + assert.EqualValues(t, 84, maxIndex) + return errors.New("test rollback") + }) + assert.Error(t, err) + maxIndex, err = getCurrentResourceIndex(db.DefaultContext, "test_index", 10) + assert.NoError(t, err) + assert.EqualValues(t, 73, maxIndex) // the max index doesn't change because the transaction was rolled back +} + +func TestGetNextResourceIndex(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + xe := unittest.GetXORMEngine() + assert.NoError(t, xe.Sync(&TestIndex{})) + + // create a new record + maxIndex, err := db.GetNextResourceIndex(db.DefaultContext, "test_index", 20) + assert.NoError(t, err) + assert.EqualValues(t, 1, maxIndex) + + // increase the existing record + maxIndex, err = db.GetNextResourceIndex(db.DefaultContext, "test_index", 20) + assert.NoError(t, err) + assert.EqualValues(t, 2, maxIndex) + + // commit transaction + err = db.WithTx(func(ctx context.Context) error { + maxIndex, err = db.GetNextResourceIndex(ctx, "test_index", 20) + assert.NoError(t, err) + assert.EqualValues(t, 3, maxIndex) + return nil + }) + assert.NoError(t, err) + maxIndex, err = getCurrentResourceIndex(db.DefaultContext, "test_index", 20) + assert.NoError(t, err) + assert.EqualValues(t, 3, maxIndex) + + // rollback transaction + err = db.WithTx(func(ctx context.Context) error { + maxIndex, err = db.GetNextResourceIndex(ctx, "test_index", 20) + assert.NoError(t, err) + assert.EqualValues(t, 4, maxIndex) + return errors.New("test rollback") + }) + assert.Error(t, err) + maxIndex, err = getCurrentResourceIndex(db.DefaultContext, "test_index", 20) + assert.NoError(t, err) + assert.EqualValues(t, 3, maxIndex) // the max index doesn't change because the transaction was rolled back +} From 5fc0902a796d013e2449ca50d56ee1fd357a3601 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 16 Oct 2022 12:41:15 +0800 Subject: [PATCH 3/4] Update models/db/index.go --- models/db/index.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/db/index.go b/models/db/index.go index df9a5431699f7..7eaac7038b439 100644 --- a/models/db/index.go +++ b/models/db/index.go @@ -43,7 +43,7 @@ func SyncMaxResourceIndex(ctx context.Context, tableName string, groupID, maxInd return err } if affected == 0 { - // if nothing is updated, the record might not exist, try to insert and update it + // if nothing is updated, the record might not exist or is larger, it's safe to try to insert and update it again _, errIns := e.Exec(fmt.Sprintf("INSERT INTO %s (group_id, max_index) VALUES (?, 0)", tableName), groupID) _, err = e.Exec(fmt.Sprintf("UPDATE %s SET max_index=? WHERE group_id=? AND max_index Date: Sun, 16 Oct 2022 12:53:48 +0800 Subject: [PATCH 4/4] remove a unnecessary update in SyncMaxResourceIndex --- models/db/index.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/models/db/index.go b/models/db/index.go index 7eaac7038b439..58a976ad52876 100644 --- a/models/db/index.go +++ b/models/db/index.go @@ -43,12 +43,8 @@ func SyncMaxResourceIndex(ctx context.Context, tableName string, groupID, maxInd return err } if affected == 0 { - // if nothing is updated, the record might not exist or is larger, it's safe to try to insert and update it again - _, errIns := e.Exec(fmt.Sprintf("INSERT INTO %s (group_id, max_index) VALUES (?, 0)", tableName), groupID) - _, err = e.Exec(fmt.Sprintf("UPDATE %s SET max_index=? WHERE group_id=? AND max_index