Don't ignore minimalPut parameter in ReadWriteCache #3178
Conversation
| @@ -85,10 +85,20 @@ public bool IsPuttable(long txTimestamp, object newVersion, IComparer comparator | |||
| { | |||
| return false; | |||
| } | |||
| return Version == null ? | |||
There was a problem hiding this comment.
Seems CachedItem converted to lock CacheLock behaves differently for non-versioned entity.
CachedItem behaved as minimalPut is always true and CacheLock as minimalPut is always false (so CacheLock allows to update existing non-versioned entity, and CachedItem - always skips put for existing item)
Not sure if it's a bug - I kept this behavior for now in CacheLock.
P.S. Also I'm a bit confused how UnlockTimestamp and Timeout properties are used..
There was a problem hiding this comment.
CacheLock behaves differently for non-versioned entity
Actually let's try to properly support it with 8f4fd4e
There was a problem hiding this comment.
UnlockTimestamp initial value (-1) is irrelevant until Multiplicity has dropped to 0, at which point UnlockTimestamp is updated with the unlocking timestamp and starts to be relevant: it avoids a concurrent "stale" transaction to put stale data in the cache.
The Timeout here is not actually a timeout, it is a TimeoutTimestamp: it has been computed from the locking timestamp to which the timeout has been added.
There was a problem hiding this comment.
it avoids a concurrent "stale" transaction to put stale data in the cache.
So it's OK that UnlockTimestamp is ignored for versioned entity (as we know it's either new or same version entity)?
There was a problem hiding this comment.
If the cached version is older than the one to be put yes. But with your change, it may be as old, in which case this is debattable.
There was a problem hiding this comment.
But with your change, it may be as old, in which case this is debattable.
Why old? My change allows only to put the same version (Compare returns 0).
There was a problem hiding this comment.
I am just meaning the same age: cached version as old as the one to put, (or as new as,) that is just a way to tell "same age". This case is normally useful only for adding data for lazy loaded properties which was not already loaded and cached.
If the version is strictly newer, we know for sure that the cached data is stale and so, there is no harm in updating it with newer data, even if the session putting data in cache is not timestamped after the unlock timestamp. (Better to have less stale data.)
If the version is the same while the unlock timestamp tell us the session putting in cache may be stale, is it better to let other sessions see the additional date the put may add (for lazy properties), or let them hit the db?
| @@ -85,10 +85,20 @@ public bool IsPuttable(long txTimestamp, object newVersion, IComparer comparator | |||
| { | |||
| return false; | |||
| } | |||
| return Version == null ? | |||
There was a problem hiding this comment.
UnlockTimestamp initial value (-1) is irrelevant until Multiplicity has dropped to 0, at which point UnlockTimestamp is updated with the unlocking timestamp and starts to be relevant: it avoids a concurrent "stale" transaction to put stale data in the cache.
The Timeout here is not actually a timeout, it is a TimeoutTimestamp: it has been computed from the locking timestamp to which the timeout has been added.
| @@ -24,11 +41,20 @@ namespace NHibernate.Cache | |||
| /// </remarks> | |||
| public partial class ReadWriteCache : IBatchableCacheConcurrencyStrategy | |||
There was a problem hiding this comment.
The summary comment states:
Repeatable read isolation is compromised in the case of concurrent writes.
Which means implementing correctly minimal put may cause more violations of repeatable read semantic.
But since it was not guaranteeing it anyway, and since not putting when minimal put is false causes bugs, bette implement it anyway I think.
Co-authored-by: Frédéric Delaporte <[email protected]>
| @@ -85,10 +85,20 @@ public bool IsPuttable(long txTimestamp, object newVersion, IComparer comparator | |||
| { | |||
| return false; | |||
| } | |||
| return Version == null ? | |||
There was a problem hiding this comment.
If the cached version is older than the one to be put yes. But with your change, it may be as old, in which case this is debattable.
Co-authored-by: Frédéric Delaporte <[email protected]>
| @@ -85,10 +85,20 @@ public bool IsPuttable(long txTimestamp, object newVersion, IComparer comparator | |||
| { | |||
| return false; | |||
| } | |||
| return Version == null ? | |||
There was a problem hiding this comment.
I am just meaning the same age: cached version as old as the one to put, (or as new as,) that is just a way to tell "same age". This case is normally useful only for adding data for lazy loaded properties which was not already loaded and cached.
If the version is strictly newer, we know for sure that the cached data is stale and so, there is no harm in updating it with newer data, even if the session putting data in cache is not timestamped after the unlock timestamp. (Better to have less stale data.)
If the version is the same while the unlock timestamp tell us the session putting in cache may be stale, is it better to let other sessions see the additional date the put may add (for lazy properties), or let them hit the db?
Fixes #3176