Skip to content

Commit 0e2e48d

Browse files
authored
Merge pull request #11125 from dolthub/aaron/nbs-try-lock-on-read-only-optimization
go/{store/nbs,dbfactory,env}: Add functionality to fast-fail fslock, instead of serially waiting 100ms per DB load, when we already have some ReadOnly databases in the MultiRepoEnv.
2 parents 1f713b5 + f643a80 commit 0e2e48d

8 files changed

Lines changed: 240 additions & 37 deletions

File tree

go/cmd/dolt/dolt.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -753,11 +753,22 @@ If you're interested in running this command against a remote host, hit us up on
753753
// repositories in our MultiEnv are ReadOnly. This includes the
754754
// case where there are no repositories in our MultiEnv
755755
var allReposAreReadOnly bool = true
756+
var anyIsReadOnly bool = false
756757
err = mrEnv.Iter(func(name string, dEnv *env.DoltEnv) (stop bool, err error) {
758+
if anyIsReadOnly {
759+
if dEnv.DBLoadParams == nil {
760+
dEnv.DBLoadParams = map[string]any{dbfactory.SkipJournalLockTimeoutParam: true}
761+
} else {
762+
dEnv.DBLoadParams[dbfactory.SkipJournalLockTimeoutParam] = true
763+
}
764+
}
757765
readOnly, err := dEnv.IsAccessModeReadOnly(ctx)
758766
if err != nil {
759767
return true, fmt.Errorf("Failed to load database %s due to error: %w", name, err)
760768
}
769+
if readOnly {
770+
anyIsReadOnly = true
771+
}
761772

762773
allReposAreReadOnly = allReposAreReadOnly && readOnly
763774

go/libraries/doltcore/dbfactory/file.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,11 @@ const (
7171
//
7272
// Intended for embedded-driver usage so higher layers can implement their own retry/backoff policy.
7373
FailOnJournalLockTimeoutParam = "fail_on_journal_lock_timeout"
74+
75+
// Immediatly proceed with opening the database or failing the open (based on FailOnJournalLockTimeoutParam) as
76+
// soon as a non-blocking fslock call indicates that the LOCK is unavailable. Do not spend a short timeout
77+
// waiting for it to become available.
78+
SkipJournalLockTimeoutParam = "skip_journal_lock_timeout"
7479
)
7580

7681
// DoltDataDir is the directory where noms files will be stored
@@ -209,6 +214,9 @@ func (fact FileFactory) CreateDbNoCache(ctx context.Context, nbf *types.NomsBinF
209214
if _, ok := params[FailOnJournalLockTimeoutParam]; ok {
210215
opts.FailOnLockTimeout = true
211216
}
217+
if _, ok := params[SkipJournalLockTimeoutParam]; ok {
218+
opts.SkipLockFileTimeout = true
219+
}
212220
}
213221
newGenSt, err = nbs.NewLocalJournalingStoreWithOptions(ctx, nbf.VersionString(), path, q, mmapArchiveIndexes, recCb, opts)
214222
} else {

go/libraries/doltcore/env/multi_repo_env.go

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,13 @@ func multiEnvForConfigDirectoryEnv(ctx context.Context, config config.ReadWriteC
197197
logrus.Warnf("failed to load database with error: %s", dbErr.Error())
198198
}
199199
}
200+
if skipLockTimeout, err := dEnv.IsAccessModeReadOnly(ctx); err == nil && skipLockTimeout {
201+
if dbLoadParams == nil {
202+
dbLoadParams = map[string]any{dbfactory.SkipJournalLockTimeoutParam: true}
203+
} else {
204+
dbLoadParams[dbfactory.SkipJournalLockTimeoutParam] = true
205+
}
206+
}
200207
envSet[dbName] = dEnv
201208
openedEnvs = append(openedEnvs, dEnv)
202209
}
@@ -266,9 +273,16 @@ func multiEnvForConfigDirectoryEnv(ctx context.Context, config config.ReadWriteC
266273
}
267274

268275
func (mrEnv *MultiRepoEnv) ReloadDBs(ctx context.Context) {
276+
anyIsReadOnly := false
269277
for _, namedEnv := range mrEnv.envs {
270278
dEnv := namedEnv.env
271-
279+
if anyIsReadOnly {
280+
if dEnv.DBLoadParams == nil {
281+
dEnv.DBLoadParams = map[string]any{dbfactory.SkipJournalLockTimeoutParam: true}
282+
} else {
283+
dEnv.DBLoadParams[dbfactory.SkipJournalLockTimeoutParam] = true
284+
}
285+
}
272286
if dEnv.doltDB == nil {
273287
LoadDoltDB(ctx, dEnv)
274288
}
@@ -286,6 +300,10 @@ func (mrEnv *MultiRepoEnv) ReloadDBs(ctx context.Context) {
286300
if cfgErr != nil {
287301
logrus.Warnf("failed to load database configuration at %s with error: %s", dEnv.urlStr, cfgErr.Error())
288302
}
303+
} else if !anyIsReadOnly {
304+
if isReadOnly, err := dEnv.IsAccessModeReadOnly(ctx); err == nil && isReadOnly {
305+
anyIsReadOnly = true
306+
}
289307
}
290308
}
291309
}

go/store/nbs/journal.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -608,11 +608,19 @@ func (c journalConjoiner) chooseConjoinees(upstream []tableSpec) (conjoinees []t
608608
return c.child.chooseConjoinees(pruned)
609609
}
610610

611-
func newJournalLock(dir string, failOnTimeout bool) (*fslock.Lock, chunks.ExclusiveAccessMode, error) {
611+
func newJournalLock(dir string, timeout time.Duration, failOnTimeout bool) (*fslock.Lock, chunks.ExclusiveAccessMode, error) {
612612
lock := fslock.New(filepath.Join(dir, lockFileName))
613613
// try to take the file lock. if we fail, make the manifest read-only.
614614
// if we succeed, hold the file lock until we close the journalManifest
615-
err := lock.LockWithTimeout(lockFileTimeout)
615+
var err error
616+
if timeout == 0 {
617+
err = lock.TryLock()
618+
if errors.Is(err, fslock.ErrLocked) {
619+
err = fslock.ErrTimeout
620+
}
621+
} else {
622+
err = lock.LockWithTimeout(timeout)
623+
}
616624
if errors.Is(err, fslock.ErrTimeout) {
617625
if failOnTimeout {
618626
return nil, chunks.ExclusiveAccessMode_ReadOnly, ErrDatabaseLocked

go/store/nbs/journal_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ func makeTestChunkJournal(t *testing.T) *ChunkJournal {
3737
dir, err := os.MkdirTemp("", "")
3838
require.NoError(t, err)
3939
t.Cleanup(func() { file.RemoveAll(dir) })
40-
l, _, err := newJournalLock(dir, false)
40+
l, _, err := newJournalLock(dir, lockFileTimeout, false)
4141
require.NoError(t, err)
4242
m, err := newJournalManifest(ctx, dir, l)
4343
require.NoError(t, err)
@@ -54,7 +54,7 @@ func makeTestChunkJournal(t *testing.T) *ChunkJournal {
5454
}
5555

5656
func openTestChunkJournal(t *testing.T, dir string) *ChunkJournal {
57-
l, _, err := newJournalLock(dir, false)
57+
l, _, err := newJournalLock(dir, lockFileTimeout, false)
5858
require.NoError(t, err)
5959
m, err := newJournalManifest(t.Context(), dir, l)
6060
require.NoError(t, err)
@@ -111,7 +111,7 @@ func TestChunkJournalReadOnly(t *testing.T) {
111111
rw := makeTestChunkJournal(t)
112112
assert.Equal(t, chunks.ExclusiveAccessMode(chunks.ExclusiveAccessMode_Exclusive), rw.AccessMode())
113113

114-
_, _, err := newJournalLock(rw.backing.dir, true)
114+
_, _, err := newJournalLock(rw.backing.dir, lockFileTimeout, true)
115115
require.ErrorIs(t, err, ErrDatabaseLocked)
116116
})
117117
}

go/store/nbs/store.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -791,14 +791,24 @@ type JournalingStoreOptions struct {
791791
// FailOnLockTimeout returns an error if the exclusive journal manifest lock cannot be acquired
792792
// within Dolt's internal lock timeout, instead of falling back to opening in read-only mode.
793793
FailOnLockTimeout bool
794+
795+
// If true, instead of waiting a short time to try to acquire the lock, proceed immediately
796+
// as soon as the lock acquire has failed. This is useful to Dolt if some databases have
797+
// already been loaded in ExclusiveAccessMode_ReadOnly and there is no real reason to wait
798+
// around trying to get Exclusive mode if you fail on the first non-blocking flock call.
799+
SkipLockFileTimeout bool
794800
}
795801

796802
func NewLocalJournalingStoreWithOptions(ctx context.Context, nbfVers, dir string, q MemoryQuotaProvider, mmapArchiveIndexes bool, warningsCb func(error), opts JournalingStoreOptions) (*NomsBlockStore, error) {
797803
if err := checkDir(dir); err != nil {
798804
return nil, err
799805
}
800806

801-
lock, staticAccessMode, err := newJournalLock(dir, opts.FailOnLockTimeout)
807+
timeout := lockFileTimeout
808+
if opts.SkipLockFileTimeout {
809+
timeout = 0
810+
}
811+
lock, staticAccessMode, err := newJournalLock(dir, timeout, opts.FailOnLockTimeout)
802812
if err != nil {
803813
return nil, err
804814
}

integration-tests/bats/stats.bats

Lines changed: 53 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -476,53 +476,76 @@ EOF
476476
}
477477

478478
@test "stats: worker thread quiesces and then runs again on new writes" {
479-
get_stats_time() {
480-
dolt sql -r csv -q 'call dolt_stats_info()' | tail -n 1 | sed -e 's|.*lastUpdate"":""||' -e 's|""}"$||'
479+
# genCnt goes up once every time stats is swapped.
480+
get_stats_gen() {
481+
dolt sql -r csv -q 'call dolt_stats_info()' | tail -n 1 | sed -e 's|.*genCnt"":||' -e 's|,.*||'
481482
}
482-
newtime=
483-
wait_for_new_time() {
484-
# We don't use dolt_stats_wait here, because that would
485-
# trigger a stats run on its own. Here we want to assert
486-
# that an incoming write triggered the run.
487-
cnt=0
488-
while [[ "$newtime" == "$origtime" ]]; do
489-
newtime=$(get_stats_time)
490-
if [ "$cnt" -eq 8 ]; then
491-
echo "took too long to run stats after a write" 1>&2
492-
exit 1
483+
484+
# Poll until the generation counter holds steady, i.e. the worker has
485+
# stopped running. Fails if it keeps advancing, which is what a
486+
# non-quiescing worker would do.
487+
wait_for_quiesced() {
488+
local prev cur stable
489+
prev=$(get_stats_gen)
490+
stable=0
491+
for _ in $(seq 1 20); do
492+
sleep 0.2
493+
cur=$(get_stats_gen)
494+
if [[ "$cur" == "$prev" ]]; then
495+
stable=$((stable+1))
496+
if [[ "$stable" -ge 5 ]]; then
497+
return 0
498+
fi
499+
else
500+
stable=0
493501
fi
494-
sleep 1
495-
done
502+
prev=$cur
503+
done
504+
echo "stats worker never quiesced; generation still advancing (last=$cur)"
505+
return 1
496506
}
497-
wait_for_quiesced() {
498-
dolt sql -q 'call dolt_stats_wait();'
499-
origtime=$(get_stats_time)
500-
cnt=0
501-
while [[ ! "$cnt" -eq 8 ]]; do
502-
newtime=$(get_stats_time)
503-
[[ "$newtime" == "$origtime" ]] || false
504-
cnt=$((cnt+1))
507+
508+
# Assert a trigger made the worker run, then quiesce, a small finite
509+
# number of times. $1 is the generation before the trigger.
510+
assert_ran_and_quiesced() {
511+
local base=$1 cur delta
512+
# First confirm the trigger actually woke the worker.
513+
for _ in $(seq 1 20); do
514+
cur=$(get_stats_gen)
515+
[[ "$cur" != "$base" ]] && break
516+
sleep 0.2
505517
done
518+
if [[ "$cur" == "$base" ]]; then
519+
echo "stats did not run after a write; generation stuck at $base"
520+
return 1
521+
fi
522+
# Then confirm it stops, after only a few runs.
523+
wait_for_quiesced
524+
cur=$(get_stats_gen)
525+
delta=$((cur - base))
526+
if [[ "$delta" -gt 6 ]]; then
527+
echo "stats ran more times than expected for one change ($base -> $cur)"
528+
return 1
529+
fi
506530
}
507531

508532
start_sql_server
509533

510534
# After it runs, stats quiesces until a new write.
511535
wait_for_quiesced
536+
base=$(get_stats_gen)
512537

513538
# Doing a write makes it run again
514539
dolt sql -q 'create table vals (id int primary key)'
515-
wait_for_new_time
516-
517-
wait_for_quiesced
540+
assert_ran_and_quiesced "$base"
541+
base=$(get_stats_gen)
518542

519543
# Creating a new database makes it run again
520544
dolt sql -q 'create database newdb'
521-
wait_for_new_time
522-
523-
wait_for_quiesced
545+
assert_ran_and_quiesced "$base"
546+
base=$(get_stats_gen)
524547

525548
# Writing against the new database makes it run again
526549
dolt sql -q 'create table `newdb`.`newtable` (id int primary key)'
527-
wait_for_new_time
550+
assert_ran_and_quiesced "$base"
528551
}
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
// Copyright 2026 Dolthub, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package main
16+
17+
import (
18+
"fmt"
19+
"testing"
20+
"time"
21+
22+
"github.com/stretchr/testify/require"
23+
24+
driver "github.com/dolthub/dolt/go/libraries/doltcore/dtestutils/sql_server_driver"
25+
)
26+
27+
// This test asserts the behavior of the fast-fail file-lock optimization for
28+
// dolt CLI commands which run against a data dir while a `dolt sql-server` is
29+
// running in that data dir.
30+
//
31+
// When opening a directory of databases, we don't want to wait for the
32+
// lock-acquire timeout once we've failed to get one.
33+
//
34+
// This test creates many databases and runs a server in the data dir. It then
35+
// spawns a CLI command in the data dir root and times it. The command must
36+
// finish well under the un-optimized cost, which is ~N * 100ms.
37+
38+
const (
39+
// perDatabaseLockTimeout mirrors lockFileTimeout in nbs/journal.go
40+
perDatabaseLockTimeout = 100 * time.Millisecond
41+
42+
// numReadOnlyDatabases is chosen large enough that the un-optimized,
43+
// serial lock-wait cost is unmistakably larger than the optimized cost
44+
numReadOnlyDatabases = 32
45+
46+
// We will try up to this many times to see good behavior. If we have
47+
// lots of scheduling contention, we can still be slow enough that we
48+
// think we are waiting on the lock files but we are actually just
49+
// running slowly.
50+
maxTrials = 3
51+
)
52+
53+
// makeEmptyDatabases creates n freshly initialized, empty databases as
54+
// subdirectories of the data dir root |rs|. Empty databases still need
55+
// locking, and so meet our use case.
56+
func makeEmptyDatabases(t *testing.T, rs driver.RepoStore, n int) {
57+
for i := 0; i < n; i++ {
58+
_, err := rs.MakeRepo(fmt.Sprintf("db_%02d", i))
59+
require.NoError(t, err)
60+
}
61+
}
62+
63+
// timeShowDatabases runs `dolt sql -q "show databases"` from the data dir root
64+
// |rs| up to maxTrials times, returning the best time seen. If the time is
65+
// ever < maxAcceptable, it returns that immediately.
66+
func timeShowDatabases(t *testing.T, rs driver.RepoStore, maxAcceptable time.Duration) time.Duration {
67+
var best time.Duration
68+
for trial := 0; trial < maxTrials; trial++ {
69+
cmd := rs.DoltCmd("sql", "-q", "show databases")
70+
start := time.Now()
71+
out, err := cmd.CombinedOutput()
72+
elapsed := time.Since(start)
73+
require.NoError(t, err, "show databases failed, output:\n%s", string(out))
74+
require.Regexp(t, "db_00", string(out))
75+
if trial == 0 || elapsed < best {
76+
best = elapsed
77+
}
78+
if best < maxAcceptable {
79+
// We take the very first run. Serially waiting for
80+
// the timeouts would have definitely taken longer
81+
// than this.
82+
return best
83+
}
84+
}
85+
return best
86+
}
87+
88+
// TestReadOnlyDatabaseLoadSkipsLockTimeout asserts that loading many read-only
89+
// databases behind a running sql-server does not serially pay the file-lock
90+
// timeout for every database.
91+
func TestReadOnlyDatabaseLoadSkipsLockTimeout(t *testing.T) {
92+
// No Parallel because it's just a bit sensitive to wall-clock time.
93+
u, err := driver.NewDoltUser()
94+
require.NoError(t, err)
95+
t.Cleanup(func() {
96+
u.Cleanup()
97+
})
98+
99+
rs, err := u.MakeRepoStore()
100+
require.NoError(t, err)
101+
makeEmptyDatabases(t, rs, numReadOnlyDatabases)
102+
103+
// Start a sql-server in the data dir root. It holds the file lock on every
104+
// database, forcing the CLI below to open them all read-only. The helper
105+
// blocks until the server is up and serving, so server startup time is not
106+
// included in the measurement below.
107+
var ports DynamicResources
108+
ports.global = &GlobalPorts
109+
ports.t = t
110+
RunServerUntilEndOfTest(t, rs, &driver.Server{
111+
Args: []string{"--port", `{{get_port "server"}}`},
112+
DynamicPort: "server",
113+
}, &ports)
114+
115+
unoptimizedCost := numReadOnlyDatabases * perDatabaseLockTimeout
116+
maxAcceptable := unoptimizedCost / 2
117+
118+
best := timeShowDatabases(t, rs, maxAcceptable)
119+
120+
require.Lessf(t, best, maxAcceptable,
121+
"loading %d read-only databases behind a running sql-server took %s; "+
122+
"without the fast-fail lock optimization this would have taken about "+
123+
"%s (%d serial %s file-lock waits)",
124+
numReadOnlyDatabases, best, unoptimizedCost, numReadOnlyDatabases, perDatabaseLockTimeout)
125+
}

0 commit comments

Comments
 (0)