Use a LockedResource to numerate issues and prs

integrations/locked_resource_test.go

@@ -20,14 +20,14 @@ const (
 	// blockerDelay minus tolerance to complete.
 	// Note: these values might require tuning in order to avoid
 	// false negatives.
-	waiterDelay = 100 * time.Millisecond
+	waiterDelay  = 100 * time.Millisecond
 	blockerDelay = 200 * time.Millisecond
-	tolerance = 50 * time.Millisecond	// Should be <= (blockerDelay-waiterDelay)/2
+	tolerance    = 50 * time.Millisecond // Should be <= (blockerDelay-waiterDelay)/2
 )
 
 type waitResult struct {
-	Waited	time.Duration
-	Err		error
+	Waited time.Duration
+	Err    error
 }
 
 func TestLockedResource(t *testing.T) {
@@ -75,16 +75,16 @@ func blockTest(name string, f func(ctx models.DBContext) error) error {
 		cw <- blockTestFunc(name, false, ref, f)
 	}()
 
-	resb := <- cb
-	resw := <- cw
+	resb := <-cb
+	resw := <-cw
 	if resb.Err != nil {
 		return resb.Err
 	}
 	if resw.Err != nil {
 		return resw.Err
 	}
 
-	if resw.Waited < blockerDelay - tolerance {
+	if resw.Waited < blockerDelay-tolerance {
 		return fmt.Errorf("Waiter not blocked on %s; wait: %d ms, expected > %d ms",
 			name, resw.Waited.Milliseconds(), (blockerDelay - tolerance).Milliseconds())
 	}

models/issue.go

@@ -75,7 +75,10 @@ var (
 
 const issueTasksRegexpStr = `(^\s*[-*]\s\[[\sx]\]\s.)|(\n\s*[-*]\s\[[\sx]\]\s.)`
 const issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[x]\]\s.)|(\n\s*[-*]\s\[[x]\]\s.)`
-const issueMaxDupIndexAttempts = 3
+
+// IssueLockedEnumerator is the name of the locked_resource used to
+// numerate issues in a repository.
+const IssueLockedEnumerator = "repository-index"
 
 func init() {
 	issueTasksPat = regexp.MustCompile(issueTasksRegexpStr)
@@ -898,19 +901,23 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 	}
 
 	// Milestone validation should happen before insert actual object.
-	if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").
-		Where("repo_id=?", opts.Issue.RepoID).
-		Insert(opts.Issue); err != nil {
-		return ErrNewIssueInsert{err}
-	}
 
-	inserted, err := getIssueByID(e, opts.Issue.ID)
+	// Obtain the next issue number for this repository, which will be locked
+	// and reserved for the remaining of the transaction. Should the transaction
+	// be rolled back, the previous value will be restored.
+	locked, err := GetLockedResource(e, IssueLockedEnumerator, opts.Issue.RepoID)
 	if err != nil {
-		return err
+		return fmt.Errorf("GetLockedResource(%s)", IssueLockedEnumerator)
+	}
+	locked.Counter++
+	if err := UpdateLockedResource(e, locked); err != nil {
+		return fmt.Errorf("UpdateLockedResource(%s)", IssueLockedEnumerator)
 	}
+	opts.Issue.Index = locked.Counter
 
-	// Patch Index with the value calculated by the database
-	opts.Issue.Index = inserted.Index
+	if _, err = e.Insert(opts.Issue); err != nil {
+		return err
+	}
 
 	if opts.Issue.MilestoneID > 0 {
 		if _, err = e.Exec("UPDATE `milestone` SET num_issues=num_issues+1 WHERE id=?", opts.Issue.MilestoneID); err != nil {
@@ -988,24 +995,6 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 
 // NewIssue creates new issue with labels for repository.
 func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) {
-	// Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887
-	i := 0
-	for {
-		if err = newIssueAttempt(repo, issue, labelIDs, uuids); err == nil {
-			return nil
-		}
-		if !IsErrNewIssueInsert(err) {
-			return err
-		}
-		if i++; i == issueMaxDupIndexAttempts {
-			break
-		}
-		log.Error("NewIssue: error attempting to insert the new issue; will retry. Original error: %v", err)
-	}
-	return fmt.Errorf("NewIssue: too many errors attempting to insert the new issue. Last error was: %v", err)
-}
-
-func newIssueAttempt(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err = sess.Begin(); err != nil {

models/locked_resource.go

@@ -11,9 +11,9 @@ import (
 )
 
 type LockedResource struct {
-	LockType	string      `xorm:"pk VARCHAR(30)"`
-	LockKey		int64		`xorm:"pk"`
-	Counter		int64		`xorm:"NOT NULL DEFAULT 0"`
+	LockType string `xorm:"pk VARCHAR(30)"`
+	LockKey  int64  `xorm:"pk"`
+	Counter  int64  `xorm:"NOT NULL DEFAULT 0"`
 }
 
 func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResource, error) {
@@ -29,7 +29,7 @@ func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResourc
 	} else if !has {
 		return nil, fmt.Errorf("unexpected upsert fail  %s:%d", lockType, lockKey)
 	}
-	
+
 	return locked, nil
 }
 
@@ -72,16 +72,16 @@ func TempLockResourceCtx(ctx DBContext, lockType string, lockKey int64) error {
 
 func upsertLockedResource(e Engine, resource *LockedResource) (err error) {
 	// An atomic UPSERT operation (INSERT/UPDATE) is the only operation
-	// that ensures that the key is actually locked. 
+	// that ensures that the key is actually locked.
 	switch {
 	case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL:
 		_, err = e.Exec("INSERT INTO locked_resource (lock_type, lock_key) "+
 			"VALUES (?,?) ON CONFLICT(lock_type, lock_key) DO UPDATE SET lock_key = ?",
-			resource.LockType, resource.LockKey, resource.LockKey);
+			resource.LockType, resource.LockKey, resource.LockKey)
 	case setting.Database.UseMySQL:
 		_, err = e.Exec("INSERT INTO locked_resource (lock_type, lock_key) "+
 			"VALUES (?,?) ON DUPLICATE KEY UPDATE lock_key = lock_key",
-			resource.LockType, resource.LockKey);
+			resource.LockType, resource.LockKey)
 	case setting.Database.UseMSSQL:
 		// https://weblogs.sqlteam.com/dang/2009/01/31/upsert-race-condition-with-merge/
 		_, err = e.Exec("MERGE locked_resource WITH (HOLDLOCK) as target "+
@@ -90,9 +90,9 @@ func upsertLockedResource(e Engine, resource *LockedResource) (err error) {
 			"WHEN MATCHED THEN UPDATE SET target.lock_key = target.lock_key "+
 			"WHEN NOT MATCHED THEN INSERT (lock_type, lock_key) "+
 			"VALUES (src.lock_type, src.lock_key);",
-			resource.LockType, resource.LockKey);
+			resource.LockType, resource.LockKey)
 	default:
 		return fmt.Errorf("database type not supported")
 	}
 	return
-}
+}

models/locked_resource_test.go

@@ -30,8 +30,8 @@ func TestLockedResource(t *testing.T) {
 
 	// Get lock, increment counter value
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1",1)
-		if	!assert.NoError(t, err) || !assert.NotEmpty(t, lck1) || !assert.Equal(t, int64(0), lck1.Counter) {
+		lck1, err := GetLockedResource(sess, "test-1", 1)
+		if !assert.NoError(t, err) || !assert.NotEmpty(t, lck1) || !assert.Equal(t, int64(0), lck1.Counter) {
 			return false
 		}
 		lck1.Counter++
@@ -41,20 +41,20 @@ func TestLockedResource(t *testing.T) {
 
 	// Get lock, check counter value
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1",1)
+		lck1, err := GetLockedResource(sess, "test-1", 1)
 		return assert.NoError(t, err) && assert.NotEmpty(t, lck1) && assert.Equal(t, int64(1), lck1.Counter)
 	})
 
 	// Attempt temp lock on an existing key, expect error
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		err := TempLockResource(sess, "test-1",1)
+		err := TempLockResource(sess, "test-1", 1)
 		// Must give error
 		return assert.Error(t, err)
 	})
 
 	// Delete lock
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1",1)
+		lck1, err := GetLockedResource(sess, "test-1", 1)
 		if !assert.NoError(t, err) || !assert.NotEmpty(t, lck1) {
 			return false
 		}
@@ -63,10 +63,9 @@ func TestLockedResource(t *testing.T) {
 
 	// Attempt temp lock on an valid key, expect success
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		return assert.NoError(t, TempLockResource(sess, "test-1",1))
+		return assert.NoError(t, TempLockResource(sess, "test-1", 1))
 	})
 
 	// Note: testing the validity of the locking mechanism (i.e. whether it actually locks)
 	// is be done at the integration tests to ensure that all the supported databases are checked.
 }
-

models/migrations/v125.go

@@ -5,16 +5,43 @@
 package migrations
 
 import (
+	"code.gitea.io/gitea/models"
+
 	"xorm.io/xorm"
 )
 
 func addLockedResourceTable(x *xorm.Engine) error {
 
 	type LockedResource struct {
-		LockType	string      `xorm:"pk VARCHAR(30)"`
-		LockKey		int64		`xorm:"pk"`
-		Counter		int64		`xorm:"NOT NULL DEFAULT 0"`
+		LockType string `xorm:"pk VARCHAR(30)"`
+		LockKey  int64  `xorm:"pk"`
+		Counter  int64  `xorm:"NOT NULL DEFAULT 0"`
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+
+	if err := sess.Sync2(new(LockedResource)); err != nil {
+		return err
 	}
-
-	return x.Sync2(new(LockedResource))
+
+	// Remove data we're goint to rebuild
+	if _, err := sess.Delete(&LockedResource{LockType: models.IssueLockedEnumerator}); err != nil {
+		return err
+	}
+
+	// Create current data for all repositories with issues and PRs
+	if _, err := sess.Exec("INSERT INTO locked_resource (lock_type, lock_key, counter) "+
+		"SELECT ?, max_data.repo_id, max_data.max_index "+
+		"FROM ( SELECT issue.repo_id AS repo_id, max(issue.index) AS max_index "+
+		"FROM issue GROUP BY issue.repo_id) AS max_data",
+		models.IssueLockedEnumerator); err != nil {
+		return err
+	}
+
+	return sess.Commit()
 }

models/pull.go

@@ -531,24 +531,6 @@ func (pr *PullRequest) SetMerged() (err error) {
 
 // NewPullRequest creates new pull request with labels for repository.
 func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
-	// Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887
-	i := 0
-	for {
-		if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr); err == nil {
-			return nil
-		}
-		if !IsErrNewIssueInsert(err) {
-			return err
-		}
-		if i++; i == issueMaxDupIndexAttempts {
-			break
-		}
-		log.Error("NewPullRequest: error attempting to insert the new issue; will retry. Original error: %v", err)
-	}
-	return fmt.Errorf("NewPullRequest: too many errors attempting to insert the new issue. Last error was: %v", err)
-}
-
-func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err = sess.Begin(); err != nil {