zeroshade opened a new pull request, #2036:
URL: https://github.com/apache/iceberg-go/pull/2036
## Rationale for the change
Concurrent `CreateTable`/`CreateView` against a SQLite catalog can fail with
`SQLITE_BUSY` instead of the catalog sentinel (`ErrTableAlreadyExists` /
`ErrViewAlreadyExists`).
This surfaced as a flaky
`TestSqlCatalog/TestConcurrentTableViewCollisionReturnsCatalogSentinel`:
```
Error: Target error should be in err chain:
expected: "view already exists"
in chain: "failed to create table: database is locked (5) (SQLITE_BUSY)"
```
It failed on `main` ([run
35642756763](https://github.com/apache/iceberg-go/actions/runs/35642756763))
and on an unrelated dependency bump ([run
35499928776](https://github.com/apache/iceberg-go/actions/runs/35499928776)),
so it was blocking other PRs.
## Root cause
SQLite permits one writer at a time, and the write transaction in these
paths reads before it writes — `namespaceExistsInTx`, then
`checkIdentifierAvailable`, then the `INSERT`. A lock conflict on that
*upgrade* is returned immediately and **does not consult the busy handler**, so
a `busy_timeout` cannot absorb it. I verified that directly: with
`_pragma=busy_timeout(3000)` the writer still returns `SQLITE_BUSY`, having
waited the full timeout.
Only retrying the whole transaction helps, which `retrySerializableWriteTx`
already does — the budget was simply too small. 3 attempts with 10ms/20ms
backoff is 30ms of waiting, and a loaded runner holds the write lock longer
than that. Once the budget ran out before the winning transaction committed,
the losing caller got the driver's lock error. The post-hoc
`checkIdentifierAvailableInCatalog` fallback also can't recover it, because at
that point the winner still hasn't committed and there is no visible collision
yet.
## Changes
1. `serializableWriteMaxAttempts` 3 -> 6, giving roughly 310ms of retry
budget. This costs nothing when there is no contention, since the delays only
apply between failed attempts.
2. A regression test that makes the contention deterministic by holding the
database write lock for longer than the old budget.
## Are these changes tested?
Yes.
- The new `TestConcurrentTableViewCollisionUnderLockContention` fails
**10/10** before the change with exactly the CI error, and passes **12/12**
after. Reverting only the constant makes it fail again, so it pins the budget
rather than restating the implementation.
- The previously flaky
`TestConcurrentTableViewCollisionReturnsCatalogSentinel` passes 40/40 under
`-race`.
- `go test -race ./catalog/sql/...` is green.
## Are there any user-facing changes?
Yes, and an improvement: a concurrent writer that would previously fail with
a transient `SQLITE_BUSY` now waits for the competing transaction and reports
the real outcome. Worst-case added latency under contention is ~310ms; there is
no change without contention.
Note the same budget also covers serialization failures on other dialects
(Postgres `40001`/`40P01`, MySQL 1205/1213), which benefit from the wider
budget for the same reason.
## AI Disclosure
- Model: Claude Opus 4.8
- Platform/Tool: omp
- Human Oversight: fully reviewed
- Prompt Summary: Diagnose and fix a flaky SQLite catalog test failing on
main with SQLITE_BUSY
--
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]
