Use an atomic operation to calculate Issue.Index #7944

guillep2k · 2019-08-22T19:44:57Z

Please notice that I've referenced the master version of xorm (xorm.io/builder,github.com/go-xorm/xorm) to build this. I'm not sure if I did it correctly.

Closes #7887

This PR takes advantage of the new feature supported by xorm of building the INSERT from a SELECT.

This is the new code built by xorm (SQLite):

2019/08/22 12:21:16 .../xorm/session_raw.go:178:exec() [I] [SQL] INSERT INTO `issue` (`repo_id`, `poster_id`, `original_author`, `original_author_id`, `name`, `content`, `milestone_id`, `priority`, `is_closed`, `is_pull`, `num_comments`, `ref`, `deadline_unix`, `created_unix`, `updated_unix`, `closed_unix`, `is_locked`, `index`) SELECT ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, coalesce(MAX(`index`),0)+1 FROM `issue` WHERE (repo_id=?) []interface {}{1, 1, "", 0, "This is a pull title", "", 0, 0, false, true, 0, "", 0, 1566487276, 1566487276, 0, false, 1} - took: 1.240578ms

Instead of:

2019/08/22 16:39:39 .../xorm/session_raw.go:83:queryRows() [I] [SQL] SELECT COALESCE((SELECT MAX(`index`) FROM issue WHERE repo_id = ?),0) []interface {}{10} - took: 67.98µs
2019/08/22 16:39:39 .../xorm/session_raw.go:178:exec() [I] [SQL] INSERT INTO `issue` (`repo_id`,`index`,`poster_id`,`original_author`,`original_author_id`,`name`,`content`,`milestone_id`,`priority`,`is_closed`,`is_pull`,`num_comments`,`ref`,`deadline_unix`,`created_unix`,`updated_unix`,`closed_unix`,`is_locked`) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) []interface {}{10, 2, 12, "", 0, "create a success pr", "", 0, 0, false, true, 0, "", 0, 1566502779, 1566502779, 0, false} - took: 521.156µs

This will prevent the possible race condition mentioned in #7848 by making the index number calculated in the same operation as the insert.

A thread with an alternate solution and its discussion is: #7898,

codecov-io · 2019-08-22T20:02:58Z

Codecov Report

Merging #7944 into master will increase coverage by 0.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #7944      +/-   ##
==========================================
+ Coverage   41.47%   41.49%   +0.01%     
==========================================
  Files         478      479       +1     
  Lines       63975    63965      -10     
==========================================
+ Hits        26535    26541       +6     
+ Misses      33988    33968      -20     
- Partials     3452     3456       +4

Impacted Files	Coverage Δ
models/issue.go	`50% <50%> (+0.03%)`	⬆️
modules/process/manager.go	`76.81% <0%> (-4.35%)`	⬇️
models/repo_indexer.go	`66.66% <0%> (-2.04%)`	⬇️
modules/setting/setting.go	`45.06% <0%> (-0.28%)`	⬇️
models/migrations/v81.go	`0% <0%> (ø)`	⬆️
models/migrations/v27.go	`0% <0%> (ø)`	⬆️
models/convert.go	`0% <0%> (ø)`	⬆️
models/user.go	`50.68% <0%> (ø)`	⬆️
routers/install.go	`0% <0%> (ø)`	⬆️
cmd/dump.go	`0% <0%> (ø)`	⬆️
... 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 26af340...1a528db. Read the comment docs.

guillep2k · 2019-08-22T20:13:55Z

I've just noticed that xorm re-reads the calculated value for me, so calling getIssueByID() is unnecessary. I'll update this PR in a minute.

guillep2k · 2019-08-23T21:26:05Z

*Rebased due to changes in go.mod at master.

sapk · 2019-08-23T21:49:53Z

I haven't tested it but does it select the max index for the specific repo id or across all repo ?

sapk

Looking at the generated SQL request from xorm repo, it should be good.

guillep2k · 2019-08-24T06:02:31Z

Something wrong with the issue/comment template? Mmm....

sapk · 2019-08-24T09:40:08Z

On a side note: the concurrency problem exist also for PR since they used GetMaxIndexOfIssue. But first let validate this PR. 😄

…into issue-idx-insert-select

lafriks · 2019-08-24T12:30:56Z

models/issue.go

@@ -1109,10 +1103,19 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 	}

 	// Milestone and assignee validation should happen before insert actual object.
-	if _, err = e.Insert(opts.Issue); err != nil {
+	if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").


Is this really supported in all db types?

yes it should

@lafriks Check my comment about DB support here: #7898 (comment)

As far as the syntax and the functionality, they are supported. About the atomicity, in theory it's there, but It's very difficult to test. There some WIP about that here #7950 and here #7959.

To validate there is an integration TestAPICreateIssue that validate the creation across all db types.

guillep2k · 2019-08-24T13:40:00Z

On a side note: the concurrency problem exist also for PR since they used GetMaxIndexOfIssue. But first let validate this PR. 😄

@sapk In fact, that value is being ignored (it always was). I might PR for that later.

guillep2k · 2019-08-24T15:14:46Z

@typeless , @sapk, @lunny
The good news is that I've managed to create a test that can make this fail.
The bad news is that I've managed to create a test that can make this fail.

See #7959.

Sadly, for solving our duplicate index problem, the INSERT ... SELECT method only works with SQLite. All MySQL, MSSQL and PostgreSQL fail one way or another:

MSSQL: insert successfully but not returned id (internally it's Cannot insert duplicate key row in object 'dbo.issue' with unique index 'UQE_issue_repo_index'.).
MySQL: Deadlock found when trying to get lock; try restarting transaction.
MySQL8: Deadlock found when trying to get lock; try restarting transaction.
PostgreSQL: pq: duplicate key value violates unique constraint "UQE_issue_repo_index".

With MySQL, a lock is in place but somehow creates a deadlock. With MSSQL and PostgreSQL, if there's a lock, it is not kept from the SELECT part into the INSERT part.

So, we're back to square one regarding this, and the only viable solution at the moment seems to retry the insert, as @lunny proposed.

guillep2k · 2019-08-24T15:21:33Z

Close due to failing tests on #7959

lafriks · 2019-08-24T17:15:28Z

Did you try SetExpr with full select max?

guillep2k · 2019-08-24T17:51:18Z

Did you try SetExpr with full select max?

@lafriks Yes, that's #7959. I've branched this to add the test. I didn't want to add the test here for performance reasons (the integration tests would have a lot of time added to them for everybody).

lunny · 2019-08-25T12:34:56Z

I don't that will only supports sqlite3 because I have added tests on go-xorm/xorm#1401 and it will be executed on sqlite/mysql/postgres/mssql both on circleci and drone.
https://cloud.drone.io/go-xorm/xorm/67 and https://circleci.com/gh/go-xorm/xorm/952?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

guillep2k · 2019-08-25T13:04:46Z

I don't that will only supports sqlite3 because I have added tests on go-xorm/xorm#1401 and it will be executed on sqlite/mysql/postgres/mssql both on circleci and drone.

@lunny I'm sorry, of course the statement works well in all databases. Your modification generates the right code and it passed all the existing tests on Gitea.

However, it does not fix #7887. It does not work if multiple users attempt to insert at the same time. There are all kinds of concurrency problems. Please see my test in #7959 for details.

guillep2k mentioned this pull request Aug 22, 2019

Retry create issue to cope with duplicate keys #7898

Merged

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 22, 2019

lunny added the type/enhancement An improvement of existing functionality label Aug 23, 2019

lunny added this to the 1.10.0 milestone Aug 23, 2019

typeless mentioned this pull request Aug 23, 2019

WIP: Add a concurrent test for issue creation #7950

Closed

guillep2k added 4 commits August 23, 2019 18:02

Replace insert by insert.where to calculate issue.index atomically

a1b93a2

Remove unnecessary read-back of the index value

3ae8be2

Checkout latest xorm code to include go-xorm/xorm#1401

d59f4bf

Apply make vendor

741d920

guillep2k force-pushed the issue-idx-insert-select branch from aad1cf1 to 741d920 Compare August 23, 2019 21:18

sapk approved these changes Aug 23, 2019

View reviewed changes

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 Aug 23, 2019

Remove dead code

8473a96

guillep2k mentioned this pull request Aug 24, 2019

Experiment (NOT FOR MERGING): Add test to stress insert ... select for Issue #7959

Closed

Merge branch 'master' into issue-idx-insert-select

bc566c1

guillep2k added 2 commits August 24, 2019 08:27

Reinsert reading-back of Issue.Index

dd23020

Merge branch 'issue-idx-insert-select' of github.com:guillep2k/gitea …

1a528db

…into issue-idx-insert-select

lafriks reviewed Aug 24, 2019

View reviewed changes

guillep2k closed this Aug 24, 2019

guillep2k deleted the issue-idx-insert-select branch October 3, 2019 23:02

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use an atomic operation to calculate Issue.Index #7944

Use an atomic operation to calculate Issue.Index #7944

guillep2k commented Aug 22, 2019

codecov-io commented Aug 22, 2019 •

edited

Loading

guillep2k commented Aug 22, 2019

guillep2k commented Aug 23, 2019

sapk commented Aug 23, 2019

sapk left a comment

guillep2k commented Aug 24, 2019

sapk commented Aug 24, 2019

lafriks Aug 24, 2019

sapk Aug 24, 2019

guillep2k Aug 24, 2019 •

edited

Loading

sapk Aug 24, 2019

guillep2k commented Aug 24, 2019

guillep2k commented Aug 24, 2019

guillep2k commented Aug 24, 2019

lafriks commented Aug 24, 2019

guillep2k commented Aug 24, 2019

lunny commented Aug 25, 2019

guillep2k commented Aug 25, 2019

Use an atomic operation to calculate Issue.Index #7944

Use an atomic operation to calculate Issue.Index #7944

Conversation

guillep2k commented Aug 22, 2019

codecov-io commented Aug 22, 2019 • edited Loading

Codecov Report

guillep2k commented Aug 22, 2019

guillep2k commented Aug 23, 2019

sapk commented Aug 23, 2019

sapk left a comment

Choose a reason for hiding this comment

guillep2k commented Aug 24, 2019

sapk commented Aug 24, 2019

lafriks Aug 24, 2019

Choose a reason for hiding this comment

sapk Aug 24, 2019

Choose a reason for hiding this comment

guillep2k Aug 24, 2019 • edited Loading

Choose a reason for hiding this comment

sapk Aug 24, 2019

Choose a reason for hiding this comment

guillep2k commented Aug 24, 2019

guillep2k commented Aug 24, 2019

guillep2k commented Aug 24, 2019

lafriks commented Aug 24, 2019

guillep2k commented Aug 24, 2019

lunny commented Aug 25, 2019

guillep2k commented Aug 25, 2019

codecov-io commented Aug 22, 2019 •

edited

Loading

guillep2k Aug 24, 2019 •

edited

Loading