From cb1a4da5c271d9639a8305b8f7d03dd54e835f4e Mon Sep 17 00:00:00 2001 From: Giteabot Date: Mon, 7 Aug 2023 18:55:25 +0800 Subject: [PATCH] Bypass MariaDB performance bug of the "IN" sub-query, fix incorrect IssueIndex (#26279) (#26368) Backport #26279 by @wxiaoguang Close #26277 Fix #26285 Co-authored-by: wxiaoguang --- models/activities/action.go | 32 ++++++++++++++++++++++-------- models/activities/action_test.go | 34 ++++++++++++++++++++++++++++++++ services/issue/issue.go | 2 +- 3 files changed, 59 insertions(+), 9 deletions(-) diff --git a/models/activities/action.go b/models/activities/action.go index 57f579372f80..59115f82fd63 100644 --- a/models/activities/action.go +++ b/models/activities/action.go @@ -685,18 +685,34 @@ func NotifyWatchersActions(acts []*Action) error { } // DeleteIssueActions delete all actions related with issueID -func DeleteIssueActions(ctx context.Context, repoID, issueID int64) error { +func DeleteIssueActions(ctx context.Context, repoID, issueID, issueIndex int64) error { // delete actions assigned to this issue - subQuery := builder.Select("`id`"). - From("`comment`"). - Where(builder.Eq{"`issue_id`": issueID}) - if _, err := db.GetEngine(ctx).In("comment_id", subQuery).Delete(&Action{}); err != nil { - return err + e := db.GetEngine(ctx) + + // MariaDB has a performance bug: https://jira.mariadb.org/browse/MDEV-16289 + // so here it uses "DELETE ... WHERE IN" with pre-queried IDs. + var lastCommentID int64 + commentIDs := make([]int64, 0, db.DefaultMaxInSize) + for { + commentIDs = commentIDs[:0] + err := e.Select("`id`").Table(&issues_model.Comment{}). + Where(builder.Eq{"issue_id": issueID}).And("`id` > ?", lastCommentID). + OrderBy("`id`").Limit(db.DefaultMaxInSize). + Find(&commentIDs) + if err != nil { + return err + } else if len(commentIDs) == 0 { + break + } else if _, err = db.GetEngine(ctx).In("comment_id", commentIDs).Delete(&Action{}); err != nil { + return err + } else { + lastCommentID = commentIDs[len(commentIDs)-1] + } } - _, err := db.GetEngine(ctx).Table("action").Where("repo_id = ?", repoID). + _, err := e.Where("repo_id = ?", repoID). In("op_type", ActionCreateIssue, ActionCreatePullRequest). - Where("content LIKE ?", strconv.FormatInt(issueID, 10)+"|%"). + Where("content LIKE ?", strconv.FormatInt(issueIndex, 10)+"|%"). // "IssueIndex|content..." Delete(&Action{}) return err } diff --git a/models/activities/action_test.go b/models/activities/action_test.go index 7044bcc004ab..9a4274088004 100644 --- a/models/activities/action_test.go +++ b/models/activities/action_test.go @@ -4,6 +4,7 @@ package activities_test import ( + "fmt" "path" "testing" @@ -284,3 +285,36 @@ func TestConsistencyUpdateAction(t *testing.T) { assert.NoError(t, db.GetEngine(db.DefaultContext).Where("id = ?", id).Find(&actions)) unittest.CheckConsistencyFor(t, &activities_model.Action{}) } + +func TestDeleteIssueActions(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // load an issue + issue := unittest.AssertExistsAndLoadBean(t, &issue_model.Issue{ID: 4}) + assert.NotEqualValues(t, issue.ID, issue.Index) // it needs to use different ID/Index to test the DeleteIssueActions to delete some actions by IssueIndex + + // insert a comment + err := db.Insert(db.DefaultContext, &issue_model.Comment{Type: issue_model.CommentTypeComment, IssueID: issue.ID}) + assert.NoError(t, err) + comment := unittest.AssertExistsAndLoadBean(t, &issue_model.Comment{Type: issue_model.CommentTypeComment, IssueID: issue.ID}) + + // truncate action table and insert some actions + err = db.TruncateBeans(db.DefaultContext, &activities_model.Action{}) + assert.NoError(t, err) + err = db.Insert(db.DefaultContext, &activities_model.Action{ + OpType: activities_model.ActionCommentIssue, + CommentID: comment.ID, + }) + assert.NoError(t, err) + err = db.Insert(db.DefaultContext, &activities_model.Action{ + OpType: activities_model.ActionCreateIssue, + RepoID: issue.RepoID, + Content: fmt.Sprintf("%d|content...", issue.Index), + }) + assert.NoError(t, err) + + // assert that the actions exist, then delete them + unittest.AssertCount(t, &activities_model.Action{}, 2) + assert.NoError(t, activities_model.DeleteIssueActions(db.DefaultContext, issue.RepoID, issue.ID, issue.Index)) + unittest.AssertCount(t, &activities_model.Action{}, 0) +} diff --git a/services/issue/issue.go b/services/issue/issue.go index ce2b1c0976ef..03d34ba12d88 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -242,7 +242,7 @@ func deleteIssue(ctx context.Context, issue *issues_model.Issue) error { issue.MilestoneID, err) } - if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID); err != nil { + if err := activities_model.DeleteIssueActions(ctx, issue.RepoID, issue.ID, issue.Index); err != nil { return err }