Re: [PR] fix(catalog/sql): return ErrNamespaceAlreadyExists on a concurrent create [iceberg-go]

["zeroshade (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org