Optimistic locking #265

dranikpg · 2022-02-19T14:45:55Z

Hi! I've implemented optimistic locking as discussed here.

It's automatically enabled by including a LockVersion int field into the struct.

It adds a where lock_version = $ clause to select only valid records and uses set lock_version = lock_version + 1 for updating locks.

What I've changed

Mainly repository.update() and repository.delete(). They now include custom logic for applying locks.

Because using SQL expressions like += requires reloading the whole struct, I've added a NoReload flag to Mutates to surpress triggering reloads for lock updates. In case no reload is required, the locks are incremented manually.

Tests

I've added simple tests for updating and deleting actual and stale records.

dranikpg · 2022-02-19T18:10:34Z

@Fs02 The reltest setup somewhat confuses me 🤔 An older version of it is still under rel/reltest? How should I push a breaking API change (Repository.Delete()) so that rel and reltest stay in sync?

Fs02 · 2022-02-20T02:18:36Z

@dranikpg ah right, let me remove it first

Fs02 · 2022-02-20T05:43:59Z

Removed deprecated reltest: #266

dranikpg · 2022-02-20T11:19:12Z

That still leaves me wondering how to update both rel and reltest as they depend on each other. The CI currently breaks because reltest still uses the old Repository interface. Updating reltest requires having an updated version of rel itself 🔁

One solution is to specify dependencies with tags/branches during PR reviews and then change them before merging.

Fs02 · 2022-02-23T06:42:57Z

sorry for the wait, I've decided to extract migrator package, now rel doesn't depends on reltest anymore
#267

codecov · 2022-02-23T16:29:07Z

Codecov Report

Merging #265 (2bfede5) into master (23fd349) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #265   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           33        33           
  Lines         2737      2768   +31     
=========================================
+ Hits          2737      2768   +31

Impacted Files	Coverage Δ
document.go	`100.00% <100.00%> (ø)`
filter_query.go	`100.00% <100.00%> (ø)`
mutation.go	`100.00% <100.00%> (ø)`
repository.go	`100.00% <100.00%> (ø)`

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 23fd349...2bfede5. Read the comment docs.

dranikpg · 2022-02-23T17:28:47Z

document.go

@@ -517,13 +520,23 @@ func extractBoolFlag(name string) DocumentFlag {
 	return Invalid
 }

+func extractIntFlag(name string) DocumentFlag {
+	if name == "lock_version" {


Field names like "created_at" and "updated_at" are also not constants, so I've also kept this a literal everywhere 🤷🏻

dranikpg · 2022-02-23T17:30:05Z

mutation.go

+	Type     ChangeOp
+	Field    string
+	Value    interface{}
+	NoReload Reload


Skip reload for queries that use SQL expressions which can be evaluated without querying

dranikpg · 2022-02-23T17:31:34Z

rel_test.go

+	Address   Address    `ref:"address_id" fk:"id"`
+	Histories *[]History `ref:"id" fk:"transaction_id"`


Auto field name deduction doesn't work with embedded structs 😞 I'll fix this

dranikpg · 2022-02-23T17:32:18Z

repository.go

+	if unscoped {
+		return 0, false
+	}


See comments below

dranikpg · 2022-02-23T17:32:36Z

repository.go

+		if err := r.applyMutates(cw, doc, mutation, filter); err != nil {
+			return err


To reduce function complexity

dranikpg · 2022-02-23T17:33:26Z

repository.go

+		baseQueries = []Querier{filter, mutation.Unscoped, mutation.Cascade}
+		queries     = baseQueries


We have to reload mutation queriers (like version_lock += 1) when reloading a document, so we store the base slice

dranikpg · 2022-02-23T17:33:53Z

repository.go

+	if version, ok := r.lockVersion(*doc, mutation.Unscoped); ok {
+		Inc("lock_version").SkipReload().Apply(doc, &mutation)
+		queries = append(queries, LockVersion(version))


And add a query + mutation if the document has a version lock and the query is scoped

dranikpg · 2022-02-23T17:35:15Z

repository.go

+
+	var (
+		pField string
+		query  = r.withDefaultScope(doc.data, Build(doc.Table(), queries...), false)


I first thought of reusing withDefaultScope for applying version_lock mutations

dranikpg · 2022-02-23T17:47:17Z

repository.go

+	if mutation.Reload {
+		baseQuery := r.withDefaultScope(doc.data, Build(doc.Table(), baseQueries...), false)
+		if err := r.find(cw, doc, baseQuery.UsePrimary()); err != nil {
+			return err
+		}
+	} else if version, ok := r.lockVersion(*doc, mutation.Unscoped); ok {
+		doc.SetValue("lock_version", version+1)
+	}
+


But abstracting away version_lock logic doesn't allow us to simply modify the version lock afterwards.

Otherwise we could extend mutates and mark some of them to be "not included" in reload queries and to be computable without a reload. This would, for example, allow handling other increments with a version lock without reloading the document.

dranikpg · 2022-02-23T17:47:59Z

repository.go

-		cascade = Cascade(false)
+		cw       = fetchContext(ctx, r.rootAdapter)
+		doc      = NewDocument(record)
+		mutation = applyMutators(nil, false, false, mutators...)


Apply mutators without enabling cascading and "structset"

Fs02 · 2022-02-26T16:23:33Z

repository.go

+			return err
+		}
+	} else if version, ok := r.lockVersion(*doc, mutation.Unscoped); ok {
+		doc.SetValue("lock_version", version+1)


I think this will return incorrect version if multiple process is updating the same record concurrently without using lock

to make things simple, I think what we can do is:

if lock is enabled: automatically update using Set with new version value is computed from rel side

if lock is disabled: don't automatically update version, warn user that lock version will be replaced

Sorry for the long pause, had to take a break.

Why? If SET version = version + 1 WHERE version = x exectues successfully, then the version has to be x + 1. As far as I understand, we don't care about other fields. Mixing usage with and without locks is up to the user.

If SET version = version + 1 WHERE version = x exectues successfully, then the version has to be x + 1.

ah right this is true

but I can see the implementation can be simplified if we just do SET version = newVersion WHERE version = previousVersion, where we locally compute newVersion = previousVersion+1 in memory

this way, we don't have to introduce NoReload, semantics that rel.Inc/rel.Dec/rel.Fragment always trigger reload will remain true.
(it always trigger reload, because there's no way to compute the updated record correctly in memory)

Yeah, thats true.

They key problem here is that Set() calls are always applied to the document beforehand (in Mutate.Apply()), regardless of whether the record is found/any erros occured. This is the default behaviour for rel now.
Should version locks be rolled back in case of unsuccessful updates?

ah true,

Should version locks be rolled back in case of unsuccessful updates?

I think yes

dranikpg · 2022-03-20T09:23:36Z

I've removed all NoReload stuff and replaced the increment with a Set. A simple defer now rollbacks the version lock.

Fs02 · 2022-03-21T05:51:08Z

repository_test.go

+	adapter.AssertExpectations(t)
+}
+
+func TestRepository_LockVersion_Delete(t *testing.T) {


can you add a test for LockVersion + SoftDelete

Added. If I understand correctly, soft deletes don't update the documents fields?

Fs02

LGTM, Thank you!!

Optimistic locking

7e6b278

Merge branch 'master' into optimistic-locking

50e4c7e

Fix codeclimate issues

d2ebfa1

dranikpg commented Feb 23, 2022

View reviewed changes

Fs02 reviewed Feb 26, 2022

View reviewed changes

Replace increment with set. Add automatic rollback on error

96c97dc

dranikpg marked this pull request as ready for review March 20, 2022 09:21

Merge branch 'master' into optimistic-locking

ef8a76a

Fs02 reviewed Mar 21, 2022

View reviewed changes

Add test for soft deletes. Update version in soft delete

2bfede5

Fs02 approved these changes Mar 22, 2022

View reviewed changes

Fs02 merged commit f3a1b01 into go-rel:master Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimistic locking #265

Optimistic locking #265

dranikpg commented Feb 19, 2022

dranikpg commented Feb 19, 2022

Fs02 commented Feb 20, 2022

Fs02 commented Feb 20, 2022

dranikpg commented Feb 20, 2022

Fs02 commented Feb 23, 2022

codecov bot commented Feb 23, 2022 •

edited

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

dranikpg Feb 23, 2022

Fs02 Feb 26, 2022

dranikpg Mar 8, 2022

Fs02 Mar 9, 2022

dranikpg Mar 9, 2022

Fs02 Mar 10, 2022

dranikpg commented Mar 20, 2022 •

edited

Fs02 Mar 21, 2022

dranikpg Mar 21, 2022

Fs02 left a comment

		Address Address `ref:"address_id" fk:"id"`
		Histories *[]History `ref:"id" fk:"transaction_id"`

		if err := r.applyMutates(cw, doc, mutation, filter); err != nil {
		return err

		baseQueries = []Querier{filter, mutation.Unscoped, mutation.Cascade}
		queries = baseQueries

Optimistic locking #265

Optimistic locking #265

Conversation

dranikpg commented Feb 19, 2022

What I've changed

Tests

dranikpg commented Feb 19, 2022

Fs02 commented Feb 20, 2022

Fs02 commented Feb 20, 2022

dranikpg commented Feb 20, 2022

Fs02 commented Feb 23, 2022

codecov bot commented Feb 23, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg commented Mar 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fs02 left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 23, 2022 •

edited

dranikpg commented Mar 20, 2022 •

edited