From e4b1ea6f15790b3ce918a29404f93353d7143a1c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 7 Aug 2023 18:23:59 +0800 Subject: [PATCH] Bypass MariaDB performance bug of the "IN" sub-query, fix incorrect IssueIndex (#26279) Close #26277 Fix #26285 --- 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 7f22605d0d..432bf8bf3f 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 7044bcc004..9a42740880 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 b6c6a26cbd..9ca4e21b17 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -248,7 +248,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 }