zeroshade commented on code in PR #1635:
URL: https://github.com/apache/iceberg-go/pull/1635#discussion_r3926371927
##########
catalog/sql/sql.go:
##########
@@ -1269,9 +1269,46 @@ func (c *Catalog) CreateNamespace(ctx context.Context,
namespace table.Identifie
})
}
- _, err := tx.NewInsert().Model(&toInsert).Exec(ctx)
- if err != nil {
- return fmt.Errorf("error inserting namespace properties
for namespace '%s': %w", namespace, err)
+ // A concurrent writer may have won the race; return the
sentinel in the same
+ // form as the pre-check above. This must stay the first read
on the outer tx
+ // so MySQL's REPEATABLE READ snapshot lands after the winner
committed.
+ recheck := func(insertErr error) error {
+ _, exists, checkErr := c.resolveNamespaceKeyInTx(ctx,
tx, namespace)
+ if checkErr == nil && exists {
+ return fmt.Errorf("%w: %s",
catalog.ErrNamespaceAlreadyExists, strings.Join(namespace, "."))
+ }
+ if checkErr != nil {
+ return fmt.Errorf("error inserting namespace
properties for namespace '%s': %w", namespace, errors.Join(insertErr, checkErr))
+ }
+
+ return fmt.Errorf("error inserting namespace properties
for namespace '%s': %w", namespace, insertErr)
+ }
+
+ // 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.
+ if tx.Dialect().Name() == dialect.PG {
+ sp, err := tx.BeginTx(ctx, nil)
+ if err != nil {
+ return fmt.Errorf("error creating savepoint for
namespace '%s': %w", namespace, err)
+ }
+
+ if _, err = sp.NewInsert().Model(&toInsert).Exec(ctx);
err != nil {
+ if rbErr := sp.Rollback(); rbErr != nil {
+ return fmt.Errorf("error inserting
namespace properties for namespace '%s': %w", namespace, errors.Join(err,
rbErr))
+ }
+
+ return recheck(err)
+ }
+
+ if err = sp.Commit(); err != nil {
+ return fmt.Errorf("error releasing savepoint
for namespace '%s': %w", namespace, err)
Review Comment:
**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.
##########
catalog/sql/sql.go:
##########
@@ -1269,9 +1269,46 @@ func (c *Catalog) CreateNamespace(ctx context.Context,
namespace table.Identifie
})
}
- _, err := tx.NewInsert().Model(&toInsert).Exec(ctx)
- if err != nil {
- return fmt.Errorf("error inserting namespace properties
for namespace '%s': %w", namespace, err)
+ // A concurrent writer may have won the race; return the
sentinel in the same
+ // form as the pre-check above. This must stay the first read
on the outer tx
+ // so MySQL's REPEATABLE READ snapshot lands after the winner
committed.
+ recheck := func(insertErr error) error {
+ _, exists, checkErr := c.resolveNamespaceKeyInTx(ctx,
tx, namespace)
+ if checkErr == nil && exists {
+ return fmt.Errorf("%w: %s",
catalog.ErrNamespaceAlreadyExists, strings.Join(namespace, "."))
+ }
+ if checkErr != nil {
+ return fmt.Errorf("error inserting namespace
properties for namespace '%s': %w", namespace, errors.Join(insertErr, checkErr))
+ }
+
+ return fmt.Errorf("error inserting namespace properties
for namespace '%s': %w", namespace, insertErr)
+ }
+
+ // 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.
+ if tx.Dialect().Name() == dialect.PG {
+ sp, err := tx.BeginTx(ctx, nil)
+ if err != nil {
+ return fmt.Errorf("error creating savepoint for
namespace '%s': %w", namespace, err)
+ }
+
+ if _, err = sp.NewInsert().Model(&toInsert).Exec(ctx);
err != nil {
+ if rbErr := sp.Rollback(); rbErr != nil {
+ return fmt.Errorf("error inserting
namespace properties for namespace '%s': %w", namespace, errors.Join(err,
rbErr))
+ }
+
+ return recheck(err)
+ }
+
+ if err = sp.Commit(); err != nil {
+ return fmt.Errorf("error releasing savepoint
for namespace '%s': %w", namespace, err)
+ }
+
+ return nil
+ }
+
+ if _, err := tx.NewInsert().Model(&toInsert).Exec(ctx); err !=
nil {
+ return recheck(err)
Review Comment:
**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.
<details><summary>Evidence</summary>
```text
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.
```
</details>
##########
catalog/sql/sql.go:
##########
@@ -1269,9 +1269,46 @@ func (c *Catalog) CreateNamespace(ctx context.Context,
namespace table.Identifie
})
}
- _, err := tx.NewInsert().Model(&toInsert).Exec(ctx)
- if err != nil {
- return fmt.Errorf("error inserting namespace properties
for namespace '%s': %w", namespace, err)
+ // A concurrent writer may have won the race; return the
sentinel in the same
+ // form as the pre-check above. This must stay the first read
on the outer tx
+ // so MySQL's REPEATABLE READ snapshot lands after the winner
committed.
+ recheck := func(insertErr error) error {
+ _, exists, checkErr := c.resolveNamespaceKeyInTx(ctx,
tx, namespace)
+ if checkErr == nil && exists {
+ return fmt.Errorf("%w: %s",
catalog.ErrNamespaceAlreadyExists, strings.Join(namespace, "."))
+ }
+ if checkErr != nil {
+ return fmt.Errorf("error inserting namespace
properties for namespace '%s': %w", namespace, errors.Join(insertErr, checkErr))
+ }
+
+ return fmt.Errorf("error inserting namespace properties
for namespace '%s': %w", namespace, insertErr)
+ }
+
+ // 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.
Review Comment:
**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.
--
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]
