zeroshade commented on code in PR #2036:
URL: https://github.com/apache/iceberg-go/pull/2036#discussion_r4159200884
##########
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:
Good catch on the bucket math. Dropped the hold to 50ms: contended through
attempt 3 (70ms), leaving attempts 4 and 5 to see the committed winner, so the
hold has to drift past 150ms before the collision can land on the last attempt.
Comment on the constant spells that out. Still fails 5/5 with the old budget of
3 (SQLITE_BUSY), passes 15/15 under -race with 6.
##########
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:
Added the attempt schedule and a note that the same constants bound the
Postgres/MySQL retries. Agreed MSSQL is not covered by
`isRetryableSerializableError` at all; that is a separate change (error 1205 in
the mssql driver), not for this PR.
##########
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:
Done: `BeginTx(ctx, nil)` / `ExecContext(ctx, ...)`.
--
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]
