Database Performance Optimizations #6

hussam789 · 2025-10-30T21:45:16Z

User description

PR #7

PR Type

Enhancement, Tests

Description

Refactor annotation cleanup to use batched ID fetching and deletion
- Avoid MySQL deadlocks by splitting single-statement queries
- Fetch IDs in memory, then delete in separate batches
Handle SQLite parameter limit (999) for large batch sizes
- Use direct ID insertion when batch size exceeds limit
- Support batch sizes up to 32767 for SQLite >= 3.32.0
Convert cleanup tests to integration tests with parameterized scenarios
- Add test case for large batch size (40003 annotations)
- Bulk insert test data for improved performance
Reduce cleanup job frequency from 10 to 1 minute

Diagram Walkthrough

flowchart LR
  A["Single-statement<br/>DELETE queries"] -->|"Deadlock risk<br/>on MySQL"| B["Split into<br/>ID fetch + delete"]
  B -->|"Batch by ID"| C["fetchIDs"]
  C -->|"Delete batch"| D["deleteByIDs"]
  D -->|"Check DB type"| E{"SQLite with<br/>large batch?"}
  E -->|"Yes"| F["Direct ID<br/>insertion"]
  E -->|"No"| G["SQL parameters"]
  F --> H["Execute delete"]
  G --> H

File Walkthrough

Relevant files

Tests

cleanup_test.go `Convert to integration tests with parameterized scenarios` pkg/services/annotations/annotationsimpl/cleanup_test.go Rename tests to `TestIntegration*` and add skip for short test runs Refactor test structure to use parameterized test cases with customizable batch sizes Add new test case for large batch size (40003 annotations) exceeding SQLite parameter limit Refactor `createTestAnnotations` to bulk insert data in batches for better performance Move test data cleanup into per-test cleanup functions Add `errors` import for error joining	+91/-38

Enhancement

xorm_store.go `Split cleanup queries into batched ID fetch and delete` pkg/services/annotations/annotationsimpl/xorm_store.go Replace single-statement DELETE queries with batched ID fetch and delete approach Add `fetchIDs` method to load annotation IDs into memory respecting batch size Add `deleteByIDs` method with SQLite parameter limit workaround (999 limit) Add `asAny` helper to convert int64 slice to any slice for SQL parameters Extract `untilDoneOrCancelled` as reusable batching loop for multi-statement callbacks Update `CleanAnnotations` and `CleanOrphanedAnnotationTags` to use new batching approach Add import for `migrator` package to detect database type	+107/-23

Configuration changes

cleanup.go `Increase cleanup job frequency` pkg/services/cleanup/cleanup.go Reduce cleanup job ticker interval from 10 minutes to 1 minute	+1/-1

… deadlocks on MySQL (#80329) * Split subquery when cleaning annotations * update comment * Raise batch size, now that we pay attention to it * Iterate in batches * Separate cancellable batch implementation to allow for multi-statement callbacks, add overload for single-statement use * Use split-out utility in outer batching loop so it respects context cancellation * guard against empty queries * Use SQL parameters * Use same approach for tags * drop unused function * Work around parameter limit on sqlite for large batches * Bulk insert test data in DB * Refactor test to customise test data creation * Add test for catching SQLITE_MAX_VARIABLE_NUMBER limit * Turn annotation cleanup test to integration tests * lint --------- Co-authored-by: Sofia Papagiannaki <[email protected]>

qodo-code-review · 2025-10-30T21:45:53Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive information exposure Description: Extensive debug/error logs print full ID lists and conditions for deletions, which may leak internal identifiers and query details into logs; use debug level and avoid logging raw ID arrays. xorm_store.go [533-558] Referred Code } r.log.Error("Annotations to clean by time", "count", len(ids), "ids", ids, "cond", cond, "err", err) x, y := r.deleteByIDs(ctx, "annotation", ids) r.log.Error("cleaned annotations by time", "count", len(ids), "affected", x, "err", y) return x, y }) totalAffected += affected if err != nil { return totalAffected, err } } if cfg.MaxCount > 0 { // Similar strategy as the above cleanup process, to avoid deadlocks. affected, err := untilDoneOrCancelled(ctx, func() (int64, error) { cond := fmt.Sprintf(`%s ORDER BY id DESC %s`, annotationType, r.db.GetDialect().LimitOffset(r.cfg.AnnotationCleanupJobBatchSize, cfg.MaxCount)) ids, err := r.fetchIDs(ctx, "annotation", cond) if err != nil { return 0, err } ... (clipped 5 lines)
	SQL injection Description: Building a DELETE ... IN (...) statement by directly interpolating ID values for SQLite can risk SQL injection if IDs are not strongly typed from trusted sources; ensure IDs originate strictly from prior SELECTs and are not user-controlled. xorm_store.go [606-618] Referred Code // If the batch size is bigger than that, and we're on SQLite, we have to put the IDs directly into the statement. const sqliteParameterLimit = 999 if r.db.GetDBType() == migrator.SQLite && r.cfg.AnnotationCleanupJobBatchSize > sqliteParameterLimit { values := fmt.Sprint(ids[0]) for _, v := range ids[1:] { values = fmt.Sprintf("%s, %d", values, v) } sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, values) } else { placeholders := "?" + strings.Repeat(",?", len(ids)-1) sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, placeholders) args = asAny(ids) }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing Audit Logs: The new cleanup operations that delete annotations and tags do not emit audit logs identifying the actor, timestamp, or action details, which may be required for critical data deletions. Referred Code func (r *xormRepositoryImpl) CleanAnnotations(ctx context.Context, cfg setting.AnnotationCleanupSettings, annotationType string) (int64, error) { var totalAffected int64 if cfg.MaxAge > 0 { cutoffDate := timeNow().Add(-cfg.MaxAge).UnixNano() / int64(time.Millisecond) // Single-statement approaches, specifically ones using batched sub-queries, seem to deadlock with concurrent inserts on MySQL. // We have a bounded batch size, so work around this by first loading the IDs into memory and allowing any locks to flush inside each batch. // This may under-delete when concurrent inserts happen, but any such annotations will simply be cleaned on the next cycle. // // We execute the following batched operation repeatedly until either we run out of objects, the context is cancelled, or there is an error. affected, err := untilDoneOrCancelled(ctx, func() (int64, error) { cond := fmt.Sprintf(`%s AND created < %v ORDER BY id DESC %s`, annotationType, cutoffDate, r.db.GetDialect().Limit(r.cfg.AnnotationCleanupJobBatchSize)) ids, err := r.fetchIDs(ctx, "annotation", cond) if err != nil { return 0, err } r.log.Error("Annotations to clean by time", "count", len(ids), "ids", ids, "cond", cond, "err", err) x, y := r.deleteByIDs(ctx, "annotation", ids) r.log.Error("cleaned annotations by time", "count", len(ids), "affected", x, "err", y) return x, y }) ... (clipped 42 lines)
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Logging As Errors: Operational logs for normal batching flows are written at error level and include raw IDs, which may hinder actionable context and overwhelm error logs; consider structured info/debug logs and clearer messages. Referred Code cond := fmt.Sprintf(`%s AND created < %v ORDER BY id DESC %s`, annotationType, cutoffDate, r.db.GetDialect().Limit(r.cfg.AnnotationCleanupJobBatchSize)) ids, err := r.fetchIDs(ctx, "annotation", cond) if err != nil { return 0, err } r.log.Error("Annotations to clean by time", "count", len(ids), "ids", ids, "cond", cond, "err", err) x, y := r.deleteByIDs(ctx, "annotation", ids) r.log.Error("cleaned annotations by time", "count", len(ids), "affected", x, "err", y) return x, y }) totalAffected += affected if err != nil { return totalAffected, err } } if cfg.MaxCount > 0 { // Similar strategy as the above cleanup process, to avoid deadlocks. affected, err := untilDoneOrCancelled(ctx, func() (int64, error) { cond := fmt.Sprintf(`%s ORDER BY id DESC %s`, annotationType, r.db.GetDialect().LimitOffset(r.cfg.AnnotationCleanupJobBatchSize, cfg.MaxCount)) ... (clipped 32 lines)
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Verbose Error Logs: Logs include internal SQL conditions and lists of database IDs at error level which could expose internal details if surfaced to users; verify these are internal-only and appropriately leveled. Referred Code x, y := r.deleteByIDs(ctx, "annotation", ids) r.log.Error("cleaned annotations by time", "count", len(ids), "affected", x, "err", y) return x, y }) totalAffected += affected if err != nil { return totalAffected, err } } if cfg.MaxCount > 0 { // Similar strategy as the above cleanup process, to avoid deadlocks. affected, err := untilDoneOrCancelled(ctx, func() (int64, error) { cond := fmt.Sprintf(`%s ORDER BY id DESC %s`, annotationType, r.db.GetDialect().LimitOffset(r.cfg.AnnotationCleanupJobBatchSize, cfg.MaxCount)) ids, err := r.fetchIDs(ctx, "annotation", cond) if err != nil { return 0, err } r.log.Error("Annotations to clean by count", "count", len(ids), "ids", ids, "cond", cond, "err", err) ... (clipped 25 lines)
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unstructured Logs: Logging statements output raw slices of IDs and SQL conditions without structured fields or safeguards, risking noisy logs and potential sensitive leakage if IDs are user-derived. Referred Code x, y := r.deleteByIDs(ctx, "annotation", ids) r.log.Error("cleaned annotations by time", "count", len(ids), "affected", x, "err", y) return x, y }) totalAffected += affected if err != nil { return totalAffected, err } } if cfg.MaxCount > 0 { // Similar strategy as the above cleanup process, to avoid deadlocks. affected, err := untilDoneOrCancelled(ctx, func() (int64, error) { cond := fmt.Sprintf(`%s ORDER BY id DESC %s`, annotationType, r.db.GetDialect().LimitOffset(r.cfg.AnnotationCleanupJobBatchSize, cfg.MaxCount)) ids, err := r.fetchIDs(ctx, "annotation", cond) if err != nil { return 0, err } r.log.Error("Annotations to clean by count", "count", len(ids), "ids", ids, "cond", cond, "err", err) ... (clipped 25 lines)
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: SQL Param Bypass: For SQLite with large batches, IDs are interpolated directly into the SQL IN clause instead of using parameters; while IDs come from internal queries, confirm they cannot be influenced externally. Referred Code // If the batch size is bigger than that, and we're on SQLite, we have to put the IDs directly into the statement. const sqliteParameterLimit = 999 if r.db.GetDBType() == migrator.SQLite && r.cfg.AnnotationCleanupJobBatchSize > sqliteParameterLimit { values := fmt.Sprint(ids[0]) for _, v := range ids[1:] { values = fmt.Sprintf("%s, %d", values, v) } sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, values) } else { placeholders := "?" + strings.Repeat(",?", len(ids)-1) sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, placeholders) args = asAny(ids) }

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-10-30T21:46:52Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Use batching to handle SQLite parameter limits Refactor the `deleteByIDs` function to handle SQLite's parameter limit by chunking the IDs into smaller batches and executing separate parameterized queries, instead of embedding IDs directly into the SQL string. Examples: pkg/services/annotations/annotationsimpl/xorm_store.go [605-618] // SQLite has a parameter limit of 999. // If the batch size is bigger than that, and we're on SQLite, we have to put the IDs directly into the statement. const sqliteParameterLimit = 999 if r.db.GetDBType() == migrator.SQLite && r.cfg.AnnotationCleanupJobBatchSize > sqliteParameterLimit { values := fmt.Sprint(ids[0]) for _, v := range ids[1:] { values = fmt.Sprintf("%s, %d", values, v) } sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, values) } else { ... (clipped 4 lines) Solution Walkthrough: Before: func deleteByIDs(ctx, table, ids) (int64, error) { if dbType == SQLite && len(ids) > 999 { // Build SQL string by concatenating IDs values := fmt.Sprint(ids[0]) for _, v := range ids[1:] { values = fmt.Sprintf("%s, %d", values, v) } sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, values) session.Exec(sql) } else { // Use parameterized query placeholders := "?" + strings.Repeat(",?", len(ids)-1) sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, placeholders) session.Exec(sql, ids...) } // ... } After: func deleteByIDs(ctx, table, ids) (int64, error) { const batchSize = 999 // SQLite parameter limit for i := 0; i < len(ids); i += batchSize { end := i + batchSize if end > len(ids) { end = len(ids) } batch := ids[i:end] // Always use parameterized query, but for a smaller batch placeholders := "?" + strings.Repeat(",?", len(batch)-1) sql := fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, placeholders) // Execute delete for the current chunk session.Exec(sql, asAny(batch)...) } // ... } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a security anti-pattern (building SQL queries via string formatting) in the new `deleteByIDs` function and proposes a safer, more robust standard practice.	Medium
General	Use appropriate log level for diagnostics Change the log level from `Error` to `Debug` for routine operational messages to avoid cluttering error logs and triggering false alarms. pkg/services/annotations/annotationsimpl/xorm_store.go [534-537] - r.log.Error("Annotations to clean by time", "count", len(ids), "ids", ids, "cond", cond, "err", err) + r.log.Debug("Annotations to clean by time", "count", len(ids), "ids", ids, "cond", cond, "err", err) x, y := r.deleteByIDs(ctx, "annotation", ids) - r.log.Error("cleaned annotations by time", "count", len(ids), "affected", x, "err", y) + r.log.Debug("cleaned annotations by time", "count", len(ids), "affected", x, "err", y) Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that `log.Error` is misused for diagnostic logging, which can obscure real errors; changing to `log.Debug` is the correct practice.	Medium
General	Improve performance by using strings.Builder Replace the looped `fmt.Sprintf` with `strings.Builder` when constructing the SQL `IN` clause to improve performance and reduce memory allocations. pkg/services/annotations/annotationsimpl/xorm_store.go [608-614] if r.db.GetDBType() == migrator.SQLite && r.cfg.AnnotationCleanupJobBatchSize > sqliteParameterLimit { - values := fmt.Sprint(ids[0]) + var sb strings.Builder + sb.WriteString(strconv.FormatInt(ids[0], 10)) for _, v := range ids[1:] { - values = fmt.Sprintf("%s, %d", values, v) + sb.WriteString(", ") + sb.WriteString(strconv.FormatInt(v, 10)) } - sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, values) + sql = fmt.Sprintf(`DELETE FROM %s WHERE id IN (%s)`, table, sb.String()) } else { Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies an inefficient string-building pattern in a loop and proposes using `strings.Builder`, which is the standard and more performant approach in Go.	Low
More

hussam789 mentioned this pull request Oct 30, 2025

Dual Storage Architecture #5

Open

qodo-code-review bot added the Review effort 3/5 label Oct 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Database Performance Optimizations #6

Database Performance Optimizations #6

Uh oh!

hussam789 commented Oct 30, 2025 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Oct 30, 2025

Uh oh!

qodo-code-review bot commented Oct 30, 2025

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Database Performance Optimizations #6

Are you sure you want to change the base?

Database Performance Optimizations #6

Uh oh!

Conversation

hussam789 commented Oct 30, 2025 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Oct 30, 2025

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Oct 30, 2025

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hussam789 commented Oct 30, 2025 •

edited by qodo-code-review bot

Loading