This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new c5c427290 fix(catalog/sql): widen the write-retry budget for lock
contention (#2036)
c5c427290 is described below
commit c5c4272905234a73c6d68d7b4e86e6b340013f58
Author: Matt Topol <[email protected]>
AuthorDate: Thu Oct 1 14:59:26 2026 -0400
fix(catalog/sql): widen the write-retry budget for lock contention (#2036)
* fix(catalog/sql): widen the write-retry budget for lock contention
Concurrent CreateTable/CreateView against a SQLite catalog could fail with
SQLITE_BUSY instead of ErrTableAlreadyExists/ErrViewAlreadyExists.
SQLite serializes writers, and the write transaction in these paths reads
(namespace existence, identifier availability) before it inserts. A lock
conflict on that upgrade is reported immediately and does not consult the
busy
handler, so a busy timeout cannot absorb it -- only retrying the whole
transaction can, which retrySerializableWriteTx already does. The budget was
just too small: 3 attempts with 10ms/20ms backoff is 30ms of waiting, and a
loaded machine holds the write lock longer than that. When the budget ran
out
before the winning transaction committed, the losing caller saw the driver's
lock error rather than the catalog conflict it should report.
Raise the budget to 6 attempts (~310ms). It costs nothing without
contention,
since the delays only apply between failed attempts.
This was observed as a flaky
TestConcurrentTableViewCollisionReturnsCatalogSentinel,
which failed on main and on unrelated pull requests.
The new regression test makes the contention deterministic by holding the
database write lock for longer than the old budget; it fails before this
change
and passes after.
Signed-off-by: Matt Topol <[email protected]>
* fix(catalog/sql): tighten contention test margin, document cross-dialect
budget
---------
Signed-off-by: Matt Topol <[email protected]>
---
catalog/sql/sql.go | 15 +++++++++-
catalog/sql/sql_test.go | 74 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/catalog/sql/sql.go b/catalog/sql/sql.go
index 5a29937ab..6ca265b1c 100644
--- a/catalog/sql/sql.go
+++ b/catalog/sql/sql.go
@@ -307,7 +307,20 @@ 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.
+ //
+ // Attempts fire at 0/10/30/70/150/310ms (delay doubles per attempt),
so the
+ // worst case is ~310ms of waiting, and nothing when there is no
contention.
+ // The same budget gates retrySerializableWriteTx for every dialect that
+ // isRetryableSerializableError recognises (Postgres 40001/40P01, MySQL
+ // 1205/1213, ...), so changing either constant changes their worst
case too.
+ serializableWriteMaxAttempts = 6
serializableWriteRetryDelay = 10 * time.Millisecond
)
diff --git a/catalog/sql/sql_test.go b/catalog/sql/sql_test.go
index 58cb293db..c41bba3c8 100644
--- a/catalog/sql/sql_test.go
+++ b/catalog/sql/sql_test.go
@@ -35,6 +35,7 @@ import (
"sync"
"sync/atomic"
"testing"
+ "time"
"github.com/apache/arrow-go/v18/arrow"
"github.com/apache/arrow-go/v18/arrow/array"
@@ -2772,6 +2773,79 @@ 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. Attempts fire at
0/10/30/70/150/310ms,
+ // so a 50ms hold is contended through attempt 3 (70ms) and leaves
attempts 4
+ // and 5 for the loser to observe the committed winner; the hold would
have to
+ // drift past 150ms before the collision could land on the last attempt.
+ const holdWriteLockFor = 50 * 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.BeginTx(ctx, nil)
+ s.Require().NoError(err)
+ _, err = tx.ExecContext(ctx, "INSERT INTO lock_holder VALUES (1)")
+ s.Require().NoError(err)
+
+ released := make(chan struct{})
+ time.AfterFunc(holdWriteLockFor, func() {
+ _ = tx.Rollback()
+ _ = holder.Close()
+ close(released)
+ })
+
+ type createResult struct {
+ objectType string
+ err error
+ }
+ start := make(chan struct{})
+ results := make(chan createResult, 2)
+ go func() {
+ <-start
+ _, err := db.CreateTable(ctx, identifier, tableSchemaNested)
+ results <- createResult{objectType: sqlcat.TableType, err: err}
+ }()
+ go func() {
+ <-start
+ err := db.CreateView(ctx, identifier, tableSchemaNested,
"SELECT 1", nil)
+ results <- createResult{objectType: sqlcat.ViewType, err: err}
+ }()
+ close(start)
+
+ first, second := <-results, <-results
+ <-released
+
+ s.Require().NotEqual(first.err == nil, second.err == nil,
+ "expected exactly one concurrent create to succeed; table/view
results: %v / %v", first.err, second.err)
+ if first.err != nil {
+ first, second = second, first
+ }
+ s.Require().NoError(first.err)
+ if first.objectType == sqlcat.TableType {
+ s.ErrorIs(second.err, catalog.ErrTableAlreadyExists)
+ } else {
+ s.ErrorIs(second.err, catalog.ErrViewAlreadyExists)
+ }
+}
+
func (s *SqliteCatalogTestSuite)
TestCreateRemovesMetadataIfCatalogInsertFails() {
ctx := context.Background()
db := s.getCatalogSqlite()
