Retry DELETE commits on conflict when the re-apply is safe#1189
Open
paultmathew wants to merge 3 commits into
Open
Retry DELETE commits on conflict when the re-apply is safe#1189paultmathew wants to merge 3 commits into
paultmathew wants to merge 3 commits into
Conversation
added 3 commits
July 17, 2026 13:24
A DELETE that lost a commit conflict previously failed outright. It now refreshes and retries against the new tip (the same path INSERT already uses): IcebergAddSnapshot::IsRetryable() now returns true for DELETE. Retrying re-applies the delete against the refreshed manifests, which is only safe if the data it targets still exists. DeleteReapplyUnsafe() inspects every snapshot committed between the delete's scan snapshot and the tip (ranging over sequence numbers, since parent links are not populated on read snapshots): a concurrent pure append is safe, while a commit that removed/rewrote data files or added delete files makes the re-apply unsafe. In the unsafe case StageSingleTableCommit throws a clear "conflicts with a concurrent commit" error instead of silently clobbering. Tests: test_delete_commit_retry (append -> retry+succeed; concurrent delete -> abort); test_transactional_delete updated (resolves its retry FIXME); test_transactional_deletes_not_retried renamed to _retried with the serially-correct outcome. Follow-ups (out of scope): deletion-vector merge for two deletes on the same file, positional-delete re-scan under concurrent compaction, and the write.delete.isolation-level property (duckdb#1162).
Tishj
reviewed
Jul 17, 2026
| int64_t tip_sequence = *tip->sequence_number; | ||
| for (auto &entry : metadata.snapshots) { | ||
| auto &snapshot = entry.second; | ||
| if (!snapshot.sequence_number) { |
Member
There was a problem hiding this comment.
I'm not really sure what we're checking here, I believe sequence_number should always be set for committed data, and we aren't expecting to find uncommitted snapshots here, right?
I think we should assert that it's not missing or throw an InvalidConfigurationException if it's missing
Tishj
reviewed
Jul 17, 2026
| } | ||
| auto base = metadata.GetSnapshotById(base_snapshot_id); | ||
| auto tip = metadata.GetSnapshotById(tip_snapshot_id); | ||
| if (!base || !tip || !base->sequence_number || !tip->sequence_number) { |
Tishj
reviewed
Jul 17, 2026
| } | ||
| auto &metrics = snapshot.metrics.metrics; | ||
| auto removed = metrics.find(IcebergSnapshotMetricType::DELETED_DATA_FILES); | ||
| if (removed != metrics.end() && removed->second > 0) { |
Member
There was a problem hiding this comment.
If we're relying on the presence of the metrics, it being missing should be seen as "might have deleted data files", this makes it behave as "has no deleted data files" instead
Tishj
reviewed
Jul 17, 2026
| return true; | ||
| } | ||
| auto added_deletes = metrics.find(IcebergSnapshotMetricType::ADDED_DELETE_FILES); | ||
| if (added_deletes != metrics.end() && added_deletes->second > 0) { |
Tishj
reviewed
Jul 17, 2026
| //! removed or rewrote data, making it unsafe to re-apply the delete on retry. | ||
| //! Pure appends are safe. Ranges over sequence numbers (parent links aren't | ||
| //! populated on read snapshots); an unreachable base is treated as unsafe. | ||
| static bool DeleteReapplyUnsafe(const IcebergTableMetadata &metadata, int64_t base_snapshot_id, |
Member
There was a problem hiding this comment.
Can we invert this method? It feels weird reading return true; as a negative thing (meaning can't retry)
Tishj
reviewed
Jul 17, 2026
|
|
||
| //! DELETE commit-retry guard: reject a re-apply over a concurrent commit that | ||
| //! removed/rewrote the targeted data (no-op on first attempt / non-deletes). | ||
| if (current_snapshot && current_snapshot->snapshot_id && transaction_data.delete_scan_snapshot_id && |
Member
There was a problem hiding this comment.
Let's move this entire block into a static void VerifyDeleteRetryability(...) method, where we can break up this giant condition with separate if (...) { return; } blocks with the throw at the end for the cases that fall through
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
2 participants
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.
Follow-up to #786 / #1115 (INSERT commit retries) and #1181: extend commit-retry to
DELETE, kept intentionally small per the discussion on #786.What
Today a
DELETEthat loses a commit conflict fails outright. This makes it retry against the refreshed tip the same wayINSERTalready does:IcebergAddSnapshot::IsRetryable()now returns true forDELETE.IcebergCommitState::LoadExistingManifestsalready re-scans the current tip when the cached list is empty), which is only safe if the data the delete targets still exists.DeleteReapplyUnsafe()inspects every snapshot committed between the delete's scan snapshot and the tip — ranging over sequence numbers, sinceparent_snapshot_idis not populated when snapshots are read from the catalog. A concurrent append is safe (retry and succeed); a commit that removed/rewrote data files or added delete files is unsafe, andStageSingleTableCommitthrows a clear conflict error rather than silently clobbering.This addresses the
RETRY_BLOCKERFIXME inConstructManifestListfor the common cases, and the# FIXME: In the future, retries should alleviate this errorintest_transactional_delete.Tests
test_delete_commit_retry(new): concurrent append -> delete retries and succeeds; concurrent delete on the same data -> aborts with a clear error.test_transactional_delete(updated): the racing commit now succeeds via retry (its FIXME is resolved); a concurrently-appended matching row is not removed under the default (that is isolation-level work, see below).test_transactional_deletes_not_retried-> renamedtest_transactional_deletes_retriedwith the serially-correct outcome.Out of scope (follow-ups)
RewriteFiles).write.delete.isolation-level(serializable/snapshot) — Add write isolation levels + semantic conflict detection (write.{merge,update,delete}.isolation-level) #1162 — layering serializable added-data detection on top of this.Notes
Tested against the local
apache/iceberg-rest-fixturecatalog. Happy to add a Lakekeeper-gated concurrency test if preferred (the SQLite-backed fixture can't enforce true serializable commits).