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

Query.forUpdate works like REFRESH / Lock entity and refresh L1 and L2 cache in a single database call #3020

Closed
rbygrave opened this issue Apr 1, 2023 Discussed in #2958 · 1 comment · Fixed by #3021
Assignees
Labels
Milestone

Comments

@rbygrave
Copy link
Member

rbygrave commented Apr 1, 2023

Discussed in #2958

Originally posted by AntoineDuComptoirDesPharmacies February 3, 2023
Hi everyone, Hi @rbygrave,

It could be interesting to have capability to refresh L1 and L2 cache while locking a row in database through SELECT FOR UPDATE.
Indeed, it will allow us to do only one SELECT to lock and refresh the data instead of two.

At the moment, if you lock the row without doing a clear call to refresh, you may experience strange behaviors like the following scenario :

In a transaction block :

  1. We SELECT an object by Id (L1 and L2 cache set)
  2. We lock this object through a SELECT FOR UPDATE command, (eg. DB.lock from ENH: Add lock(bean) method as convenience for DB pessimistic locking #2362). (Do not refresh L1 and L2 cache)
  3. Later in the transaction, we SELECT object by this same id (Return value from step 1)

What we noticed is that, if the row in database is updated by another process between step 1 and step 2, the step 3 will only see old values of the Bean because it will grab it from level 2 cache set in step 1.

If we disable level 2 cache on step 3, we will still see the old value because none caches are refreshed in step 2.

If we set PersistenceContext to QUERY in the SELECT FOR UPDATE of step 2, the Bean returned contains the fresh values (from database) but it do not refresh level 1 and level 2 cache neither.

Solution is to call DB.refresh after DB.lock in step 2 but it imply two database query instead of one.

Yours faithfully,
LCDP

@rbygrave rbygrave self-assigned this Apr 1, 2023
@rbygrave
Copy link
Member Author

rbygrave commented Apr 1, 2023

The last comment in there is from me with:

Currently I think this means we could treat this like a bug. What that means is that:

I can't think of a use case where the current behavior is useful / what people would ever want. If the use case somehow does not want refresh then that means it should want query.setPersistenceContextScope(QUERY) rather than 'just ignore the fresh database values' (the current behavior does not seem useful in any way)
I think we should look to just change the behavior and not try and support some feature toggle / property configuration that turns it off because it seems like the current behavior of ignoring the fresh db values is never useful plus it would align with the behavior with EclipseLink (and maybe other JPA providers)

rbygrave added a commit that referenced this issue Apr 1, 2023
Query.forUpdate() will load data into beans existing in the persistence context.
In discussion the thinking is that we can think of this as a bug fix.
@rbygrave rbygrave linked a pull request Apr 2, 2023 that will close this issue
@rbygrave rbygrave added this to the 13.17.0 milestone Apr 5, 2023
rbygrave added a commit that referenced this issue Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants