Skip to content

feat(core): release disk space faster on table drop#6555

Merged
bluestreak01 merged 8 commits intomasterfrom
feat-drop-table-faster
Dec 28, 2025
Merged

feat(core): release disk space faster on table drop#6555
bluestreak01 merged 8 commits intomasterfrom
feat-drop-table-faster

Conversation

@ideoma
Copy link
Copy Markdown
Collaborator

@ideoma ideoma commented Dec 18, 2025

This change closes WAL writers faster that helps deleting them from disk.

Before the change when table is dropped, there are a lot of logging like this:

2025-12-17T15:05:03.179283Z I i.q.c.w.WalPurgeJob table is dropped, but has WALs containing segments with pending tasks [table=dbRoot/price_1h~12]

Now when idle WAL writers are closed at the time of the drop, the WAL and table files are deleted faster.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 18, 2025 •
edited
Loading

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

When a table is dropped, CairoEngine now notifies the reader pool and WAL writer pool through new notifyDropped() calls. Pools clean up allocated resources for the dropped table via a new notification method that iterates through allocations, performs compare-and-swap operations, closes tenants, and marks slots as unallocated. A new close reason constant CR_DROPPED is added to track these cleanup operations.

Changes

Cohort / File(s) Summary
Pool notification dispatch
core/src/main/java/io/questdb/cairo/CairoEngine.java		 Invokes readerPool.notifyDropped(tableToken) and walWriterPool.notifyDropped(tableToken) during table drop before existing post-drop processing
Pool resource cleanup
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java		 Adds public notifyDropped(TableToken token) method that iterates pool entries, performs CAS operations to move allocations from allocated to thread state, closes tenants with EV_EXPIRE event and idle close reason, and marks slots as UNALLOCATED
Pool constants and close reason handling
core/src/main/java/io/questdb/cairo/pool/PoolConstants.java		 Adds CR_DROPPED = 6 constant; refactors closeReasonText(int) from traditional switch to switch expression with case for CR_DROPPED returning "DROPPED" and default handling for "UNKNOWN"

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • AbstractMultiTenantPool.notifyDropped: Pay close attention to CAS operations and thread-safety semantics when moving allocations between states during concurrent table drops
  • CairoEngine integration: Verify the order and timing of pool notifications relative to existing MatViewRefreshTask and state-update logic
  • CR_DROPPED constant usage: Confirm the new close reason is properly utilized in the pool cleanup path

Possibly related PRs

Suggested labels

storage

Suggested reviewers

  • bluestreak01

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: faster disk space release when tables are dropped by closing WAL writers faster.
Description check ✅ Passed The description is directly related to the changeset, explaining the motivation and benefit of closing WAL writers faster during table drops.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ideoma
Copy link
Copy Markdown
Collaborator Author

ideoma commented Dec 18, 2025

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 18, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot]
coderabbitai bot reviewed Dec 18, 2025
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a800ac5 and 7cd6f0b.

📒 Files selected for processing (3)
  • core/src/main/java/io/questdb/cairo/CairoEngine.java (1 hunks)
  • core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java (1 hunks)
  • core/src/main/java/io/questdb/cairo/pool/PoolConstants.java (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/io/questdb/cairo/pool/AbstractMultiTenantPool.java (2)
core/src/main/java/io/questdb/std/Unsafe.java (1)
  • Unsafe (40-542)
core/src/main/java/io/questdb/cairo/pool/PoolConstants.java (1)
  • PoolConstants (27-46)
⏰ 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). (36)
  • 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 (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 (Trigger Enterprise CI Trigger Enterprise Pipeline)
  • GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
  • GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
  • GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
  • GitHub Check: New pull request (Hosted Running tests on mac-griffin)
  • GitHub Check: New pull request (SelfHosted Griffin 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-x86-graal)
  • GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
  • 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 (Check Changes Check changes)
  • GitHub Check: build
  • GitHub Check: build
🔇 Additional comments (3)
core/src/main/java/io/questdb/cairo/CairoEngine.java (1)

1197-1198: LGTM! Pool notifications correctly trigger cleanup on table drop.

The calls to notifyDropped() on both pools appropriately notify them to release idle resources when a table is dropped, which aligns with the PR objective of freeing disk space faster.

core/src/main/java/io/questdb/cairo/pool/PoolConstants.java (2)

29-29: New constant CR_DROPPED added for table drop operations.

The addition of CR_DROPPED provides a distinct close reason for dropped tables, improving observability and logging clarity. Ensure this constant is used consistently where tables are dropped (see review comment in AbstractMultiTenantPool.java).

36-44: LGTM! Switch expression refactoring is clean and complete.

The refactoring to a switch expression is idiomatic Java and correctly maps all close reason constants, including the new CR_DROPPED, to their text representations.

bluestreak01
bluestreak01 reviewed Dec 18, 2025
View reviewed changes
@bluestreak01
Copy link
Copy Markdown
Member

bluestreak01 commented Dec 18, 2025

can we have a test also?

@glasstiger
Copy link
Copy Markdown
Contributor

glasstiger commented Dec 27, 2025

[PR Coverage check]

😍 pass : 100 / 113 (88.50%)

file detail

path covered line new line coverage
🔵 io/questdb/cairo/wal/WalWriter.java 0 1 00.00%
🔵 io/questdb/cairo/pool/PoolConstants.java 6 8 75.00%
🔵 io/questdb/cairo/pool/AbstractMultiTenantPool.java 53 62 85.48%
🔵 io/questdb/cairo/CairoEngine.java 12 13 92.31%
🔵 io/questdb/griffin/engine/table/ReaderPoolRecordCursorFactory.java 3 3 100.00%
🔵 io/questdb/cairo/pool/TableReaderMetadataTenantImpl.java 2 2 100.00%
🔵 io/questdb/cairo/DefaultCairoConfiguration.java 1 1 100.00%
🔵 io/questdb/PropertyKey.java 2 2 100.00%
🔵 io/questdb/cairo/TxnScoreboardPoolV2.java 1 1 100.00%
🔵 io/questdb/cairo/pool/WalWriterPool.java 3 3 100.00%
🔵 io/questdb/cairo/pool/SqlCompilerPool.java 3 3 100.00%
🔵 io/questdb/cairo/CairoConfigurationWrapper.java 1 1 100.00%
🔵 io/questdb/cairo/pool/TableMetadataPool.java 1 1 100.00%
🔵 io/questdb/PropServerConfiguration.java 2 2 100.00%
🔵 io/questdb/cairo/pool/SequencerMetadataPool.java 3 3 100.00%
🔵 io/questdb/cairo/pool/ReaderPool.java 7 7 100.00%

@questdb-butler
Copy link
Copy Markdown

questdb-butler commented Dec 27, 2025

⚠️ Enterprise CI Failed

The enterprise test suite failed for this PR.

Build: View Details
Tested Commit: d888de941f67949ff7e559b9360d6bbe128ff22d

Please investigate the failure before merging.

@bluestreak01 bluestreak01 merged commit f3be96a into master Dec 28, 2025
42 of 43 checks passed
@bluestreak01 bluestreak01 deleted the feat-drop-table-faster branch December 28, 2025 22:31
bluestreak01 added a commit that referenced this pull request Dec 30, 2025
@bluestreak01
Revert "feat(core): release disk space faster on table drop (#6555)"
b58d696 
This reverts commit f3be96a.
@bluestreak01 bluestreak01 restored the feat-drop-table-faster branch December 30, 2025 02:31
@ideoma ideoma mentioned this pull request Dec 30, 2025
Merged
@coderabbitai coderabbitai bot mentioned this pull request Jan 18, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@bluestreak01 bluestreak01 bluestreak01 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ideoma @bluestreak01 @glasstiger @questdb-butler