-
Notifications
You must be signed in to change notification settings - Fork 631
fix: postgres bulk insert issue #1780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThree small changes: cache paging logic now treats out-of-range pages as hits with empty results; GORM DB config sets CreateBatchSize to 1000; version tag bumped from v4.7.10 to v4.7.11. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1780 +/- ##
===========================================
- Coverage 36.39% 36.37% -0.03%
===========================================
Files 248 248
Lines 21328 21329 +1
===========================================
- Hits 7763 7759 -4
- Misses 12741 12746 +5
Partials 824 824
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bridge-history-api/internal/logic/history_logic.go (1)
374-376: Logic change is correct, but consider returning the actual total.Returning
isHit=truefor out-of-range pages is correct—when cached data exists but the requested page exceeds it, we know the answer without a DB fetch.However, returning
0for total instead ofuint64(total)means callers will report 0 items exist rather than the actual count. Compare with line 402 which returnsuint64(total). Clients may need the real total for pagination UI even when requesting an out-of-range page.Consider returning actual total for consistency
if start >= total { - return nil, 0, true, nil + return nil, uint64(total), true, nil }Also, this change appears unrelated to the PR's stated objective of fixing Postgres bulk insert issues. Please confirm if this is intentional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bridge-history-api/internal/logic/history_logic.gocommon/database/db.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: tests
- GitHub Check: check
- GitHub Check: test
🔇 Additional comments (1)
common/database/db.go (1)
53-55: LGTM! Correctly addresses the Postgres 65,535 parameter limit.The
CreateBatchSize: 1000configuration will ensure GORM automatically splits large bulk inserts, preventing failures when inserting many records. With typical table schemas, 1000 records stays safely under the 65,535 parameter limit.
eb3bb4a
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
scroll repo will encounter: postgres will return
extended protocol limited to 65535 parametersissueFix:
output:
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Bug Fixes
Performance
Chores
✏️ Tip: You can customize this high-level summary in your review settings.