iremcaginyurtturk commented on PR #1635: URL: https://github.com/apache/iceberg-go/pull/1635#issuecomment-5191744286
Thanks — you were right that the concurrent test proved nothing about the new branch. Fixed in `358e302`. **Deterministic coverage.** `TestCreateNamespaceLosesInsertRace` only passes if the insert ran and failed: a driver wrapper plants the namespace row on the same connection immediately before the catalog's insert. It has to be a separate statement — sqlite rolls back the failing statement's own changes, and a `BEFORE INSERT` trigger goes with it, which is where I started and why that approach does not work. Verified both ways: with the recovery removed it fails on `Target error should be in err chain`, with it in place it passes. **The re-check moved into the transaction**, `resolveNamespaceKeyInTx(ctx, tx, ...)`. Two reasons, both found writing the test above. On a separate connection it cannot see a winner that has not committed yet, and with a constrained pool it deadlocks outright — the new test sets `SetMaxOpenConns(1)` and the old code hung there rather than failing. Thanks for pushing on this; the original would have been unreliable in exactly the situation it was meant to handle. **Error collection** in the concurrent test now accumulates and asserts after the drain, as suggested — one stray error reports once instead of three times. I kept that test as the smoke test, with a comment saying the deterministic one is the regression test. **Comment** now covers the `checkErr == nil` fall-through. On the follow-up: agreed, and `CreateNamespace` being the only create path with its check outside the transaction is the real fix — this recovers from the race rather than closing it. Happy to open an issue for moving it to `withSerializableWriteTx` alongside `CreateTable` and `CreateView`, unless you would rather fold it into this PR. `go test ./catalog/...` passes, `golangci-lint run catalog/sql/...` reports 0 issues. -- 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]
