fix(catalog/sql): return ErrNamespaceAlreadyExists on a concurrent create

[iremcaginyurtturk]

Problem

CreateNamespace in the sql catalog resolves the namespace key, returns ErrNamespaceAlreadyExists if it is present, and otherwise inserts the properties. Two callers creating the same namespace both pass that check, and the loser's insert violates the primary key on iceberg_namespace_properties. The driver's error is returned unwrapped:

error inserting namespace properties for namespace '[raw]': constraint failed:
UNIQUE constraint failed: iceberg_namespace_properties.catalog_name,
iceberg_namespace_properties.namespace, iceberg_namespace_properties.property_key (1555)

Callers that write errors.Is(err, catalog.ErrNamespaceAlreadyExists) — the contract the other implementations honour, and the natural way to write create-if-absent — see a failure for a namespace that does exist. It surfaces as an intermittent first-run failure in any tool that loads two tables into a new namespace concurrently; that is how I ran into it.

Fix

Re-resolve the namespace key when the insert fails, and return ErrNamespaceAlreadyExists if it is present now. An insert that failed for any other reason is returned exactly as before.

I kept the pre-check rather than replacing it with an upsert: it keeps the common path a single read, and the behaviour on the raced path is then identical to the non-raced one.

[laskoviymishka]

Good catch, and the fix is in the right place. Worth noting this is actually better than both reference implementations: Java's JdbcCatalog.createNamespace calls insertProperties with no handling around the insert, so a race loser there gets an IllegalStateException, and PyIceberg's SqlCatalog.create_namespace surfaces a raw IntegrityError. Returning ErrNamespaceAlreadyExists also matches what the REST catalog already does with a 409, so the two catalogs now agree on the observable contract. Thanks for the repro in the description too, made this easy to follow.

One blocking thing before merge: TestCreateNamespaceConcurrent doesn't pin the new branch. The pre-check at line 1246 runs outside withWriteTx, and it returns ErrNamespaceAlreadyExists with formatting byte-identical to the new path at 1277, so alreadyExists == 7 holds whether a goroutine was gated at the pre-check or actually collided on the insert. Which path each one takes comes down to scheduling. I left a note inline with what I'd add.

Separately, and not for this PR: CreateNamespace is the only create path in this file still on withWriteTx with its existence check outside the transaction. CreateTable and CreateView both do the check inside withSerializableWriteTx, which closes the window structurally instead of recovering from it. Probably worth a follow-up issue.

Once the test covers that branch, happy to take another pass.

[iremcaginyurtturk]

You were right that the concurrent test proved nothing about the new branch. Fixed in 358e302.

TestCreateNamespaceLosesInsertRace only passes if the insert ran and failed — a driver wrapper plants the row on the same connection just before the catalog's insert. It has to be a separate statement: sqlite rolls back the failing statement's own changes, trigger effects included, which is why the trigger version I tried first does not work. With the recovery removed it fails, with it in place it passes.

The re-check also moved into the transaction. On a separate connection it cannot see an uncommitted winner, and with a constrained pool it deadlocks — the new test sets SetMaxOpenConns(1) and the old code hung there.

Error collection and the comment are done as suggested.

On the follow-up: agreed, moving the check inside withSerializableWriteTx closes the window rather than recovering from it. Happy to open a PR for that — as a separate one, or folded in here if you prefer.

[laskoviymishka]

plantingDriver/plantingConn is exactly what I was asking for. Planting the row on the same connection just before the catalog's own insert forces the violation deterministically instead of leaving it to scheduling, and reverting the six lines in sql.go does make TestCreateNamespaceLosesInsertRace fail, so the branch is pinned now. The unexpected []error drain and the checkErr == nil clause in the comment both landed too.

One new blocker this round: the recovery never fires on Postgres. A statement error there puts the whole transaction in aborted state, so the SELECT inside resolveNamespaceKeyInTx on the same tx comes back 25P02 instead of a row, checkErr != nil, and we return the raw insert error, which is what we returned before this PR. postgres is one of the five dialects this file declares, so it isn't hypothetical. SQLite rolls back only the offending statement, which is the only reason the new test is green.

Which means walking back something I said last round. I called CreateNamespace keeping its existence check outside the transaction a follow-up, "not for this PR." It turns out to be load-bearing for the fix, so I'd rather deal with it here. One caveat on the obvious version of that: moving the check inside a withSerializableWriteTx like CreateTable and DropNamespace do narrows the window a lot, but Postgres doesn't cover unique-index enforcement under SSI, so a loser can still surface 23505 rather than a serialization failure. The two shapes that hold across dialects are a SAVEPOINT around the insert so the re-check has a live transaction to run in, or mapping the driver's unique-violation code straight to the sentinel and dropping the re-query.

If you'd rather keep the scope tight I won't fight it, but then the comment and the test should say SQLite-only out loud, so a green test doesn't get read as multi-dialect coverage. Either way, happy to take another pass once it's settled.

[laskoviymishka]

This guard can't fire on Postgres. A statement error there marks the whole transaction aborted and every command after it gets 25P02 until a rollback, so once the insert fails with a 23505 unique violation the SELECT that resolveNamespaceKeyInTx runs on this same tx comes back checkErr != nil rather than exists. We fall through and return the raw insert error, which is exactly what we returned before this PR. postgres is one of the five dialects this file declares, so it's a supported target.

SQLite rolls back only the offending statement, which is why the new test is green. MySQL and Oracle behave like SQLite here; Postgres is the outlier, and it's the one most deployments run against.

Two shapes hold across dialects: a SAVEPOINT around the insert so the re-check has a live transaction to run in, or matching the driver's unique-violation code directly and dropping the re-query entirely. Moving the existence check inside a withSerializableWriteTx, the way CreateTable and DropNamespace do, is worth doing regardless, but on Postgres unique-index enforcement isn't covered by SSI predicate locking, so a loser can still come back with 23505 instead of a serialization failure. It narrows the window rather than replacing this block. wdyt?

[laskoviymishka]

This comment has the mechanism slightly off, and the wrong version is what hides the dialect gap. The plant isn't "its own statement" in any transactional sense; it's in the same transaction as the catalog's insert and never separately committed. What actually saves it is that SQLite rolls back only the failing statement, so the plant row survives to be seen by the re-check.

I'd say that part out loud, because on Postgres the same sequence loses both and the re-check can't run at all (see sql.go:1276). Something like "SQLite rolls back only the failing statement, so the plant stays visible to the re-check; other dialects differ" keeps the next reader from taking this green test as multi-dialect coverage.

[laskoviymishka]

non-blocking: these two panic rather than fail if the shim ever stops implementing the interface, and a panic in a driver callback takes the whole test binary down with a trace that won't point back here. ExecContext and QueryContext just above already do the ok-check, so it's mostly consistency:

beginTx, ok := c.Conn.(driver.ConnBeginTx)
if !ok {
	return nil, driver.ErrBadConn
}

return beginTx.BeginTx(ctx, opts)

Same for PrepareContext below it.

[zeroshade]

Thanks for this — the shape of the fix is the right one. Re-checking existence after a failed insert avoids driver-specific error-code sniffing entirely, which is the trap most implementations of this fall into: no pq.Error code table, no sqlite3.ErrConstraintUnique type switch, nothing that breaks when someone swaps drivers. And the sentinel wrapping is exactly right — fmt.Errorf("%w: %s", catalog.ErrNamespaceAlreadyExists, …) matches the pre-check path at sql.go:1252 byte for byte, so errors.Is behaves identically whether the caller lost the race or not.

TestCreateNamespaceLosesInsertRace is also a genuinely good test. The planting driver proves the recovery branch actually executed, which a plain concurrency test can never do — it can only show the right error came out, not which code path produced it.

One blocking issue: as written, the fix is silently inert on PostgreSQL. Details inline on sql.go:1276.

Test coverage

Beyond the planting-driver test, two gaps:

1. No Postgres or MySQL coverage. The entire portability question — which is where the blocking issue lives — is untested, and there's no pg driver in go.mod for CI to use. Even a comment on the test naming which dialects are actually exercised would help the next person, but given that the bug is dialect-specific, this is where a build-tagged integration test would earn its keep.
2. No negative case. Nothing proves that an insert failure for an unrelated reason (constraint violation on some other column, connection error, serialization failure) still surfaces the original error unchanged rather than being converted into a misleading ErrNamespaceAlreadyExists. The planting driver you already built makes this cheap to add — plant a different failure and assert the original error survives.

[zeroshade]

Blocking — this branch cannot fire on PostgreSQL, so the bug is unfixed for that dialect.

The re-check runs on tx, the same transaction whose INSERT just failed on line 1272. PostgreSQL aborts a transaction on any statement error (SQLSTATE 25P02); every subsequent command in that transaction fails with current transaction is aborted, commands ignored until end of transaction block.

So on Postgres checkErr != nil always, the && exists branch is never taken, and control falls through to line 1280 — returning the raw duplicate-key error, which is precisely the behavior this PR sets out to fix. Postgres is a first-class dialect here (sql.go:58, sql.go:243), not an exotic one.

MySQL and SQLite don't abort the transaction on a duplicate-key error, so the fix works as intended there. Net effect: correct for two of three dialects, quietly inert on the third, and nothing in the test suite would catch it.

Suggested fix, either approach:

• Wrap the insert in a savepoint — tx.Exec("SAVEPOINT ns_insert") before, ROLLBACK TO SAVEPOINT ns_insert on failure — which restores the transaction to a usable state on Postgres and is a harmless no-op on the others.
• Or do the re-check outside the failed transaction, on c.db via resolveNamespaceKey rather than resolveNamespaceKeyInTx. A fresh connection isn't poisoned by the aborted transaction. Slightly weaker isolation for the read, but for an "does it exist now" check after a failure that is fine.

The savepoint version is the more faithful of the two, since it keeps the read inside the same transactional context.

[zeroshade]

Minor: when the re-check succeeds and reports the namespace exists, the original insert error is discarded. That's defensible — the observable state is "already exists," and the caller gets the sentinel they need — but it does mean an insert that failed for some unrelated reason, on a namespace that happens to exist, reports a cause that isn't the real one.

Suggested fix: consider errors.Join-ing the driver error into the returned error on the recovery branch above, so errors.Is(err, catalog.ErrNamespaceAlreadyExists) still works for callers while operators keep the underlying cause in logs. Not blocking either way.

[zeroshade]

sql.Register installs a driver name into a process-global registry and panics with Register called twice for driver ... on a duplicate. This is safe here only because drvName derives from databaseName() (sql_test.go:152), which is unique per call.

That's a load-bearing property of a helper defined ~1900 lines away, and the failure mode is a panic that takes the whole test binary down rather than a single failing test.

Suggested fix: a one-line comment noting the dependency on databaseName() uniqueness would keep someone from later switching to a fixed driver name for readability.

[laskoviymishka]

The SAVEPOINT route is exactly one of the two shapes I floated last round, and it does resolve the Postgres blocker — the insert rolls back to the savepoint and the re-check runs on a live transaction instead of coming back 25P02, so the recovery actually fires on pg and not just SQLite. The failUnrelated negative case landed too, and the pgAbortDriver emulation is a nice way to prove the aborted-transaction path without a real pg in CI. This is close.

One thing I'd sort before merge: the race-loser return isn't quite the same shape as the serial pre-check. The pre-check returns a plain fmt.Errorf("%w: %s", …), and this path wraps that in an errors.Join with the raw insert error, so .Error() picks up an extra line and errors.As unwraps to the driver error only when you lost the race. errors.Is holds either way, but I don't think it's byte-identical to the pre-check the way the thread reads it — dropping the insert error from the join gets them back in sync. Details inline.

The rest is small:

• the rollback-failure and re-check-failure branches both drop their diagnostic (rbErr, checkErr) — I'd join them in so a poisoned tx doesn't get reported as a plain insert error.
• worth confirming the failUnrelated emulation actually fails without the recovery (the boolRows EOF thing inline), so the negative case is genuinely pinned.
• one sanity check that bun emits standard savepoint SQL for the other declared dialects — mssql uses SAVE TRANSACTION, so it's worth a look before we call this multi-dialect.

The broader "pre-check lives outside the tx" restructure I went back and forth on last round — I think the savepoint settles the load-bearing part of it, so I'm happy to leave the full move to a follow-up issue rather than hold this on it.

Fix the error shape and I'm happy to approve.

[laskoviymishka]

The race-loser here returns errors.Join(sentinel, err), but the pre-check path a few lines up returns a plain fmt.Errorf("%w: %s", catalog.ErrNamespaceAlreadyExists, …). errors.Is holds either way so the 409 mapping is safe, but .Error() now carries the raw insert error on a second line, and errors.As will unwrap to the driver's constraint error only on the race path — so two paths that mean the same thing produce structurally different errors depending on scheduling.

I'd drop err from the join and return the same form as the pre-check so they're identical. If we want the cause for logs, %v keeps it in the string without putting it in the unwrap chain.

[laskoviymishka]

Two failure branches in this block drop their cause. On rollback failure we return the insert err and throw away rbErr — but a failed ROLLBACK TO SAVEPOINT is a different and worse condition than a failed insert, and on Postgres it's exactly what leaves the tx poisoned, so that's the diagnostic I'd most want to keep. Same shape at the re-check just below: when checkErr != nil we fall straight through to the insert-error return and lose that the re-check itself failed.

I'd errors.Join(err, rbErr) on the rollback path and thread checkErr into the fall-through return so neither cause vanishes. wdyt?

[laskoviymishka]

Next returns a row (with int64(0)) when val is false rather than signaling EOF, so this only reads as "not exists" if bun's existence check scans the column value rather than row presence. If it ever keys on presence, the failUnrelated case would see a row, conclude the namespace exists, and hand back ErrNamespaceAlreadyExists — the exact thing that test asserts against — so it'd fail opaquely instead of catching a regression.

Simplest way to make it robust either way is to return io.EOF when !val so "not exists" is genuinely an empty result:

func (r *boolRows) Next(dest []driver.Value) error {
	if r.done || !r.val {
		return io.EOF
	}
	r.done = true
	dest[0] = int64(1)

	return nil
}

Worth a quick check that reverting the recovery in sql.go still fails TestCreateNamespaceInsertFailureSurfacesOriginalError — if it does, the negative case is genuinely pinned.

[laskoviymishka]

Small robustness thing on the emulation: both isRollbackToSavepoint and isNamespaceExistsProbe match bun's exact SQL text (the ROLLBACK TO SAVEPOINT prefix, the literal EXISTS). Postgres and SQLite also accept ROLLBACK TO <name> without the keyword, and if a bun version ever changes either shape the aborted flag never clears or the probe stops firing, and the test falls through to an opaque ErrorIs failure instead of exercising the recovery.

Not blocking, but a one-line comment naming the bun format these depend on — or asserting the savepoint query was actually seen — would save the next person the debugging. The driver-scoped aborted/insertAttempted state is fine given SetMaxOpenConns(1) and you've already documented that, so I'd leave it.

[iremcaginyurtturk]

Thanks for the detailed pass. Pushed the fixes:

• Error shape: the race-loser now returns the same fmt.Errorf("%w: %s", …) as the serial pre-check, so the two paths are byte-identical and errors.As no longer unwraps to the driver error only on the race path.
• Dropped causes: the rollback-failure branch now errors.Join(err, rbErr) and the re-check-failure branch errors.Join(err, checkErr), so a poisoned tx / failed re-check isn't reported as a plain insert error.
• mssql: confirmed bun emits SAVE TRANSACTION for the MSSavepoint feature, so the savepoint path holds across the declared dialects.
• Emulation robustness: added a comment naming the bun SQL shape the matchers depend on.

On the boolRows EOF suggestion — I went a different way, and wanted to flag why: bun's Exists() runs SELECT EXISTS(...) and scans the value via QueryRowContext(...).Scan(&bool), so an empty result comes back as sql.ErrNoRows rather than "false" and the probe errors out (I hit this — the tests fail). So I kept boolRows emitting an explicit 0/1 row and documented that it must, rather than signalling EOF. I did confirm the pinning holds: reverting the savepoint makes TestCreateNamespaceLosesInsertRace fail, and the negative case would fail if the re-check ever reported "exists" for the unrelated-failure path — so both are genuinely pinned. Happy to revisit if you'd prefer a different shape there.

[zeroshade]

The savepoint shape resolves the Postgres aborted-transaction blocker cleanly — the insert failure rolls back to the savepoint and the re-check runs on a live transaction, with the pgAbortDriver emulation pinning exactly that dialect behavior without needing pg in CI. The race-loser now returns the sentinel byte-for-byte the same as the pre-check path, and the previously-dropped rollback/re-check causes are preserved via errors.Join. Driver-independent recovery (no error-code sniffing) remains the strongest property of this fix. Nice work.

Withdrawing my earlier suggestion to join the insert error onto the exists-path sentinel — path consistency argues the other way and the comment documents the choice.

Deferring final confirmation of laskoviymishka's threads to him since his change request predates this revision.

---

This review was drafted with an AI-assisted tool and may contain mistakes; an Apache Iceberg Go maintainer has reviewed and confirmed the submission. See the contributing docs for what the project considers a maintainer review.

[laskoviymishka]

Everything from round 3 landed. The race-loser return is a plain fmt.Errorf("%w: %s", catalog.ErrNamespaceAlreadyExists, …) now, matching the pre-check byte-for-byte, so the two paths finally agree and errors.As unwraps the same on both. I see zeroshade withdrew the join-the-insert-error suggestion, so we're settled there. The rollback and re-check branches errors.Join their diagnostics now, failUnrelated pins the negative case, and the mssql SAVE TRANSACTION question got looked at. zeroshade's already approved and deferred my threads back to me, so consider those cleared.

One new thing before merge, and it's the other half of the multi-dialect savepoint worry from last round. I checked mssql's SAVE TRANSACTION on the create side, but the commit side has its own dialect gap: sp.Commit() on a savepoint-backed tx emits RELEASE SAVEPOINT, and Oracle has no such statement. It supports SAVEPOINT and ROLLBACK TO SAVEPOINT but releases savepoints implicitly. bun only skips the RELEASE for the mssql feature flag as far as I can tell, so on Oracle the commit should fail and roll the insert back.

Oracle's a declared dialect and it worked before this PR (plain Exec, no savepoint), so this would be a regression on the happy path, not a pre-existing gap. I've left the details inline. Worth confirming against bun's oracle dialect before we merge, and if it holds up, I'd gate the savepoint to Postgres, which is the only dialect that actually needs it, rather than take it on every path.

The rest is small and inline: an insertAttempted assertion so a missed intercept fails loudly instead of as a confusing error-shape mismatch, and a comment pinning the MySQL re-check as the first consistent read. Neither blocks.

Sort the Oracle path and I'm happy to approve.

[laskoviymishka]

While we're here: this re-check being correct on MySQL leans on it being the first consistent read in the outer tx. Under REPEATABLE READ the snapshot is taken at the first non-locking SELECT, and since the insert is the only statement before it, the snapshot lands after the race winner committed and exists comes back true. Safe today.

It's a quiet invariant though. If anyone later adds an existence SELECT before the insert inside withWriteTx, the snapshot would establish too early and the re-check could miss the winner. A one-line comment that this must stay the first read on the outer tx would keep it from breaking silently. Not a blocker.

[laskoviymishka]

This is the other leg of the multi-dialect savepoint worry from last round. I checked mssql's SAVE TRANSACTION on the create side, but the commit side has its own gap: sp.Commit() on a savepoint-backed tx emits RELEASE SAVEPOINT, and Oracle has no such statement. It supports SAVEPOINT and ROLLBACK TO SAVEPOINT but frees savepoints implicitly. bun only skips the RELEASE for the mssql feature flag as far as I can tell.

If that's right, this regresses Oracle on the happy path: CreateNamespace was a plain Exec before this PR with no savepoint, so the commit would now fail and the RunInTx defer rolls back the insert that just succeeded. Oracle's a declared dialect, so it's not hypothetical.

Worth confirming against bun's oracle dialect before merge. If it doesn't emit a no-op RELEASE, I'd gate the savepoint to Postgres, the only dialect that actually needs the aborted-transaction recovery, rather than take it on every path. wdyt?

[laskoviymishka]

One robustness gap in the emulation: if the wrapped conn ever doesn't implement ExecerContext (same for QueryerContext just below), this returns driver.ErrSkip, and database/sql falls back to prepare-then-exec, which skips the interceptor entirely. The injected failure never fires, the test sees a clean CreateNamespace, and it fails on a confusing nil-vs-sentinel mismatch instead of "the simulation didn't run." sqliteshim implements both today so it's not live, but nothing pins it.

Asserting drv.insertAttempted.Load() after the call in both race tests would catch that, and doubles as proof the savepoint path actually ran rather than getting skipped.

[zeroshade]

All prior blockers (Postgres 25P02, Oracle RELEASE SAVEPOINT, error shape, dropped diagnostics, negative case) are genuinely fixed and mutation-verified, but the Postgres gate left the non-Postgres recovery branch and the Postgres happy path at coverage count 0.

Re-review verification: 11 of 13 prior findings confirmed fixed at 25fd793 (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust). Still open:

• partially fixed — laskoviymishka R1: TestCreateNamespaceConcurrent cannot tell whether the new branch ran; needs a deterministic pin

Verification performed

go build ./... => OK; go vet ./catalog/sql/ ./catalog/hadoop/ => OK; go test ./catalog/sql/ -count=1 => ok (3 consecutive runs); go test ./catalog/sql/ -count=1 -race => ok (3.612s); TestCreateNamespaceConcurrent / LosesInsertRace / InsertFailureSurfacesOriginalError all PASS; unmutated concurrency test 0/25 spurious failures; coverage run 79.5% stmts; 4 mutation probes (savepoint removal, non-PG recovery removal, '&& exists' removal, exists-probe forced true) each produced the expected pass/fail; 1 throwaway probe file written and deleted, worktree ends with zero modifications.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The findings below are observations, not blockers; an Apache Iceberg Go maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.

More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.

[zeroshade]

major — Non-Postgres recovery branch has no deterministic test; the PR's own repro dialect is unpinned

The Postgres gate added in 25fd793 moved the deterministic driver test entirely onto the PG branch, so the SQLite/MySQL/Oracle/MSSQL dispatch at 1310-1312 is covered only by the scheduling-dependent TestCreateNamespaceConcurrent. This re-opens laskoviymishka's round-1 blocker for exactly the dialect in the PR description's repro (the quoted error is SQLite's 1555). Add a SQLite-dialect deterministic test using a NON-aborting driver. Note a dialect parameter on newAbortCatalog will NOT work: pgAbortDriver unconditionally sets aborted on the insert and only ROLLBACK TO SAVEPOINT clears it, so a SQLite-dialect run never clears it and the re-check fails with errEmulatedAborted. It needs a separate ~40-line driver reusing isNamespaceInsert/isNamespaceExistsProbe/boolRows.

Evidence

go tool cover: 'catalog/sql/sql.go:1311.4,1312.1 1 0' (count 0). Reverting 1310-1312 to the pre-PR raw error leaves 'go test ./catalog/sql/ -count=1' => ok. Detection rate measured over 15 independent single runs: 13/15. Throwaway probe with a non-aborting dupDriver at SQLite dialect: unmodified head => 'race-loser err = namespace already exists: my-iceberg-db-mpifixpfynaunvszosha' PASS; reverted mutant => 'error inserting namespace properties ...: UNIQUE constraint failed' FAIL.

[zeroshade]

minor — Postgres happy path (SAVEPOINT/RELEASE SAVEPOINT) is never executed by any test

Both pgAbortDriver tests force the insert to fail, so sp.Commit() at 1303 and the 'return nil' at 1307 never run. This is newly added code on the hot path of every successful CreateNamespace against Postgres, and there is no real Postgres anywhere in CI (grep over .github/workflows and dev/ finds none). A success-path assertion through the existing pg-dialect-over-sqlite harness would cover it cheaply.

[zeroshade]

nit — Gate comment conflates two independent justifications

'Only Postgres aborts the whole tx on a failed insert, so only it needs a savepoint to keep the re-check runnable; Oracle has no RELEASE SAVEPOINT.' These are two separate facts: the first justifies why PG needs the savepoint, the second why it must not be applied universally. As written, Oracle reads like the reason for a Postgres-only gate. Also, the 'only Postgres' claim is asserted for MSSQL without evidence (SQL Server's behavior depends on XACT_ABORT); scoping the sentence to the dialects actually reasoned about would be more honest.