Repository navigation
Keep the DuckDB spatial index loaded instead of re-reading it - #1057
DarkChromaMC wants to merge 1 commit into
Conversation
A lookup that found no loaded index read the whole index table, and read it again whenever the consumer published a batch during the read. On a busy server with a large index the read never finished and native memory grew until the process was killed. The loaded index was also dropped every time any table completed a segment. The index is now streamed without a sort on its own connection, one lookup loads it at a time, committed segments are appended to it, and only a reset or rebuild makes a lookup read it again. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
👷 Deploy request for coreprotect pending review.Visit the deploys page to approve it
|
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e3487e0a2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| List<PendingSegment> batch = new ArrayList<>(); | ||
| if (state.writeCompleted(connection, prefix, batch)) { | ||
| connection.commit(); | ||
| written.addAll(batch); |
There was a problem hiding this comment.
When begin() must build or substantially catch up a large index, every committed checkpoint is appended here to the method-wide written list and remains reachable until all sources finish. This defeats the INDEX_CHECKPOINT_SEGMENTS memory bound: for a fresh index it retains the entire index's 4/8-KiB Bloom arrays in heap (potentially hundreds of megabytes) instead of allowing each committed batch to be collected, so startup can OOM on the large databases this code targets. Publish each committed batch to an active cache/load as it commits, or retain it only while that is actually necessary.
Useful? React with 👍 / 👎.
|
|
||
| WriterState loaded = loadWriterState(connection, prefix); | ||
| List<PendingSegment> written = new ArrayList<>(); | ||
| WriterState loaded = loadWriterState(connection, prefix, written); |
There was a problem hiding this comment.
If this catch-up has already committed checkpoint batches and a later source/page throws, the call exits before written is passed to addCommittedSegments. A retry skips those batches because indexedThrough() now sees them, and successful initialization no longer clears the cache; therefore, when a lookup cached the older snapshot before or during the catch-up, that cache permanently retains a gap and future lookups scan the entire affected row-ID range. Publish committed checkpoints as they happen or invalidate the cache on this exception path.
Useful? React with 👍 / 👎.
Fixes #1056.
On a large DuckDB server a single lookup could re-read the whole spatial index in a loop and allocate native memory until the process was killed. Details and measurements are in the issue.
What changes (
DuckDBSpatialIndexonly)publish()appends them to the loaded index instead of dropping it. The catch-up scan inbegin()does the same with the segments it writes. A batch that wrote no index rows, including a rolled back one, leaves the loaded index alone.reset(), an externalBuilderwrite, a segment that doesn't follow the loaded ones). Segments committed while a load is running are collected and added to its result.jdbc_stream_resultson and without theORDER BY. The segments are sorted in Java. The duplicated connection also means a lookup that is reading an older snapshot can't cache a stale index.Lookups read the index snapshots without a lock, so appending builds new snapshot objects and swaps them in.
Not changed
Testing
src/testis ignored here. That branch also adds two package-private test hooks.Open points
🤖 Generated with Claude Code