Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SQLite concurrency problems by using BEGIN IMMEDIATE #10368

Merged
merged 6 commits into from Feb 26, 2020

Conversation

guillep2k
Copy link
Member

@guillep2k guillep2k commented Feb 19, 2020

This PR proposes a solution for the fortuitous deadlock errors produced in SQLite. The way it works is by preventing any two transactions (BeginTransaction() => Commit()) from running at the same time.

The reason I think this is needed is because I see no right way of orchestrating our queries in a way that they will never interfere with each other. As we move actions into queues, there are more operations running simultaneously.

I've tested this several times and I couldn't get a deadlock, while it was a 70~80% of getting one without this modification.

As a solution this might be a little extreme, but I'm leaving this here just in case. I labeled it as status/blocked until certain consensus is reached.

Sources:

https://www.sqlite.org/lockingv3.html
https://www.sqlite.org/unlock_notify.html
https://godoc.org/github.com/mattn/go-sqlite3#SQLiteDriver.Open

@guillep2k guillep2k added the pr/wip This PR is not ready for review label Feb 19, 2020
@guillep2k guillep2k changed the title Test locking immediate for SQLite3 DO NOT MERGE - Test locking immediate for SQLite3 Feb 19, 2020
@guillep2k guillep2k added status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR and removed pr/wip This PR is not ready for review status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR labels Feb 19, 2020
@guillep2k guillep2k changed the title DO NOT MERGE - Test locking immediate for SQLite3 Fix SQLite concurrency problems by using BEGIN IMMEDIATE Feb 19, 2020
@guillep2k guillep2k marked this pull request as ready for review February 19, 2020 21:06
@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

Merging #10368 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10368      +/-   ##
==========================================
- Coverage    43.7%   43.67%   -0.03%     
==========================================
  Files         586      586              
  Lines       81411    81411              
==========================================
- Hits        35582    35560      -22     
- Misses      41422    41442      +20     
- Partials     4407     4409       +2
Impacted Files Coverage Δ
models/user.go 49.3% <0%> (ø) ⬆️
models/action.go 48.82% <0%> (ø) ⬆️
services/pull/check.go 51.21% <0%> (-4.27%) ⬇️
services/pull/temp_repo.go 29.05% <0%> (-2.57%) ⬇️
services/pull/patch.go 60.37% <0%> (-2.52%) ⬇️
models/unit.go 37.03% <0%> (-2.47%) ⬇️
services/pull/pull.go 34.89% <0%> (-1.18%) ⬇️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
models/error.go 28.71% <0%> (-0.52%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0574fc0...08a6461. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 19, 2020
@guillep2k guillep2k added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Feb 22, 2020
@6543
Copy link
Member

6543 commented Feb 25, 2020

EDIT: what would be the alternative ... since sqlite is only for smal installations I think querys can be exec sequencial not paralell. But we should leave a big note in the documentation for sqlite and if it is the right desission as backend

@mrsdizzie
Copy link
Member

This seems like an appropriate solution.

Here is a relevant section from the definitive guide to Sqlite:

Deadlocks
Although you may find the preceding discussion on locking to be interesting, you are probably wondering at this point why any of it matters. Why do you need to know about locking? Well, if you don’t know what you are doing, you can end up in a deadlock.
Consider the following scenario illustrated in Table 4-2. Two sessions, A and B—completely oblivious to one another—are working on the same database at the same time. Session A issues the first command, B the second and third, A the fourth, and so on.
Table 4-2. A Portrait of a Deadlock

Screen Shot 2020-02-26 at 4 12 18 PM

Both sessions wind up in a deadlock. Session B was the first to try to write to the database and therefore has a pending lock. A attempts to write but fails when INSERT tries to promote its shared lock to a reserved lock.

For the sake of argument, let’s say that A decides to just wait around for the database to become writable. So does B. Then at this point, everyone else is effectively locked out too. If you try to open a third session, it won’t even be able to read from the database. The reason is that B has a pending lock, which prevents any sessions from acquiring shared locks. So, not only are A and B deadlocked, they have locked everyone else out of the database as well. Basically, you have a shared lock and one pending lock that don’t want to relinquish control, and until one does, nobody can do anything.

How do you avoid a deadlock? It’s not like A and B can sit down in a meeting and work it out with their lawyers. A and B don’t even know each other exists. The answer is to pick the right transaction type for the job.

Transaction Types
SQLite has three different transaction types that start transactions in different locking states. Transactions can be started as deferred, immediate, or exclusive. A transaction’s type is specified in the begin command:
begin [ deferred | immediate | exclusive ] transaction;
A deferred transaction does not acquire any locks until it has to. Thus, with a deferred transaction, the begin statement itself does nothing—it starts in the unlocked state. This is the default. If you simply issue a begin, then your transaction is deferred and therefore sitting in the unlocked state. Multiple sessions can simultaneously start deferred transactions at the same time without creating any locks. In this case, the first read operation against a database acquires a shared lock, and similarly the first write operation attempts to acquire a reserved lock.

An immediate transaction attempts to obtain a reserved lock as soon as the begin command is executed. If successful, begin immediate guarantees that no other session will be able to write to the database. As you know, other sessions can continue to read from the database, but the reserved lock prevents any new sessions from reading. Another consequence of the reserved lock is that no other sessions will be able to successfully issue a begin immediate or begin exclusive command. SQLitewill return a SQLITE_BUSY error.
During this time, you can make some modifications to the database, but you may not necessarily be able to commit them. When you call commit, you could get SQLITE_BUSY. This means that there are other readers active, as in the earlier example. Once they are gone, you can commit the transaction.
An exclusive transaction obtains an exclusive lock on the database. This works similarly to immediate, but when you successfully issue it, exclusive guarantees that no other session is active in the database and that you can read or write with impunity.

The crux of the problem in the preceding example is that both sessions ultimately wanted to write to the database, but they made no attempt to relinquish their locks. Ultimately, it was the shared lock that caused the problem. If both sessions had started with begin immediate, then the deadlock would not have occurred. In this case, only one of the sessions would have been able to enter begin immediate at one time, while the other would have to wait. The one that has to wait could keep retrying with the assurance that it would eventually get in. begin immediate and begin exclusive, if used by all sessions that want to write to the database, provide a synchronization mechanism, thereby preventing deadlocks. For this approach to work, though, everyone has to follow the rules.

The bottom line is this: if you are using a database that no other connections are using, then a simple begin will suffice. If, however, you are using a database that other connections are also writing to, both you, and they should use begin immediate or begin exclusive to initiate transactions. It works out best that way for both of you.

@guillep2k
Copy link
Member Author

Note that when it says:

SQLite will return a SQLITE_BUSY error

in our case it will just block because we're using the sqlite_unlock_notify library.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 26, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 26, 2020
@guillep2k guillep2k removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Feb 26, 2020
@guillep2k guillep2k added type/bug and removed type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Feb 26, 2020
@lafriks lafriks added this to the 1.12.0 milestone Feb 26, 2020
@lunny lunny merged commit 4a2d239 into go-gitea:master Feb 26, 2020
@guillep2k guillep2k deleted the txlock-test branch February 26, 2020 23:52
guillep2k added a commit to guillep2k/gitea that referenced this pull request Feb 27, 2020
)

* Test locking immediate for SQLite3

* fix url field separator

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: zeripath <art27@cantab.net>
@guillep2k guillep2k added the backport/done All backports for this PR have been created label Feb 27, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants