There is a danger of deadlock wherever we have database operations
inside a database iterator. The iterator itself pins a connection, so
the nested operations need another connection; if we've reached
maxOpenConns then it blocks until a connection is released. If all
connections are consumed by such iterators then none can make progress.
Luckily, we have limited concurrency for most such iterator loops. They
are part of scanning, pulling, reverting, etc where there is only ever
one such routine per folder. The exception is block reuse in the copier
routine, which is limited by the `Copiers` setting per folder. This
could in practice deadlock since you could set copiers to eight and end
up with six `AllLocalBlocksWithHash` iterators when `maxDBConns=6`, all
of which need to make additional database calls inside the loop.
This PR fixes the problem twice;
- The problematic loop does not need to be reentrant. The set of blocks
that may be returned by the iterator is finite so we can easily just
collect them to a slice before we start processing them. This avoids the
problem entirely.
- We do not need to limit database connections as strictly as we
currently do. Increase the maximum allowed, while reducing the number of
held-open idle connections slightly. This is not an exact science, but
ideally we want "most" operations to be able to use the pinned
connections to avoid cache churn. Most operations are short lived
queries, or single-goroutine iterators with short lived queries inside,
so four connections seems like it should usually be enough. 🤷
- Additionally, set a cap on Copiers. Currently you could set it to an
arbitrarily large number, which is not advantageous. Limit it to
2*NumCPU which scales somewhat with system performance.
Signed-off-by: Jakob Borg <jakob@kastelo.net>
This changes the files table to use normalisation for the names and
versions. The idea is that these are often common between all remote
devices, and repeating an integer is more efficient than repeating a
long string. A new benchmark bears this out; for a database with 100k
files shared between 31 devices, with some worst case assumption on
version vector size, the database is reduced in size by 50% and the test
finishes quicker:
Current:
db_bench_test.go:322: Total size: 6263.70 MiB
--- PASS: TestBenchmarkSizeManyFilesRemotes (1084.89s)
New:
db_bench_test.go:326: Total size: 3049.95 MiB
--- PASS: TestBenchmarkSizeManyFilesRemotes (776.97s)
The other benchmarks end up about the same within the margin of
variability, with one possible exception being that RemoteNeed seems to
be a little slower on average:
old files/s new files/s
Update/n=RemoteNeed/size=1000-8 5.051k 4.654k
Update/n=RemoteNeed/size=2000-8 5.201k 4.384k
Update/n=RemoteNeed/size=4000-8 4.943k 4.242k
Update/n=RemoteNeed/size=8000-8 5.099k 3.527k
Update/n=RemoteNeed/size=16000-8 3.686k 3.847k
Update/n=RemoteNeed/size=30000-8 4.456k 3.482k
I'm not sure why, possibly that query can be optimised anyhow.
Signed-off-by: Jakob Borg <jakob@kastelo.net>
No practical effect, just a tiny bit of fun to stamp the database files
with an application ID that identifies them.
Signed-off-by: Jakob Borg <jakob@kastelo.net>
This reduces database migration memory usage in my test scenario from
3.8 GB to 440 MB. In principle I don't think we're causing many temp
tables to be generated anyway in normal usage, but if we do and someone
can benchmark a performance difference, we can add a tunable. I ran the
database benchmark before and after and didn't see a difference above
the noise level.
Signed-off-by: Jakob Borg <jakob@kastelo.net>
This changes the database structure to use one database per folder, with
a small main database to coordinate. Reverts the prior change to buffer
all files in memory when pulling, meaning there is now a phase where the
WAL file will grow significantly, at least for initial sync of folders
with many directories.
---------
Co-authored-by: bt90 <btom1990@googlemail.com>
Jakob BorgandGitHub
