[infinispan-dev] Doubts about TxDistributionInterceptor and possible break in transaction isolation

[Pedro Ruivo]

On 06/17/2013 12:56 PM, Mircea Markus wrote:
>
> On 17 Jun 2013, at 11:52, Pedro Ruivo <pedro at infinispan.org> wrote:
>
>> I've been looking at TxDistributionInterceptor and I have a couple of
>> questions (assuming REPEATABLE_READ isolation level):
>>
>> #1. why are we doing a remote get each time we write on a key? (huge
>> perform impact if the key was previously read)
> indeed this is suboptimal for transactions that write the same key repeatedly and repeatable read. Can you please create a JIRA for this?

created: https://issues.jboss.org/browse/ISPN-3235

>>
>> #2. why are we doing a dataContainer.get() if the remote get returns a
>> null value? Shouldn't the interactions with data container be performed
>> only in the (Versioned)EntryWrappingInterceptor?
> This was added in the scope of ISPN-2688 and covers the scenario in which a state transfer is in progress, the remote get returns null as the remote value was dropped (no longer owner) and this node has become the owner in between.
>

ok :)

>>
>> #3. (I didn't verify this) why are we acquire the lock is the remote get
>> is performed for a write? This looks correct for pessimistic locking but
>> not for optimistic...
> I think that, given that the local node is not owner, the lock acquisition is redundant even for pessimistic caches.
> Mind creating a test to check if dropping that lock acquisition doesn't break things?

I created a JIRA with low priority since it does not affect the 
transaction outcome/isolation and I believe the performance impact 
should be lower (you can increase the priority if you want).

https://issues.jboss.org/browse/ISPN-3237
>>
>> After this analysis, it is possible to break the isolation between
>> transaction if I do a get on the key that does not exist:
>>
>> tm.begin()
>> cache.get(k) //returns null
>> //in the meanwhile a transaction writes on k and commits
>> cache.get(k) //return the new value. IMO, this is not valid for
>> REPEATABLE_READ isolation level!
>
> Indeed sounds like a bug, well spotted.
> Can you please add a UT to confirm it and raise a JIRA?

created: https://issues.jboss.org/browse/ISPN-3236

IMO, this should be the correct behaviour (I'm going to add the test 
cases later):

tm.begin()
cache.get(k) //returns null (op#1)
//in the meanwhile a transaction writes on k and commits
write operation performed:
* put: must return the same value as op#1
* conditional put //if op#1 returns null the operation should be always 
successful (i.e. the key is updated, return true). Otherwise, the key 
remains unchanged (return false)
* replace: must return the same value as op#1
* conditional replace: replace should be successful if checked with the 
op#1 return value (return true). Otherwise, the key must remain 
unchanged (return false).
* remote: must return the same value as op#1
* conditional remove: the key should be removed if checked with the op#1 
return value (return true). Otherwise, the key must remain unchanged 
(return false)

Also, the description above should be valid after a removal of a key.

>
> Cheers,
>