Postgres multi-rows updates #257
Conversation
WalkthroughThe code refactors several database write methods in the PostgresConnector by replacing explicit transaction handling and per-row prepared statements with single, dynamically constructed multi-row SQL queries. These changes affect insert and delete operations for block failures and staging data, consolidating multiple operations into single batched queries and simplifying control flow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/storage/postgres.go (1)
238-263: Consider error handling for JSON marshaling in batch context.While the individual JSON marshaling error handling is correct, a single marshaling failure will cause the entire batch to fail. Consider whether partial success handling would be beneficial.
If partial success is desired, consider pre-validating JSON marshaling:
+ // Pre-validate all JSON marshaling to avoid partial failures + jsonData := make([]string, len(data)) for i, blockData := range data { blockDataJSON, err := json.Marshal(blockData) if err != nil { - return err + return fmt.Errorf("failed to marshal block data at index %d: %w", i, err) } + jsonData[i] = string(blockDataJSON) + } + + for i, blockData := range data { valueStrings = append(valueStrings, fmt.Sprintf("($%d, $%d, $%d)", i*3+1, i*3+2, i*3+3)) valueArgs = append(valueArgs, blockData.Block.ChainId.String(), blockData.Block.Number.String(), - string(blockDataJSON), + jsonData[i], ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/storage/postgres.go(3 hunks)
🔇 Additional comments (5)
internal/storage/postgres.go (5)
154-167: LGTM! Efficient parameter placeholder generation.The dynamic SQL construction with numbered placeholders ($1, $2, etc.) correctly prevents SQL injection while building efficient multi-row INSERT statements. The parameter indexing logic
i*5+1throughi*5+5is accurate for the 5 columns per row.
187-200: LGTM! Clean tuple-based DELETE implementation.The multi-row DELETE using tuple syntax
(chain_id, block_number) IN (($1, $2), ($3, $4), ...)is efficient and properly parameterized. The indexing logici*2+1, i*2+2correctly handles the 2 parameters per tuple.
331-344: LGTM! Consistent DELETE pattern implementation.The implementation follows the same efficient pattern as
DeleteBlockFailures, using tuple-based deletion with proper parameter indexing. The code is clean and SQL injection-safe.
153-179: Ensure batch size stays within PostgreSQL’s 65,535-parameter limitPostgreSQL permits at most 65,535 parameters per statement. Since you’re using 5 parameters per failure row, each batch must contain at most 13,107 entries. Otherwise you’ll hit a “too many parameters” error.
Suggested fixes:
- File: internal/storage/postgres.go (lines ~153–179)
• Before buildingvalueStrings/valueArgs, splitfailuresinto chunks ofmaxRows = 65535/5 = 13107.
• Loop over each chunk and execute an INSERT … ON CONFLICT for that subset.Example patch:
@@ internal/storage/postgres.go:150 - // Build multi-row INSERT without transaction for better performance - valueStrings := make([]string, 0, len(failures)) - valueArgs := make([]interface{}, 0, len(failures)*5) - - for i, failure := range failures { +const ( + maxParams = 65535 + paramsPerRow = 5 + maxRowsPerBatch = maxParams / paramsPerRow // 13107 +) + +// Split failures into batches to avoid exceeding PostgreSQL’s parameter limit +for start := 0; start < len(failures); start += maxRowsPerBatch { + end := start + maxRowsPerBatch + if end > len(failures) { + end = len(failures) + } + batch := failures[start:end] + valueStrings := make([]string, 0, len(batch)) + valueArgs := make([]interface{}, 0, len(batch)*paramsPerRow) + + for i, failure := range batch { valueStrings = append(valueStrings, fmt.Sprintf("($%d, $%d, $%d, $%d, $%d)", i*5+1, i*5+2, i*5+3, i*5+4, i*5+5)) valueArgs = append(valueArgs, failure.ChainId.String(), failure.BlockNumber.String(), failure.FailureTime.Unix(), failure.FailureCount, failure.FailureReason, ) } + + query := fmt.Sprintf(`INSERT INTO block_failures (chain_id, block_number, last_error_timestamp, failure_count, reason) + VALUES %s + ON CONFLICT (chain_id, block_number) + DO UPDATE SET + last_error_timestamp = EXCLUDED.last_error_timestamp, + failure_count = EXCLUDED.failure_count, + reason = EXCLUDED.reason, + updated_at = NOW()`, strings.Join(valueStrings, ",")) + + if _, err := p.db.Exec(query, valueArgs...); err != nil { + return err + } +} +return nil[fix_required]
153-179: StoreBlockFailures remains atomic without explicit transaction
- A search for
StoreBlockFailuresusages found no callers grouping multiple methods into one transaction.- PostgreSQL guarantees per-statement atomicity for
INSERT … ON CONFLICT, so removing the explicit transaction wrapper does not weaken consistency here.
Summary by CodeRabbit