Fix/db storage - #111
Conversation
Offload Git Diff Storage to External Blob StoreOverviewThis change moves large Git code diffs from PostgreSQL TOAST storage to external Blob Storage. It reduces database size, limits River background job retention to 30 days, and implements a dual-read fallback strategy to maintain backward compatibility. Technical Highlights
Impact
|
|
|
||
| func (s *Server) fetchPreloadedChanges(ctx context.Context, orgID, reviewID int64, meta map[string]interface{}) ([]models.CodeDiff, error) { | ||
| // 1. Try reading from Blob Storage first (org/<org_id>/review/<review_id>/artifacts/preloaded_changes.json) | ||
| rawBlob, err := blobstore.ReadArtifact(ctx, s.db, orgID, reviewID, blobstore.ArtifactPreloadedChanges) |
There was a problem hiding this comment.
Severity: warning
This block silently ignores all blob read errors. Please log any errors other than NotFound to prevent masking potential storage failures.
Suggestions:
- Check if the error is a 'not found' error using
blobstore.IsNotExist(err). - Log non-NotFound errors (e.g., network timeouts, expired credentials) to prevent silent failures that mask underlying storage issues.
| if err != nil { | ||
| log.Printf("[WARN] Failed to marshal diffs for review %d: %v", args.ReviewID, err) | ||
| } else { | ||
| if err := blobstore.SaveArtifact(ctx, w.db, args.OrgID, args.ReviewID, blobstore.ArtifactPreloadedChanges, modelDiffsPayload); err != nil { |
There was a problem hiding this comment.
Severity: warning
The UI polling API must read preloaded_changes from the blobstore; otherwise, the UI may break if the data is missing from the database metadata.
Suggestions:
- Ensure the GET/polling endpoint for reviews checks blob storage if preloaded_changes is not in DB metadata.
| }); err != nil { | ||
| log.Printf("[WARN] failed to store preloaded_changes for review %d: %v", args.ReviewID, err) | ||
| } | ||
| if !savedToBlob { |
There was a problem hiding this comment.
Severity: warning
Please set preloaded_changes to nil in metaUpdates on success to ensure that stale database values are cleared.
Suggestions:
- Set
metaUpdates[blobstore.MetaPreloadedChanges] = nilwhensavedToBlobis true to clear any stale preloaded_changes from the database.
| queryCtx, cancel := context.WithTimeout(ctx, 30*time.Minute) | ||
| defer cancel() | ||
|
|
||
| rows, err := db.QueryContext(queryCtx, query) |
There was a problem hiding this comment.
Severity: warning
This query does not implement pagination. Loading all rows at once could lead to out-of-memory (OOM) errors when dealing with large datasets.
Suggestions:
- Implement keyset pagination (e.g.,
WHERE id > $1 LIMIT 1000) to process rows in batches. - Avoid loading the entire dataset into memory at once.
| ORDER BY id ASC | ||
| ` | ||
|
|
||
| queryCtx, cancel := context.WithTimeout(ctx, 30*time.Minute) |
There was a problem hiding this comment.
Severity: warning
There is a hardcoded 30-minute timeout on queryCtx, which will abort the migration if the streaming process takes longer than 30 minutes.
Suggestions:
- Remove the hardcoded timeout or make it configurable via a command-line flag.
- Use batching/pagination to avoid a single long-running query.
| item.preloadedBytes = rawData | ||
| jobChan <- item | ||
| } | ||
| if err := rows.Err(); err != nil { |
There was a problem hiding this comment.
Severity: critical
Although rows.Err() is logged, the error is otherwise ignored, which causes the script to exit with a status of 0 even in the event of a partial failure.
Suggestions:
- Track query/iteration errors in a shared variable or channel.
- Ensure the script exits with a non-zero status code if
rows.Err()is non-nil.
2 participants
Summary
Describe what changed and why.
Linked Issue
Link the agreed issue this PR fulfills.
Validation
Describe the most specific test or validation you ran.
Checklist
SECURITY.mdand updated related documentation if needed.Notes For Reviewers
Anything specific you want reviewers to focus on.