Sequencial issue index numbering with pessimistic locking mechanism

[guillep2k]

As a stepping stone of my WIP #9787, I needed a pessimistic locking mechanism. Since this functionality is useful to solve many problems, I thought I'd use it for a simpler case to test the waters.

This PR introduces the pessimistic locking mechanism and finally gives proper solution to the problem from #7887 of calculating the index for two issues inserted at the very same time in the same repository.

The pessimistic locking is similar in concept to a mutex but works at the database level. This will allow synchronization between different Gitea instances that share the same database but run in different servers.

In addition, this PR uses the new locking mechanism to store the last issue index used for each repository in order to calculate the next value. This is a requirement for implementing the ability of deleting issues (#923), because we will want to avoid re-using the same issue number when one has been deleted (we're currently calculating the issue index by executing a SELECT MAX(index)+1, which will repeat values if issues are deleted from the end of the count).

Background

Short explanation of the concurrency problem: #7887 (comment)

Last year I tried to solve the potential problem of two threads attempting to create issues in the same repo at the same time: one of the transactions would fail because the other used the same issue index number. Reasons are too complex to detail here (see my attempts #7894, #7950, #7898, #8005, and the accepted makeshift solution in #8307), but ultimately the problem lays in the optimistic locking used in Gitea: two concurrent transactions will run unaware of the other until one of them commits, rendering the other transaction's calculations invalid.

What this solves

Currently we simply retry the issue creation if the insert fails, and it should fail very rarely. However, the proper way to solve this is by effectively serializing the number assignment.

This PR provides the serialization mechanism so it can be used in more complex scenarios later.

[codecov-io]

Codecov Report

Merging #9931 into master will decrease coverage by <.01%.
The diff coverage is 41.46%.

@@            Coverage Diff             @@
##           master    #9931      +/-   ##
==========================================
- Coverage   43.72%   43.72%   -0.01%     
==========================================
  Files         587      587              
  Lines       81090    81097       +7     
==========================================
  Hits        35458    35458              
- Misses      41244    41250       +6     
- Partials     4388     4389       +1

Impacted Files	Coverage Δ
modules/markup/markup.go	76.81% <ø> (-0.34%)	⬇️
modules/markup/markdown/goldmark.go	66.07% <ø> (-0.31%)	⬇️
models/issue.go	53.83% <0%> (+0.04%)	⬆️
services/pull/commit_status.go	5% <0%> (-0.2%)	⬇️
models/repo.go	51.32% <0%> (-0.04%)	⬇️
models/migrate.go	0% <0%> (ø)	⬆️
models/attachment.go	57.3% <100%> (+1.02%)	⬆️
modules/templates/helper.go	40.73% <100%> (ø)	⬆️
modules/notification/ui/ui.go	70.37% <73.33%> (ø)	⬆️
modules/process/manager.go	74.69% <0%> (-3.62%)	⬇️
... and 7 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 299d313...cea7c4f. Read the comment docs.

[zeripath]

Hmm thinking on the lock process should probably provide a DBContext itself.

[guillep2k]

Hmm thinking on the lock process should probably provide a DBContext itself.

You mean something like (function names aside)...?:

WithResourceLocked("my-type",444, func(){
    ... do something ...
})

[zeripath]

Something like that - I've not fleshed it out in my brain though.

Callbacks aren't greatly favoured in the Go world - for example, the lack of a io.With( ... ) function or with keyword - with preference for defers but I think there's definitely a place for them.

It might be that a specific LockedDBContext or something like that is better.

[guillep2k]

Something like that - I've not fleshed it out in my brain though.

It might be that a specific LockedDBContext or something like that is better.

It's not that these functions need a defer, because the update will be effective on commit (or cleaned up on rollback). Anyway, this kind of functions can easily be added later without breaking anything, so we can wait for more use cases to decide.

[guillep2k]

@zeripath done. I've reworked the interface a little and added support for LockKey == 0 in case it doesn't make sense to pick a particular value for it.

[lunny]

And why not to use an optimistic lock but a pessimistic lock? The record seems not be locked when select ?

[guillep2k]

And why not to use an optimistic lock but a pessimistic lock? The record seems not be locked when select ?

@lunny See my explanation of the concurrency problem with optimistic locks: #7887 (comment)

The records are not locked on SELECT unless it's a SELECT FOR UPDATE, but that doesn't work for new records (INSERT) in all of our supported databases.

This PR solves the index problem in issue, but more than anything it provides a standarized way of using lock on operations that otherwise could get wrong results (as it's happening now with the issue index calculation, that's why we need to retry if it fails). As you can see, with a pessimistic lock I was able to get rid of the retries in that case.

I've tried many combinations (SELECT ... FOR UPDATE, INSERT, SELECT ... FOR UPDATE then INSERT, etc.), but UPSERT was the only method that passed all the tests.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

[6543]

i know time is less ...

[guillep2k]

@lunny done.

[lunny]

Closed per #15599