Skip to content

DATAJDBC-493 Resolve Deadlock occurs when competing with Jdbc Repository Update and Delete. #196

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

DATAJDBC-493 Resolve Deadlock occurs when competing with Jdbc Repository Update and Delete. #196

wants to merge 8 commits into from

Conversation

mhyeon-lee
Copy link
Contributor

@schauder
Copy link
Contributor

This looks decent, but I'd like to have a complete solution DATAJDBC-493 before merging parts of the fix.

I'd therefore appreciate it if you could combine this PR with #195 and possibly the rest of what you outlined in DATAJDBC-493 .

@mhyeon-lee mhyeon-lee changed the title DATAJDBC-498 Add LockCluase to dialect DATAJDBC-493 Resolve Deadlock occurs when competing with Jdbc Repository Update and Delete. Apr 30, 2020
@mhyeon-lee
Copy link
Contributor Author

@schauder
I added the codes below. Please review.

  1. Add methods for Select With Lock (SqlGenerator, DataAccessStrategy)
  2. Acquire LOCK before executing delete in JdbcAggregateTemplate.
    (LOCK is valid in transaction scope, so it is executed only in ActiveTransaction.)
  3. The tests of # 195 are combined, and the test is successful.
    (Deadlock Exception is not thrown.)

@mhyeon-lee
Copy link
Contributor Author

I needed to modify the PostgreSQL lock clause, so I committed it further.

64bf8af

schauder
schauder requested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acquiring a lock should be a DbAction wich gets generated and executed with all the other DbActions.

If there are no other entities in the aggregate which get stored in other tables, the lock is superfluous and should not be acquired.

The lock should also be acquired for deleteAll , without actually returning the entities.

Which is something that would be nice for the single locks as well, after all we don't really need the values and wasting resources by actually hydrating the objects involved.

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java Outdated
@@ -352,6 +355,9 @@ public void deleteAll(Class<?> domainType) {
entity = triggerBeforeDelete(entity, id, change);
change.setEntity(entity);

// [DATAJDBC-493] Acquire Lock to avoid DeadLock
this.acquireLockIfActualTransactionActive(id, domainType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be encoded in a DbAction and then executed by the executor.

@mhyeon-lee
Copy link
Contributor Author

@schauder
a2c0e31

  1. Change the method of DataAccessStrategy acquireLockById. This method does not return a value. (without returning the entities, no hydrating the objects. just check id exists with lock)

  2. Add DbAction AcquireLockRoot AcquireLockAllRoot and then executed by the executor.

  3. AcquireLock DbAction is executed only when the Aggregate has a Reference Entity.

@schauder
Copy link
Contributor

schauder commented May 5, 2020

Thanks, I'll take a look.

schauder pushed a commit that referenced this pull request May 6, 2020
@schauder
DATAJDBC-493 - Avoids deadlocks by acquiring lock on aggregate root t…
04c29f4 
…able.

Introduces infrastructure to obtain locks and uses them to acquire locks on the table of the aggregate root before deleting references.
Without this lock deletes access non root entities before the aggregate root, which is the opposite order of updates and thus may cause deadlocks.

Original pull request: #196.
schauder added a commit that referenced this pull request May 6, 2020
@schauder
DATAJDBC-493 - Polishing.
aa05b79 
Using `execute` instead of `query` since we are not interested in the results.
Refactoring of the concurrency tests.
Make the concurrency tests run with all databases.
Added support for DB2.
Moved AnsiDialect to the main sources so it can be referenced in other Dialects.

Original pull request: #196.
@schauder
Copy link
Contributor

schauder commented May 6, 2020

Thanks, that is a nice fix.
I squashed, polished and merged it into master.

@schauder schauder closed this May 6, 2020
This was referenced Dec 31, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schauder schauder schauder requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mhyeon-lee @schauder