richiebachala opened a new pull request, #2542:
URL: https://github.com/apache/iceberg-go/pull/2542
Fixes #2195. Thanks @badalprasadsingh for the design and the
`ReplaceDataFiles` point.
## Why
Since #2046, any commit that removes a deletion vector sets `noReplay`. So a
copy-on-write `Delete` or `Overwrite` of a file with a DV fails on a CAS
conflict even when the concurrent commit is an unrelated append. The guard
protects against a stranded second DV: a peer replaces or adds a DV for a file
we touch, and our stale removal replays as a no-op. That can't happen when the
commit also removes the data file and a validator rejects any concurrent delete
on it.
## What
- `snapshotProducer` gets an opt-in, `markRemovedDVsValidated()`. A DV
removal stays replayable only when the producer is marked **and** every removed
DV's data file is removed in the same commit (`dvRemovalsReplayable`). Any
path-keyed delete-file removal still sets `noReplay`. Unmarked producers behave
exactly as before.
- **Copy-on-write `Delete` / `Overwrite`:** they already register
`rewriteValidator(removed)` over every removed data file, so they now mark the
producer.
- **Merge-on-read `Delete`, whole-file drops:** the delete now registers
`rewriteValidator` over the dropped files that carry a DV, and marks the
producer, but only when the delete drops whole files and nothing else.
- **Why only files with a DV:** `rewriteValidator` treats any concurrent
equality delete as a conflict. Drops of files without a DV remove no DV and
keep replaying as before.
- **Why only whole-file drops:** a partial delete gives a live file a new
DV, and nothing checks that file for a concurrent DV. A replay could then leave
two live DVs on it. So a delete that also partially deletes keeps the no-replay
behavior.
| Path | Removes the DV with its data file | Validator over the removed
files | After this PR |
|---|---|---|---|
| CoW `Delete` / `Overwrite` | yes | `rewriteValidator(removed)` (existing)
| replayable |
| MoR `Delete`, only whole-file drops | yes | `rewriteValidator` over files
with a DV (new) | replayable |
| MoR `Delete` with any partial delete | no, for the partially deleted file
| — | non-replayable (unchanged) |
| `ReplaceDataFiles` / `ReplaceDataFilesByPath` | yes | none |
non-replayable (unchanged) |
| `ReplaceFiles`, dynamic partition overwrite, `RowDelta.RemoveDeletes` |
varies | not registered before the guard | non-replayable (unchanged) |
A correction to my comment on the issue: I listed `ReplaceFiles` as covered.
It only registers `rewriteValidatorWithReferencedDataFiles` with rewrite
semantics plus added deletes, and only after `updater.commit()` has run the
guard, so it's left out here.
## Tests (`table/no_replay_removals_test.go`)
- `TestCoWDeleteRemovingDVFailsInsteadOfReplaying` from #2046 asserted the
old behavior. I renamed it
`TestCoWDeleteRemovingDVReplaysOverConcurrentAppend`. It now asserts that the
delete lands on the replay, that the rows the old DV deleted stay deleted, and
that no DV is left.
- Replays and lands over a concurrent append:
- `TestCoWWholeFileDropWithDVReplaysOverConcurrentAppend`
- `TestCoWOverwriteRemovingDVReplaysOverConcurrentAppend`
- `TestMoRWholeFileDropWithDVReplaysOverConcurrentAppend`
- `TestMoRWholeFileDropRejectsConcurrentDVOnDroppedFile`: a peer DV on the
dropped file fails with `ErrConflictingDeleteFiles` and
`ErrTransactionUnusable`, nothing is committed, and exactly one live DV remains.
- Still non-replayable, after exactly one attempt and with the no-replay
hint:
- `TestMoRDeleteMixingWholeFileDropAndPartialMergeStaysNonReplayable`
- `TestMoRDeleteMixingWholeFileDropAndFirstDVStaysNonReplayable`
- `TestReplaceDataFilesRemovingDVStaysNonReplayable`
- `TestMoRWholeFileDropWithoutDVStillReplaysOverEqualityDelete` pins that
drops without a DV gain no validator.
Against unmodified `main`, the four "replays" tests and the rejection test
fail. The rejection test fails there only because `main` refuses the commit
through the no-replay path. The three non-replayable tests and the
equality-delete test pass on both.
Mutation checks, each restored afterwards:
| Mutation | Failing test |
|---|---|
| Remove the CoW mark | the three CoW tests |
| Mark MoR drops without registering their validator |
`...RejectsConcurrentDVOnDroppedFile` |
| Register the MoR validator whenever any file is dropped |
`...WithoutDVStillReplaysOverEqualityDelete` |
| Allow the MoR mark when the delete also has partial deletes |
`...FirstDVStaysNonReplayable` |
The data-file-also-removed check in `dvRemovalsReplayable` is now redundant
for every caller: MoR never marks a delete with partial deletes, and CoW never
removes a live file's DV. I kept it as a backstop for future callers, but no
test fails without it.
Commands run:
- `go test -race ./table/... ./catalog/internal/... ./catalog/sql/...`
- `go vet ./table/... ./catalog/...` and `go vet -tags integration
./table/...`
- golangci-lint v2.14.0: 0 issues
## Not addressed here
While testing this I found that the same race already corrupts a table on
`main` without any whole-file drop. Two merge-on-read deletes each give one
live file its first DV. The second replays, since it removes nothing, and the
file ends up with two live DVs, so scans fail with `can't index multiple
deletion vectors`. This PR doesn't change that path. I can file it separately.
## Docs
One sentence in `website/src/concurrent-writes.md`.
Generated with [Snowflake
CoCo](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
