-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(core): fix spurious "cannot mmap" errors #6104
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
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR removes whole-file mapping helpers across memory classes, switches callers to explicit of(...) mapping with FilesFacade/LPSZ, defers index key-memory resizing to specific extension points, adjusts symbol map reader to map from an offset, and updates tests accordingly (including removing a too-small-file test and updating MV fuzz helpers). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant IndexReader
participant FilesFacade as FF
participant KeyMem as MemoryM
Client->>IndexReader: open(path)
IndexReader->>FF: resolve LPSZ path, getMapPageSize()
IndexReader->>KeyMem: of(FF, name, mapPageSize, start=0 or derived, tag, opts, madvise)
IndexReader->>IndexReader: readIndexMetadataAtomically()
Note right of IndexReader: After metadata read<br/>extend keyMem to getKeyEntryOffset(keyCount)
alt nulls present
IndexReader->>KeyMem: extend(getKeyEntryOffset(keyCountIncludingNulls))
end
Client->>IndexReader: updateKeyCount()
IndexReader->>KeyMem: extend(getKeyEntryOffset(newKeyCount))
sequenceDiagram
autonumber
participant Client
participant SymbolMapReader
participant FilesFacade as FF
participant OffMem as offsetMem
participant CharMem as MemoryM
Client->>SymbolMapReader: open(valuesPath, offsetsPath)
SymbolMapReader->>OffMem: getLong(maxOffset)
SymbolMapReader->>FF: getMapPageSize()
SymbolMapReader->>CharMem: of(FF, valuesPath, mapPageSize, start=maxOffset, tag, opts, madvise)
SymbolMapReader-->>Client: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/io/questdb/cairo/vm/MemoryCMRImpl.java (1)
47-49
: Constructor calls outdated of(...) overload — compile errorof(...) now takes 7 params; the constructor passes 6. Add madviseOpts (e.g., -1).
- of(ff, name, 0, size, memoryTag, 0); + of(ff, name, 0, size, memoryTag, 0, -1);
🧹 Nitpick comments (1)
core/src/main/java/io/questdb/cairo/vm/MemoryCMRImpl.java (1)
110-131
: Make the double ff.length() check explicit; opt/extend params currently unusedThe assertion intentionally calls ff.length(fd) twice. Encode that intent for readability and determinism in debug builds. Also, opts/extendSegmentSize are unused here; fine if by design, but consider a clarifying comment.
- assert !VM_PARANOIA_MODE || newSize <= ff.length(fd) || newSize <= ff.length(fd); // Some tests simulate ff.length() to be 0 once. + // Intentionally call ff.length(fd) twice: some tests simulate a transient 0 once. + final long len1 = ff.length(fd); + final long len2 = ff.length(fd); + assert !VM_PARANOIA_MODE || newSize <= len1 || newSize <= len2;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
core/src/main/java/io/questdb/cairo/AbstractIndexReader.java
(4 hunks)core/src/main/java/io/questdb/cairo/SymbolMapReaderImpl.java
(1 hunks)core/src/main/java/io/questdb/cairo/vm/MemoryCMRImpl.java
(1 hunks)core/src/main/java/io/questdb/cairo/vm/MemoryPMARImpl.java
(0 hunks)core/src/main/java/io/questdb/cairo/vm/NullMemoryCMR.java
(0 hunks)core/src/main/java/io/questdb/cairo/vm/api/MemoryM.java
(0 hunks)core/src/test/java/io/questdb/test/cairo/BitmapIndexTest.java
(17 hunks)core/src/test/java/io/questdb/test/cairo/mv/MatViewFuzzTest.java
(10 hunks)
💤 Files with no reviewable changes (3)
- core/src/main/java/io/questdb/cairo/vm/MemoryPMARImpl.java
- core/src/main/java/io/questdb/cairo/vm/NullMemoryCMR.java
- core/src/main/java/io/questdb/cairo/vm/api/MemoryM.java
⏰ 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). (28)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (12)
core/src/test/java/io/questdb/test/cairo/mv/MatViewFuzzTest.java (5)
73-73
: LGTM! Consistent update to new fuzz parameter API.The change from 2-argument to 3-argument
setFuzzParams
calls throughout the test file is consistent with the new API that includestruncateProb
parameter.
189-189
: Consistent initialization of truncateProb parameter to 0.All test methods now consistently pass
0
as thetruncateProb
parameter insetFuzzParams
calls, maintaining backward compatibility while adapting to the new API.Also applies to: 214-214, 228-228, 398-398, 412-412, 427-427, 441-441, 953-953
626-633
: Good addition of materialized view existence check.Adding a preliminary check to verify that the materialized view exists before validating its status improves error reporting. This will help diagnose issues where views fail to be created entirely, separate from validation failures.
179-179
: Truncate probability enabled in invalidation test.Setting
truncateProb
to 0.5 in the invalidation test is appropriate as truncation should lead to materialized view invalidation, which is what this test is validating.
819-821
: Well-structured API refactoring with backward compatibility.The refactoring preserves the existing 2-argument helper by making it call the full 4-argument version with default values. This maintains backward compatibility while extending the API.
core/src/main/java/io/questdb/cairo/SymbolMapReaderImpl.java (1)
191-199
: Critical fix: Symbol map memory now mapped from correct offset.The change from
charMem.wholeFile()
followed byextendAndMap()
to directly mapping from the offset stored atmaxOffset
is a key fix. This ensures the symbol values memory is correctly positioned, which would explain Windows-specific failures where memory mapping behavior may differ.core/src/test/java/io/questdb/test/cairo/BitmapIndexTest.java (3)
65-65
: LGTM! Import added for LPSZ usage.The addition of the LPSZ import is necessary for the updated memory mapping approach using
path.$()
.Also applies to: 245-247
245-247
: Improved memory mapping approach with explicit parameters.The change from
mem.wholeFile()
tomem.of(...)
with explicit FilesFacade, LPSZ path, and memory parameters aligns with the broader refactoring to use explicit region-based mappings instead of whole-file mappings.
689-690
: Good fix: Update key count before backward scan.Adding
reader.updateKeyCount()
beforelatestScanBackward
ensures the reader has the latest key information. This is important when the index has been modified after the reader was created, preventing potential issues with stale metadata.core/src/main/java/io/questdb/cairo/AbstractIndexReader.java (3)
206-207
: Extending key memory on growth is correctDeferring keyMem.extend() until keyCount increases avoids premature large mappings and matches the writer’s growth.
211-213
: Comment clarity improvementThe triple-check explanation reads clearer now. No functional concerns.
235-235
: Eager resize after header read is appropriateExtending to the computed entry offset here reduces reload races.
The PR title mentions mat views while the issue is not related with mat views, so I'd change the title - any table reader could fail on windows, not only readers open by mat view refresh. |
[PR Coverage check]😍 pass : 18 / 18 (100.00%) file detail
|
found by fuzz tests
This test after 4-6 mins running on Windows
Unrelated test failure: