zeroshade opened a new issue, #2090:
URL: https://github.com/apache/iceberg-go/issues/2090
**Problem**
`performCopyOnWriteDeletion` (table/transaction.go:2436-2482) removes and
rewrites data files that were planned against the starting snapshot. It never
checks whether those files picked up new deletes from a concurrent commit.
- The only validator on this path is `overwriteFiles.validate`
(table/snapshot_producers.go:443-472). It only checks added data files that
match the filter, and only under `serializable` (line 460).
- `validateNoNewDeletesForRewrittenFiles` already exists
(table/conflict_validation.go:922), but it's only wired into
rewrite_data_files.go:836.
- The merge-on-read path registers `validateDataFilesExist`
(transaction.go:2538). Copy-on-write registers nothing.
With `commit.retry.num-retries > 0`, a CoW commit that loses the race is
refreshed and replayed onto the new head. The replay removes the original file
and adds the rewrite, which was built from the old snapshot. Any delete
committed in between against that file is silently dropped and the deleted rows
come back.
This is the same P0 class as #2042 (deleted rows coming back), reached
through concurrency instead of pre-existing deletes. #2046 fixes the
non-concurrent case only.
With the default `commit.retry.num-retries=0` (table/properties.go:161) the
commit fails with `ErrCommitFailed` instead, so only configurations with
retries enabled are affected.
**Reproduction**
On main (dd935d83), v2 and v3, unpartitioned table with ids 1..5 in a single
data file:
```go
tbl = commitProps(tbl, iceberg.Properties{
table.WriteDeleteModeKey: table.WriteModeCopyOnWrite,
table.CommitNumRetriesKey: "2",
})
// writer A: CoW delete, rewrites the only data file; not committed yet
txA := tbl.NewTransaction()
_ = txA.Delete(ctx, iceberg.EqualTo(iceberg.Reference("id"), int64(2)), nil)
// writer B: merge-on-read delete on the same data file, commits first
txB := tbl.NewTransaction()
_ = txB.SetProperties(iceberg.Properties{table.WriteDeleteModeKey:
table.WriteModeMergeOnRead})
_ = txB.Delete(ctx, iceberg.EqualTo(iceberg.Reference("id"), int64(4)), nil)
_, _ = txB.Commit(ctx) // table reads [1 2 3 5]
out, err := txA.Commit(ctx) // err == nil, replayed onto B's snapshot
```
Observed scan of `out`: `[1 3 4 5]`, with B's delete of id=4 lost. This
happens on both v2 (position delete) and v3 (DV).
Expected: `[1 3 5]`, or A failing with `ErrCommitFailed`. With
`num-retries=0`, A does fail with `branch "main" has changed`.
**Other implementations**
- Java: Spark's CoW commit calls
`overwriteFiles.validateNoConflictingDeletes()` under both SERIALIZABLE and
SNAPSHOT isolation
(`SparkWrite.CopyOnWriteOperation.commitWithSerializableIsolation` /
`commitWithSnapshotIsolation`). `BaseOverwriteFiles.validate` then runs
`validateNoNewDeletesForDataFiles` over the removed data files, plus
`validateNoNewDeleteFiles` for the row filter.
**Proposed fix**
In `performCopyOnWriteDeletion`, register a validator that runs
`validateNoNewDeletesForRewrittenFiles` over every data file the commit removes
or rewrites, whatever the isolation level. That matches Java's
`validateNoConflictingDeletes`. Add a regression test using the repro above
with `commit.retry.num-retries > 0`.
**Related**
- #2042: non-concurrent variant
- #2046: PR fixing #2042
- #830: conflict detection umbrella
--
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]
