delete old dashboard versions in multiple batches (#23348)

pkg/services/sqlstore/dashboard_version.go

@@ -67,49 +67,68 @@ func GetDashboardVersions(query *models.GetDashboardVersionsQuery) error {
 	return nil
 }
 
-const MAX_VERSIONS_TO_DELETE = 100
+const MAX_VERSIONS_TO_DELETE_PER_BATCH = 100
+const MAX_VERSION_DELETION_BATCHES = 50
 
 func DeleteExpiredVersions(cmd *models.DeleteExpiredVersionsCommand) error {
-	return inTransaction(func(sess *DBSession) error {
-		versionsToKeep := setting.DashboardVersionsToKeep
-		if versionsToKeep < 1 {
-			versionsToKeep = 1
-		}
+	return deleteExpiredVersions(cmd, MAX_VERSIONS_TO_DELETE_PER_BATCH, MAX_VERSION_DELETION_BATCHES)
+}
 
-		// Idea of this query is finding version IDs to delete based on formula:
-		// min_version_to_keep = min_version + (versions_count - versions_to_keep)
-		// where version stats is processed for each dashboard. This guarantees that we keep at least versions_to_keep
-		// versions, but in some cases (when versions are sparse) this number may be more.
-		versionIdsToDeleteQuery := `SELECT id
-			FROM dashboard_version, (
-				SELECT dashboard_id, count(version) as count, min(version) as min
-				FROM dashboard_version
-				GROUP BY dashboard_id
-			) AS vtd
-			WHERE dashboard_version.dashboard_id=vtd.dashboard_id
-			AND version < vtd.min + vtd.count - ?`
-
-		var versionIdsToDelete []interface{}
-		err := sess.SQL(versionIdsToDeleteQuery, versionsToKeep).Find(&versionIdsToDelete)
-		if err != nil {
-			return err
-		}
+func deleteExpiredVersions(cmd *models.DeleteExpiredVersionsCommand, perBatch int, maxBatches int) error {
+	versionsToKeep := setting.DashboardVersionsToKeep
+	if versionsToKeep < 1 {
+		versionsToKeep = 1
+	}
 
-		// Don't delete more than MAX_VERSIONS_TO_DELETE version per time
-		if len(versionIdsToDelete) > MAX_VERSIONS_TO_DELETE {
-			versionIdsToDelete = versionIdsToDelete[:MAX_VERSIONS_TO_DELETE]
-		}
+	for batch := 0; batch < maxBatches; batch++ {
+		deleted := int64(0)
+
+		batchErr := inTransaction(func(sess *DBSession) error {
+			// Idea of this query is finding version IDs to delete based on formula:
+			// min_version_to_keep = min_version + (versions_count - versions_to_keep)
+			// where version stats is processed for each dashboard. This guarantees that we keep at least versions_to_keep
+			// versions, but in some cases (when versions are sparse) this number may be more.
+			versionIdsToDeleteQuery := `SELECT id
+				FROM dashboard_version, (
+					SELECT dashboard_id, count(version) as count, min(version) as min
+					FROM dashboard_version
+					GROUP BY dashboard_id
+				) AS vtd
+				WHERE dashboard_version.dashboard_id=vtd.dashboard_id
+				AND version < vtd.min + vtd.count - ?
+				LIMIT ?`
+
+			var versionIdsToDelete []interface{}
+			err := sess.SQL(versionIdsToDeleteQuery, versionsToKeep, perBatch).Find(&versionIdsToDelete)
+			if err != nil {
+				return err
+			}
+
+			if len(versionIdsToDelete) < 1 {
+				return nil
+			}
 
-		if len(versionIdsToDelete) > 0 {
 			deleteExpiredSql := `DELETE FROM dashboard_version WHERE id IN (?` + strings.Repeat(",?", len(versionIdsToDelete)-1) + `)`
 			sqlOrArgs := append([]interface{}{deleteExpiredSql}, versionIdsToDelete...)
 			expiredResponse, err := sess.Exec(sqlOrArgs...)
 			if err != nil {
 				return err
 			}
-			cmd.DeletedRows, _ = expiredResponse.RowsAffected()
+
+			deleted, err = expiredResponse.RowsAffected()
+			return err
+		})
+
+		if batchErr != nil {
+			return batchErr
 		}
 
-		return nil
-	})
+		cmd.DeletedRows += deleted
+
+		if deleted < int64(perBatch) {
+			break
+		}
+	}
+
+	return nil
 }

pkg/services/sqlstore/dashboard_version_test.go

@@ -131,7 +131,7 @@ func TestDeleteExpiredVersions(t *testing.T) {
 			So(query.Result[0].Version, ShouldEqual, versionsToWrite)
 		})
 
-		Convey("Don't delete anything if there're no expired versions", func() {
+		Convey("Don't delete anything if there are no expired versions", func() {
 			setting.DashboardVersionsToKeep = versionsToWrite
 
 			err := DeleteExpiredVersions(&models.DeleteExpiredVersionsCommand{})
@@ -144,15 +144,18 @@ func TestDeleteExpiredVersions(t *testing.T) {
 			So(len(query.Result), ShouldEqual, versionsToWrite)
 		})
 
-		Convey("Don't delete more than MAX_VERSIONS_TO_DELETE per iteration", func() {
-			versionsToWriteBigNumber := MAX_VERSIONS_TO_DELETE + versionsToWrite
+		Convey("Don't delete more than MAX_VERSIONS_TO_DELETE_PER_BATCH * MAX_VERSION_DELETION_BATCHES per iteration", func() {
+			perBatch := 10
+			maxBatches := 10
+
+			versionsToWriteBigNumber := perBatch*maxBatches + versionsToWrite
 			for i := 0; i < versionsToWriteBigNumber-versionsToWrite; i++ {
 				updateTestDashboard(savedDash, map[string]interface{}{
 					"tags": "different-tag",
 				})
 			}
 
-			err := DeleteExpiredVersions(&models.DeleteExpiredVersionsCommand{})
+			err := deleteExpiredVersions(&models.DeleteExpiredVersionsCommand{}, perBatch, maxBatches)
 			So(err, ShouldBeNil)
 
 			query := models.GetDashboardVersionsQuery{DashboardId: savedDash.Id, OrgId: 1, Limit: versionsToWriteBigNumber}
@@ -161,8 +164,8 @@ func TestDeleteExpiredVersions(t *testing.T) {
 
 			// Ensure we have at least versionsToKeep versions
 			So(len(query.Result), ShouldBeGreaterThanOrEqualTo, versionsToKeep)
-			// Ensure we haven't deleted more than MAX_VERSIONS_TO_DELETE rows
-			So(versionsToWriteBigNumber-len(query.Result), ShouldBeLessThanOrEqualTo, MAX_VERSIONS_TO_DELETE)
+			// Ensure we haven't deleted more than perBatch * maxBatches rows
+			So(versionsToWriteBigNumber-len(query.Result), ShouldBeLessThanOrEqualTo, perBatch*maxBatches)
 		})
 	})
 }