mzzz-zzm commented on issue #830:
URL: https://github.com/apache/iceberg-go/issues/830#issuecomment-4265738135
Great progress on #911 and #912! I've been working on the
"refresh-and-replay" piece
that #912 explicitly defers, and I ran into six bugs that will all fire once
`num-retries` is raised above zero. Documenting them here before opening a
PR, since
#912 describes this as a follow-up.
---
### Context
I tested a `rebase()` implementation against AWS S3 Tables (a real managed
Iceberg REST
catalog backed by S3). The test spins up two goroutines writing to the same
table
concurrently and expects both to eventually succeed via the OCC retry loop.
Six bugs
prevent that from working. Four are OCC retry correctness issues; two are
data races
exposed by concurrent manifest writing.
None of these are visible today with the default `num-retries=0`, but they
will all
surface once that default is raised.
---
### OCC retry bugs (Bugs 1–4)
These four bugs interact in a chain, which is why they are hard to discover
one at a time:
```
Attempt 0 → Writer B loses CAS race → catalog returns 400 (not 409)
↓
Bug 4: 400 not mapped to ErrCommitFailed → retry loop exits immediately
(no retry ever fires)
```
Once Bug 4 is fixed, retries fire but hit the remaining bugs in sequence:
```
Attempt 1 → Bug 3: -1 sentinel in JSON → catalog rejects every retry
unconditionally
Once Bug 3 is fixed:
Attempt 1 → Bug 1: stale lastSequenceNumber → duplicate sequence number →
400
Once Bug 1 is fixed:
Attempt 1 → Bug 2: same snapshotID resubmitted → catalog rejects re-use →
400
Once Bug 2 is fixed: both writers succeed.
```
---
#### Bug 1 — `rebase()` does not sync `lastSequenceNumber`
(`table/snapshot_producers.go`)
`rebase(freshMeta)` advances `parentSnapshotID` and the current-snapshot
pointer in
`MetadataBuilder`, but does **not** update
`MetadataBuilder.lastSequenceNumber`.
The next `commit()` calls `nextSequenceNumber()`, which returns
`*lastSequenceNumber + 1` — still the stale pre-conflict value. The retried
snapshot
gets a sequence number the concurrent winner already used.
S3 Tables validates sequence numbers strictly:
```
bad_request: The specified metadata is not valid
```
**Fix:** copy `freshMeta.LastSequenceNumber()` into
`sp.txn.meta.lastSequenceNumber`
inside `rebase()`.
---
#### Bug 2 — `rebase()` reuses the same `snapshotID` on every retry
(`table/snapshot_producers.go`)
`snapshotID` is assigned once in `createSnapshotProducer()` and never
changed.
After a rejection, `rebase()` + `commit()` resubmit the **same** snapshot-id.
S3 Tables records every attempted snapshot-id — including those from
rejected commits —
and refuses a re-submission:
```
bad_request: The specified metadata is not valid
```
This is a distinct failure path from Bug 1: even with the correct sequence
number, the
id was already seen.
**Fix:** call `sp.txn.meta.newSnapshotID()` at the top of each `rebase()`
call.
---
#### Bug 3 — `commit()` serialises `-1` sentinels into the JSON payload
(`table/snapshot_producers.go`)
`commit()` builds the set-snapshot-ref update as:
```go
NewSetSnapshotRefUpdate(branch, sp.snapshotID, BranchRef, -1, -1, -1)
```
The three trailing args are `MaxRefAgeMs`, `MaxSnapshotAgeMs`,
`MinSnapshotsToKeep`.
`-1` is the internal sentinel for "not set", but the struct serialises them
literally:
```json
"max-ref-age-ms": -1,
"max-snapshot-age-ms": -1,
"min-snapshots-to-keep": -1
```
The struct uses `omitempty`, but `omitempty` on `int64` omits only `0`, not
`-1`.
The initial commit (attempt 0) goes through `MetadataBuilder.SetSnapshotRef`
which
reaches the catalog via a different path that ends up omitting these fields
— so the
bug is **invisible on the first attempt and only appears on retries**.
S3 Tables rejects the explicit negative values:
```
bad_request: The specified metadata is not valid
```
**Fix:** pass `0` instead of `-1` to `NewSetSnapshotRefUpdate()`. The
`omitempty` tags
then correctly omit the fields, matching the behaviour of the initial commit.
> Note: the Java client omits these fields by using `null`; the Go struct
relies on
> `omitempty` + zero value. Passing `-1` breaks that contract.
---
#### Bug 4 — REST catalog does not map HTTP 400 to `ErrCommitFailed`
(`catalog/rest/rest.go`)
`CommitTable`'s error map covers only HTTP 409 → `ErrCommitFailed`. The
retry loop in
`commitWithRetry()` gates on `errors.Is(err, ErrCommitConflict)`.
S3 Tables returns **HTTP 400**, not 409, for all the OCC conflicts above.
Because 400
maps to the generic `ErrBadRequest`, the retry loop treats it as
non-retryable and
exits immediately — meaning none of Bugs 1–3 can even be reached.
**Fix:** add `http.StatusBadRequest → ErrCommitFailed` to the `CommitTable`
error map,
with a comment noting it handles catalogs that return 400 for OCC conflicts
instead of
409. This only affects `CommitTable`; other endpoints use separate error
maps so they
are unaffected.
---
### Data race bugs (Bugs 5–6)
These are independent of the OCC chain but are triggered by the same
concurrent manifest
writes. They show up under `go test -race` on any concurrent-writer workload.
---
#### Bug 5 — Data race in `Schema.MarshalJSON()` (`schema.go`)
`MarshalJSON()` contained:
```go
if s.IdentifierFieldIDs == nil {
s.IdentifierFieldIDs = []int{} // unsynchronised write on shared
*Schema
}
```
During manifest writing, goroutines sharing the same `*Schema` (fetched from
`MetadataBuilder`) call `json.Marshal` concurrently. This write races with
reads in
other goroutines.
**Fix:** work on a local copy instead of touching the receiver:
```go
ids := s.IdentifierFieldIDs
if ids == nil { ids = []int{} }
aliasCopy := *(*Alias)(s)
aliasCopy.IdentifierFieldIDs = ids
return json.Marshal(struct{ ... }{..., Alias: &aliasCopy})
```
---
#### Bug 6 — Data race in `ExpressionEvaluator()` (`visitors.go`)
`ExpressionEvaluator()` returned the **bound method** of a single allocated
instance:
```go
return (&exprEvaluator{bound: bound}).Eval, nil
```
`exprEvaluator.Eval` stores the `StructLike` row into a struct field before
walking:
```go
func (e *exprEvaluator) Eval(st StructLike) (bool, error) {
e.st = st // write on shared instance
return VisitExpr(e.bound, e)
}
```
When conflict detection scans multiple manifests in parallel via `errgroup`,
all
goroutines call the same returned func with different rows, racing on `e.st`.
**Fix:** construct a fresh instance on every call:
```go
return func(st StructLike) (bool, error) {
return VisitExpr(bound, &exprEvaluator{bound: bound, st: st})
}, nil
```
---
### Relationship to PR #912
PR #912 is exactly the right skeleton. The PR description says:
> "The default num-retries is 0 until refresh-and-replay lands in a
follow-up; without
> refreshing requirements between attempts, retrying a contended OCC commit
would fail
> deterministically."
The bugs above are precisely *why* it fails deterministically. The `rebase()`
implementation that provides the refresh-and-replay step:
1. Advances `parentSnapshotID` to the new HEAD so `existingManifests()`
reads the
correct snapshot (equivalent to Java `SnapshotProducer.apply()` being
re-called)
2. Syncs `lastSequenceNumber` from `freshMeta` (Bug 1)
3. Regenerates `snapshotID` (Bug 2)
4. Increments an `attempt` counter so each retry writes a distinct
manifest-list
filename (`snap-<id>-<attempt>-<uuid>.avro`, matching Java behaviour)
5. Is called once per pending producer inside the existing retry loop in
`commitWithRetry()`, before `commit()` is called again
Bugs 3 and 4 are simpler independent fixes that should land regardless of
whether
`rebase()` ships at the same time.
---
### Testing
- **Unit tests** (no AWS credentials needed): 10/10 pass, covering the retry
loop,
conflict detection, orphaned manifest cleanup, and context cancellation.
- **AWS integration tests** (`-tags integration_aws`): 4/4 pass against a
live S3 Tables
endpoint, including a concurrent two-writer scenario where both goroutines
must
eventually commit successfully.
- Verified on latest `main` (rebased onto `add5fce`, post-merge of the
dependency bumps
from the past few days).
Happy to open a PR once there is buy-in. I can split the fixes into separate
commits or
a single PR, whichever is easier to review.
--
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]
