Avoid using TopMemoryContext#451
Conversation
* Eliminate TopMemoryContext usage for spilled and spill-read entries. Entries now live in ApplyReplayContext and are cleaned up by its reset. Spill-read entries during replay are freed immediately after replication_handler, bounding replay memory to one entry at a time. * Fix int overflow on apply_replay_bytes. Was int, wrapped negative at ~2GB causing the spill threshold check to silently fail to activate spilling for very large in-memory queues. Changed to int64. * Simplify apply_replay_queue_append_entry signature from double pointers to direct pass-through, since the entry no longer needs to be relocated across memory contexts when spilled. * Track the in-flight spilled PQ buffer via apply_replay_pending_pqfree so it can be freed if replication_handler throws ERROR before the caller's PQfreemem runs. Defensive guard against malloc leaks on the exception path. * Cleaner separation of stream-path and replay-path entry cleanup in the main apply loop. * Fix mis-indented ConfigReloadPending check.
📝 WalkthroughWalkthroughThe replay-queue and spilling logic is refactored to track libpq "orphan" buffers across handler boundaries. The ChangesReplay-Queue & Spill Memory Management
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
mason-sharp
changed the title
| static int apply_replay_spill_count = 0; | ||
| static int apply_replay_spill_read = 0; | ||
| /* In-flight spilled PQ buffer; freed by reset if ERROR skips PQfreemem. */ | ||
| static char *apply_replay_pending_pqfree = NULL; |
There was a problem hiding this comment.
There is a reason we declined such an approach in Spock before: having free/reset machinery at two levels of the code hierarchy is hard to support and tends to be buggy. For example, this specific implementation doesn't free the entry structure itself after the apply.
One more issue - if spilling fails for some reason, it causes a memory leak of the pq-allocated data.
Another reason why we used TopMemoryContext is to highlight the general memory management issues: MessageContext is a core Context and can't be reallocated/reset by an extension. Multiple context switches are also not nice.
The better approach is to use a SpillMemoryContext that lives at the same level as the message-receiving code, more strictly caps memory usage, and does not depend on the COMMIT command. But it should be introduced after (or with) the memory management refactoring procedure.
2 participants
Eliminate TopMemoryContext usage for spilled and spill-read entries. Entries now live in ApplyReplayContext and are cleaned up by its reset. Spill-read entries during replay are freed immediately after replication_handler, bounding replay memory to one entry at a time.
Fix int overflow on apply_replay_bytes. Was int, wrapped negative at ~2GB causing the spill threshold check to silently fail to activate spilling for very large in-memory queues. Changed to int64.
Simplify apply_replay_queue_append_entry signature from double pointers to direct pass-through, since the entry no longer needs to be relocated across memory contexts when spilled.
Track the in-flight spilled PQ buffer via apply_replay_pending_pqfree so it can be freed if replication_handler throws ERROR before the caller's PQfreemem runs. Defensive guard against malloc leaks on the exception path.
Cleaner separation of stream-path and replay-path entry cleanup in the main apply loop.
Fix mis-indented ConfigReloadPending check.