Skip to content

Core: Sweep only the transaction's own snapshots after commit - #18095

Open
raunaqmorarka wants to merge 1 commit into
apache:mainfrom
raunaqmorarka:sweep-own-snapshots
Open

raunaqmorarka wants to merge 1 commit into
apache:mainfrom
raunaqmorarka:sweep-own-snapshots

Conversation

@raunaqmorarka

Copy link
Copy Markdown
Contributor

After a successful commit, BaseTransaction reads the manifest list of every snapshot added since the base captured before the retry loop, to avoid deleting a committed manifest during clean-up. That set includes snapshots other writers committed while the transaction was open. Those snapshots cannot reference files this transaction wrote, because deletedFiles only holds manifests and manifest lists produced by the transaction's own updates. Reading them is wasted work.

On S3 each manifest list read is a HEAD and a GET. With N concurrent inserts each commit reads the manifest lists of all snapshots committed since it opened, so the burst costs O(N^2) reads. Measured: 25 to 30 foreign snapshots per commit at about 35 ms each.

This change takes the starting snapshot set from base after the retry loop. applyUpdates refreshes base on every attempt and the successful commit uses that field, so current minus base is exactly the snapshots this transaction created. A manifest deleted in one attempt and reused by a retry is still protected, because the transaction's own committed snapshot references it.

Tests:

  • a transaction opened before concurrent commits from another table instance, with an injected retry, reads only its own manifest list after commit
  • a manifest merged in the first attempt but kept as is by the retry is not deleted

AI assistance (Claude Code) was used to develop this patch.

Snapshots committed by other writers while the transaction was open
cannot reference files it wrote, so reading their manifest lists during
post-commit clean-up is wasted work that grows with the number of
concurrent commits.
@github-actions github-actions Bot added the core label Sep 14, 2026
@uros-b

uros-b commented Sep 14, 2026

Copy link
Copy Markdown
Member

Nice change, thank you @raunaqmorarka! Also, well tested. Please ping committeres who are familiar with the transaction commit path / snapshot infrastructure (e.g. @nastra or @amogh-jahagirdar)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants