chore(core): release disk space faster on table drop #6585
chore(core): release disk space faster on table drop #6585bluestreak01 merged 19 commits intomasterfrom
Conversation
WalkthroughAdded configurable pool segment size to QuestDB's multi-tenant pool infrastructure, replacing hardcoded ENTRY_SIZE values. Extended CairoConfiguration with getPoolSegmentSize(), threaded rootEntry tracking through pool tenant implementations, and enhanced drop notification handling in CairoEngine. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/src/main/java/io/questdb/griffin/engine/table/ReaderPoolRecordCursorFactory.java (1)
62-67: DynamicpoolSegmentSizeintegration in reader‑pool cursor looks sound.Passing
configuration.getPoolSegmentSize()intoReaderPoolCursorand using it inselectPoolEntry()cleanly replaces the previous hardcoded entry size while preserving the iteration pattern across chainedEntrys. WithpoolSegmentSizevalidated at configuration time, this cursor logic should behave as before for default settings and respect custom segment sizes correctly.Also applies to: 79-88, 123-127, 146-168
core/src/main/java/io/questdb/cairo/pool/SqlCompilerPool.java (1)
62-67: Sql‑compiler pool now respects configurable segment size; root‑entry plumbing is correct.Using
getSqlCompilerPoolCapacity() / getPoolSegmentSize() + 1for the segment count makes the pool honor the new configurable segment size while preserving the previous over‑provisioning behavior. The addedrootEntryfield onCis threaded correctly fromnewTenant(...)and exposed viagetRootEntry()in line with other pool tenants.Also applies to: 90-99, 101-123, 192-196
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java (2)
533-555: Well-designed race handling between returnToPool and notifyDropped.The dropped-table handling correctly manages the race between
returnToPoolandnotifyDropped:
- Scenario 1 (returnToPool wins): Re-acquires slot, cleans up pool state, returns false → caller closes tenant
- Scenario 2 (notifyDropped wins): Detects tenant already closed (slot is null), returns true → prevents double-close
The logic properly coordinates resource cleanup through the return value:
true= already closed, caller should skipfalse= not closed yet, caller should closeThe double-close prevention via checking
e.getTenant(index) != tenant(line 540) is a good defensive pattern.The comment at line 523 appears incomplete: "Check if entry is being locked or dropped by checking the first entry's lockOwner". Consider completing or removing this comment for clarity.
169-207: Well-structured drop notification with proper race handling.The
notifyDropped()method correctly:
- Marks entries as dropped via the volatile
droppedflag (line 177)- Uses CAS to safely acquire slots before closing tenants (line 185)
- Handles the NEXT_ALLOCATED race condition by spinning (lines 196-202)
- Provides defensive exception handling with critical logging (lines 188-191)
The distinction between
fullDropped=true(removes from map) vsfullDropped=false(keeps in map) is properly applied by callers: CairoEngine invokes withfalseduringnotifyDropped()(TRUNCATE) and withtrueduringremoveTableToken()(full DROP).Consider adding detailed Javadoc to document:
- The
fullDroppedparameter semantics (DROP vs TRUNCATE)- Thread-safety guarantees
- Expected caller context (e.g., must hold sequencer WRITE lock)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
core/src/main/java/io/questdb/PropServerConfiguration.javacore/src/main/java/io/questdb/PropertyKey.javacore/src/main/java/io/questdb/cairo/CairoConfiguration.javacore/src/main/java/io/questdb/cairo/CairoConfigurationWrapper.javacore/src/main/java/io/questdb/cairo/CairoEngine.javacore/src/main/java/io/questdb/cairo/DefaultCairoConfiguration.javacore/src/main/java/io/questdb/cairo/TxnScoreboardPoolV2.javacore/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.javacore/src/main/java/io/questdb/cairo/pool/PoolConstants.javacore/src/main/java/io/questdb/cairo/pool/PoolListener.javacore/src/main/java/io/questdb/cairo/pool/PoolTenant.javacore/src/main/java/io/questdb/cairo/pool/ReaderPool.javacore/src/main/java/io/questdb/cairo/pool/SequencerMetadataPool.javacore/src/main/java/io/questdb/cairo/pool/SqlCompilerPool.javacore/src/main/java/io/questdb/cairo/pool/TableMetadataPool.javacore/src/main/java/io/questdb/cairo/pool/TableReaderMetadataTenantImpl.javacore/src/main/java/io/questdb/cairo/pool/WalWriterPool.javacore/src/main/java/io/questdb/cairo/wal/WalWriter.javacore/src/main/java/io/questdb/griffin/engine/table/ReaderPoolRecordCursorFactory.javacore/src/test/java/io/questdb/test/ServerMainTest.javacore/src/test/java/io/questdb/test/cairo/TxnScoreboardTest.javacore/src/test/java/io/questdb/test/cairo/fuzz/FuzzRunner.javacore/src/test/java/io/questdb/test/cairo/wal/WalTableSqlTest.javacore/src/test/java/io/questdb/test/griffin/ReaderPoolTableFunctionTest.java
💤 Files with no reviewable changes (1)
- core/src/test/java/io/questdb/test/cairo/wal/WalTableSqlTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T12:21:00.062Z
Learnt from: jerrinot
Repo: questdb/questdb PR: 6413
File: core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java:11982-12002
Timestamp: 2025-11-19T12:21:00.062Z
Learning: QuestDB Java tests use a deterministic random seed. The test utilities (e.g., io.questdb.test.tools.TestUtils and io.questdb.std.Rnd) produce reproducible sequences, so rnd_* functions (including rnd_uuid4) yield deterministic outputs across runs. Do not flag tests in core/src/test/** that assert against values produced by rnd_* as flaky due to randomness.
Applied to files:
core/src/test/java/io/questdb/test/ServerMainTest.java
🧬 Code graph analysis (1)
core/src/main/java/io/questdb/cairo/CairoEngine.java (1)
core/src/main/java/io/questdb/cairo/CairoException.java (1)
CairoException(39-429)
⏰ 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). (48)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (Hosted Running tests on windows-other-2)
- GitHub Check: New pull request (Hosted Running tests on windows-other-1)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
- 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-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- 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 on mac-griffin)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin 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 tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: New pull request
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (37)
core/src/test/java/io/questdb/test/cairo/fuzz/FuzzRunner.java (1)
969-973: Method signature changed from boolean-returning to void-returning; confirm the new error handling approach is intentional.The
goActive()method previously returned a boolean in commit f3be96a, with explicit error handling viaif (!walWriter.goActive(...)) throw. The current implementation uses a void-returning signature with exception-based error handling. This architectural change shifts error detection from the return value to exception throwing (on table drops) and the fallback metadata version check.Verify that this refactoring from boolean returns to exception-based error handling is intentional and that the secondary metadata version check provides sufficient validation in all failure scenarios, particularly for concurrent table drop operations.
core/src/main/java/io/questdb/cairo/pool/PoolConstants.java (1)
29-44: LGTM!The new
CR_DROPPEDconstant is correctly added with value 6, and the switch expression cleanly handles the new case alongside existing close reasons.core/src/main/java/io/questdb/PropertyKey.java (1)
621-622: LGTM!The new debug property follows QuestDB's naming conventions and is appropriately marked as a debug configuration option.
core/src/test/java/io/questdb/test/ServerMainTest.java (2)
160-250: Well-structured concurrency test.The test properly exercises the new drop notification handling with concurrent WAL writes. Good use of
ConcurrentIntHashMapfor tracking, proper exception handling for dropped tables, and cleanup withMisc.freeObjListandPath.clearThreadLocals().
1053-1065: LGTM!The helper method correctly manages the
WalWriterlifecycle, ensuring it's freed on exceptions before re-throwing.core/src/main/java/io/questdb/cairo/DefaultCairoConfiguration.java (1)
758-761: LGTM!The default pool segment size of 32 aligns with the configuration interface contract and follows the existing pattern in this class.
core/src/main/java/io/questdb/cairo/pool/TableReaderMetadataTenantImpl.java (2)
52-77: LGTM!The
rootEntryfield correctly maintains a stable reference to the original segment root, separate from the mutableentryfield. This enables consistent drop detection during concurrent operations.
108-111: LGTM!The
getRootEntry()accessor completes thePoolTenantinterface implementation for root-entry-aware lifecycle checks.core/src/main/java/io/questdb/cairo/pool/PoolListener.java (1)
33-33: LGTM!The new
EV_DROPPEDevent constant follows the existing numbering sequence and enables drop notifications in the pool listener system.core/src/test/java/io/questdb/test/griffin/ReaderPoolTableFunctionTest.java (2)
118-118: LGTM!Using
configuration.getPoolSegmentSize()instead of the hardcoded constant aligns the test with the new configurable pool segment sizing.
184-184: LGTM!Consistent update to use the configuration-based pool segment size.
core/src/main/java/io/questdb/cairo/pool/PoolTenant.java (1)
50-56: LGTM!The new
getRootEntry()method is a clean addition to thePoolTenantinterface, following the same pattern as the existinggetEntry()method. The Javadoc clearly explains the purpose—enabling drop status checks via the root entry of the entry chain.core/src/main/java/io/questdb/cairo/CairoConfigurationWrapper.java (1)
734-737: LGTM!The delegation follows the established pattern used throughout this wrapper class.
core/src/main/java/io/questdb/cairo/CairoConfiguration.java (1)
450-451: LGTM!The new configuration method follows the interface's conventions. Based on the AI summary, implementations provide a default value of 32, which aligns with the pool sizing requirements.
core/src/main/java/io/questdb/cairo/TxnScoreboardPoolV2.java (1)
42-42: LGTM!The change from a hardcoded
ENTRY_SIZEtoconfiguration.getPoolSegmentSize()aligns with the PR's goal of making pool sizing configurable. The multiplicationgetReaderPoolMaxSegments() * getPoolSegmentSize()correctly calculates the total entry count for the scoreboard.core/src/main/java/io/questdb/cairo/CairoEngine.java (3)
1079-1094: Improved error handling for concurrent table drop scenarios.The new try-catch block properly distinguishes between different failure modes when acquiring a WAL writer:
- If the table is already marked as dropped → throws
tableDropped- If the table was concurrently dropped/locked → throws
tableDoesNotExist- Otherwise → rethrows the original exception
This aligns with the PR objective of handling concurrent drops gracefully.
Minor: Line 1083 has a typo—"not fund" should be "not found".
🔎 Suggested typo fix
- if (isTableDropped(tableToken)) { - // If table is dropped we can have some file not fund errors, + if (isTableDropped(tableToken)) { + // If table is dropped we can have some file not found errors,
1294-1309: LGTM — Drop notifications now propagate to all relevant pools.The
notifyDroppedmethod now notifies all four pools (reader, walWriter, tableMetadata, sequencerMetadata) withfalse, which enables idle WAL writers to be closed earlier when a table is dropped. This directly addresses the PR objective of releasing disk space faster.
1421-1438: LGTM — Token removal includes pool cleanup.The
removeTableTokenmethod properly propagates drop notifications withtrueto all pools, which ensures entries are removed from the pool maps. This differs from the regularnotifyDropped(TableToken)method (line 1294) which passesfalse, keeping entries in the map for cases like truncate operations. The semantic distinction is consistent across all pool implementations (ReaderPool, WalWriterPool, TableMetadataPool, SequencerMetadataPool), which inherit the same behavior from AbstractMultiTenantPool wherefullDropped=truetriggersentries.remove(token.getDirName()).core/src/test/java/io/questdb/test/cairo/TxnScoreboardTest.java (2)
498-500: LGTM!The simplified helper method
assertScoreboardMinOrNoLocks(TxnScoreboard scoreboard)is cleaner. It correctly verifies that the scoreboard has no active locks (min == -1) without requiring an unused txn parameter.
506-506: LGTM — Test updated to use configurable pool segment size.The test now correctly uses
configuration.getPoolSegmentSize()instead of the hardcodedReaderPool.ENTRY_SIZE, aligning with the production code changes.core/src/main/java/io/questdb/cairo/pool/TableMetadataPool.java (1)
43-51: LGTM!The
newTenantmethod signature update correctly adds therootEntryparameter and propagates it to theTableReaderMetadataTenantImplconstructor. This enables the tenant to track its root entry for drop-status checks, consistent with thePoolTenant.getRootEntry()interface addition.core/src/main/java/io/questdb/cairo/wal/WalWriter.java (2)
451-461: LGTM with note on distressed flag behavior.The error handling correctly sets
distressed = truebefore checking if the table was dropped. This ensures the WAL writer is marked as distressed in both scenarios (table dropped and other failures), which aligns with the PR's goal of closing WAL writers faster on table drop. The immediate rethrow for dropped tables without logging reduces noise.
447-447: All callers are compatible with the signature change tovoid.Search across the codebase found no call sites that assign or conditionally check the
goActivereturn value. All callers simply invoke the method without using any return value, making the signature change frombooleantovoidsafe and compatible with existing code.core/src/main/java/io/questdb/cairo/pool/WalWriterPool.java (1)
52-71: Root‑entry propagation forWalWriterTenantlooks consistent with pool design.
rootEntryis threaded fromnewTenant(...)intoWalWriterTenant, stored asfinal, and exposed viagetRootEntry()without affecting existing close/return semantics. This aligns with the other pool tenants’ changes and shouldn’t introduce new lifecycle issues.Also applies to: 79-101, 136-139
core/src/main/java/io/questdb/cairo/pool/SequencerMetadataPool.java (1)
56-65: Consistent root‑entry wiring for sequencer metadata tenant.The added
rootEntryparameter and getter onSequencerMetadataTenantImplare wired correctly fromnewTenant(...)and don’t alter existing close/refresh behavior. This keeps the tenant aligned with the broader pool/rootEntry pattern introduced in the PR.Also applies to: 67-96, 151-155
core/src/main/java/io/questdb/cairo/pool/ReaderPool.java (3)
102-108: LGTM! Method signatures correctly updated to thread rootEntry.The addition of the
rootEntryparameter to bothnewCopyOfTenantandnewTenantaligns with the parent class changes and enables drop detection during concurrent operations.
120-120: LGTM! rootEntry field and getter properly implemented.The
rootEntryfield and corresponding getter correctly implement the PoolTenant interface requirement for tracking the root entry, which is essential for concurrent drop detection.Also applies to: 219-222
131-171: LGTM! Constructors properly updated for dynamic sizing and rootEntry tracking.Both constructors correctly:
- Accept and store the
rootEntryparameter- Replace hardcoded
ENTRY_SIZEwithpool.getSegmentSize()for dynamic segment sizing- Thread parameters consistently through the inheritance chain
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java (9)
45-47: LGTM! Configurable segment size properly introduced.The addition of:
- Constants (
LOCKED,POOL_SIZE) for consistent error messages- Configurable
segmentSizefield sourced from configuration- Dynamic
maxEntriescalculationThis replaces the previous hardcoded
ENTRY_SIZEapproach and enables flexible pool sizing.Also applies to: 59-59, 64-65
91-91: LGTM! Segment size properly exposed and consistently used.The
getSegmentSize()getter is added and all iteration loops correctly updated from hardcodedENTRY_SIZEto dynamicsegmentSize. The changes are mechanical and consistent throughout.Also applies to: 106-108, 119-119, 184-184, 272-272, 461-461
126-126: LGTM! Improved readability with named constant.Replacing the magic number
-1with theUNLOCKEDconstant improves code readability while maintaining the same semantics.Also applies to: 135-135
257-282: LGTM! Dropped table check properly narrows race window.The addition of
rootEntrytracking and the dropped-table check (lines 274-282) correctly:
- Distinguishes between TRUNCATE (dropped=true, entry in map) and DROP (dropped=true, entry removed)
- Checks the dropped state after CAS allocation, narrowing the race window
- Properly cleans up the allocation before throwing
- Relies on sequencer WRITE lock for ultimate correctness guarantee
The logic at line 279 correctly uses inequality comparison to detect if the entry was removed and replaced, allowing operations to proceed with a newly created table of the same name.
294-296: LGTM! Tenant creation correctly passes rootEntry.Both
newCopyOfTenantandnewTenantcalls are properly updated to pass therootEntryparameter, enabling tenants to detect concurrent drop operations.
350-350: LGTM! Entry construction and NEXT_LOCKED handling properly implemented.The changes correctly:
- Pass
segmentSizeto Entry constructor for dynamic sizing (lines 350, 380)- Handle the
NEXT_LOCKEDstate introduced by thelock()method (lines 356-361)- Throw
EntryLockedExceptionwith meaningfulLOCKEDconstant when expansion is blockedThis prevents
get()operations from proceeding when a table is locked.Also applies to: 356-361, 380-380
372-372: LGTM! More accurate error message constant.Using
POOL_SIZEinstead ofNO_LOCK_REASONprovides a more accurate error message for the pool size exceeded scenario.
575-596: LGTM! Entry class properly updated for dynamic sizing and drop tracking.The Entry class changes correctly implement:
- Dynamic sizing: Arrays allocated based on
entrySizeparameter (lines 590-592)- Drop tracking: Public volatile
droppedfield for lock-free visibility (line 580)- CAS compatibility:
nextStatusmarked volatile with clear documentation explaining why it cannot be final (lines 583-586)The
@SuppressWarnings("FieldMayBeFinal")annotation is appropriately used sincenextStatusis modified via CAS operations.
433-449: LGTM! Abstract method signatures correctly extended.Both
newCopyOfTenantandnewTenantabstract method signatures properly updated to include theEntry<T> rootEntryparameter, ensuring all pool implementations thread root entry information to their tenants.
bluestreak01
changed the title
[PR Coverage check]😍 pass : 121 / 136 (88.97%) file detail
|
3 participants
This change closes WAL writers faster, which helps delete them from disk.
Before the change, when a table is dropped, there are a lot of logs like this:
Now, when idle WAL writers are closed at the time of the drop, the WAL and table files are deleted faster.
This is another try of #6555 that is reverted. Here, the test failures are fixed by throwing the correct exception when a WAL writer is being created concurrently with table drop.