fix(transaction): re-apply table-created updates on commit retry - #8
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes lost table-created updates during commit retries after CAS conflicts.
Changes:
- Registers table-created updates with temporary transactions.
- Uses a non-owning
shared_ptrto support retry reapplication.
File summaries
| File | Description |
|---|---|
src/iceberg/update/pending_update.cc |
Preserves pending updates across commit retries. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+42
to
+43
| ICEBERG_RETURN_UNEXPECTED( | ||
| txn->AddUpdate(std::shared_ptr<PendingUpdate>(this, [](PendingUpdate*) {}))); |
PendingUpdate::Commit's table-created path (Table::NewFastAppend, NewDeleteFiles, NewOverwrite, ...) applied the update to the temporary transaction's metadata builder but never registered it in pending_updates_. When the commit lost the CAS race and the retry runner re-entered CommitOnce, the builder was rebuilt from the refreshed metadata and the re-apply loop iterated an empty list, so the retry posted an UpdateTable request with no changes, guarded only by assert-table-uuid. The catalog accepted it and the commit reported success while the update was silently dropped - an acked append or delete that vanished whenever it raced another writer. Register the update with the transaction before applying it, so a retry re-applies it onto the refreshed base like transaction-created updates. The transaction does not outlive the call, so the non-owning handle is safe. With the update registered, the transaction finalizes it, so the explicit post-commit Finalize calls in the table-created branch are dropped - keeping them would repeat cleanup and file-deletion callbacks. Transaction::Commit's empty-changes early return now finalizes registered updates so that path stays covered.
bharathv
force-pushed
the
fix-retry-drops-pending
branch
from
August 31, 2026 01:36
f09a652 to
4a7cf6e
Compare
Member
|
I am wondering if this has been fixed in upstream ? |
grzebiel
approved these changes
Aug 31, 2026
Author
I don't think it is. Commit wise, the file is up to date compared to source repo. |
mmaslankaprv
approved these changes
Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Any pending update created through the Table::New* factories — NewFastAppend, NewMergeAppend, NewDeleteFiles, NewOverwrite, NewRowDelta, … — is silently dropped when its commit retries after losing the CAS race, and the commit still reports success.
The mechanics:
In other words: whenever two writers race on a table, the loser's operation vanishes and is acknowledged. For an engine using this library for ingestion, that's an acked write lost on every commit conflict — the exact scenario the retry loop exists to handle.
The first attempt behaves correctly (the update was applied to the builder when Apply ran), which is why this survives every single-writer test. It only manifests when a commit genuinely loses a race, i.e. under concurrency.
Fix
Register the update with the temporary transaction (AddUpdate) before applying it, so a retry re-applies it onto the refreshed base exactly like transaction-created updates. The handle is non-owning (shared_ptr with a no-op deleter): the transaction is local to PendingUpdate::Commit and cannot outlive the update, and PendingUpdate does not derive from enable_shared_from_this, so this avoids imposing shared ownership on callers that hold updates by unique_ptr.
Six lines in pending_update.cc; no API change.
How this was found and verified
Found by a deterministic lost-CAS race test in Oxla's integration suite (redpanda-data/oxla#7594): load a table handle, let a concurrent append win a commit, then commit a delete through the stale handle — the first attempt 409s on assert-ref-snapshot-id and the retry must rebase. Against the current code the delete "succeeds" with no second commit request on the wire and the table unchanged; with this fix the retry refreshes, re-applies the pending delete onto the new base, re-runs its validations, and commits correctly. Both truncate- and overwrite-shaped race tests pass against a build carrying this patch.
Worth noting for reviewers: a library-local regression test would be a PendingUpdate::Commit through table ops whose first UpdateTable returns kCommitFailed — asserting that the second attempt's request carries the original changes (today it carries none). Happy to add one if there's an existing mock-catalog harness to hang it on; the Oxla end-to-end tests cover it in the meantime.