laskoviymishka commented on code in PR #2036:
URL: https://github.com/apache/iceberg-go/pull/2036#discussion_r4087470597
##########
catalog/sql/sql_test.go:
##########
@@ -2774,6 +2775,76 @@ func (s *SqliteCatalogTestSuite)
TestConcurrentTableViewCollisionReturnsCatalogS
}
}
+// TestConcurrentTableViewCollisionUnderLockContention pins the lock-retry
budget in
+// retrySerializableWriteTx. SQLite serializes writers and reports a lock
conflict on
+// upgrade immediately, without consulting the busy handler, so the losing
create only
+// observes the catalog-level collision once the winner commits. Holding the
write lock
+// for longer than the retry budget used to surface SQLITE_BUSY to the caller
instead of
+// ErrTableAlreadyExists/ErrViewAlreadyExists.
+func (s *SqliteCatalogTestSuite)
TestConcurrentTableViewCollisionUnderLockContention() {
+ // Longer than the former 30ms budget, comfortably inside the current
one.
+ const holdWriteLockFor = 100 * time.Millisecond
+
+ ctx := context.Background()
+ sqlDB := s.getDB()
+ _, err := sqlDB.Exec("PRAGMA journal_mode=WAL")
+ s.Require().NoError(err)
+ _, err = sqlDB.Exec("CREATE TABLE IF NOT EXISTS lock_holder(x)")
+ s.Require().NoError(err)
+ s.Require().NoError(sqlDB.Close())
+
+ db := s.getCatalogSqlite()
+ identifier := s.randomTableIdentifier()
+ s.Require().NoError(db.CreateNamespace(ctx,
catalog.NamespaceFromIdent(identifier), nil))
+
+ // Hold the database-wide write lock so both creates begin while
contended.
+ holder := s.getDB()
+ tx, err := holder.Begin()
Review Comment:
Small thing while we're here: the lock-holder tx uses `holder.Begin()` /
`tx.Exec(...)` while every other DB call in the test threads `ctx`.
`holder.BeginTx(ctx, nil)` and `tx.ExecContext(ctx, ...)` keep it consistent
and would respect a deadline if the test ever grows one.
##########
catalog/sql/sql_test.go:
##########
@@ -2774,6 +2775,76 @@ func (s *SqliteCatalogTestSuite)
TestConcurrentTableViewCollisionReturnsCatalogS
}
}
+// TestConcurrentTableViewCollisionUnderLockContention pins the lock-retry
budget in
+// retrySerializableWriteTx. SQLite serializes writers and reports a lock
conflict on
+// upgrade immediately, without consulting the busy handler, so the losing
create only
+// observes the catalog-level collision once the winner commits. Holding the
write lock
+// for longer than the retry budget used to surface SQLITE_BUSY to the caller
instead of
+// ErrTableAlreadyExists/ErrViewAlreadyExists.
+func (s *SqliteCatalogTestSuite)
TestConcurrentTableViewCollisionUnderLockContention() {
+ // Longer than the former 30ms budget, comfortably inside the current
one.
+ const holdWriteLockFor = 100 * time.Millisecond
Review Comment:
Not a blocker, your call whether to touch it here: the retry backoff is
bucketed, so the effective margin is tighter than 310ms minus 100ms. Attempts
fire at 0/10/30/70/150/310ms, and the 100ms hold lands in the [70,150) window,
so the real mutual race happens at attempt 4 (~150ms) and the loser gets one
more cycle to see the collision. That's ~50ms of real slack before the next
bucket, not ~210ms.
If the hold creeps past 150ms on a loaded runner (the exact condition this
targets), both creators hit attempt 4 contended and the loser's first collision
lands on the last attempt, which is the original bug. Dropping
`holdWriteLockFor` well below the boundary (say 50ms), or making
`serializableWriteMaxAttempts`/`serializableWriteRetryDelay` injectable so the
assertion doesn't ride on wall-clock timing, would harden it.
##########
catalog/sql/sql.go:
##########
@@ -308,7 +308,15 @@ func withWriteTx(ctx context.Context, db *bun.DB, fn
func(context.Context, bun.T
}
const (
- serializableWriteMaxAttempts = 3
+ // The retry budget has to outlast a competing writer's transaction,
not just a
+ // scheduling hiccup. SQLite serializes writers and reports a lock
conflict on
+ // upgrade immediately, without consulting the busy handler, so a losing
+ // CreateTable/CreateView only observes the catalog-level conflict it
should
+ // report once the winner commits. At 3 attempts the budget was 30ms,
which a
+ // loaded machine exceeds, surfacing SQLITE_BUSY instead of
+ // ErrTableAlreadyExists/ErrViewAlreadyExists. These bounds give
~310ms, and
+ // cost nothing when there is no contention.
+ serializableWriteMaxAttempts = 6
Review Comment:
The comment frames this in SQLite terms, but the budget gates
`retrySerializableWriteTx` for every dialect, so this ~310ms worst-case now
applies to the Postgres (40001/40P01) and MySQL (1205/1213) retries too. Worth
a line here so a later edit to the delay or count doesn't silently invalidate
the reasoning.
Separately, MSSQL isn't in `isRetryableSerializableError` at all, so none of
this budget helps it: a concurrent create collision on an MSSQL catalog still
leaks the raw driver error on the first attempt. Not for this PR, but the PR
body's dialect-coverage note reads as if MSSQL is covered.
--
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]
