diff --git a/splice_machine/src/main/java/com/splicemachine/derby/impl/sql/execute/sequence/AbstractSequence.java b/splice_machine/src/main/java/com/splicemachine/derby/impl/sql/execute/sequence/AbstractSequence.java index 543c2516073..5c70857d0fc 100644 --- a/splice_machine/src/main/java/com/splicemachine/derby/impl/sql/execute/sequence/AbstractSequence.java +++ b/splice_machine/src/main/java/com/splicemachine/derby/impl/sql/execute/sequence/AbstractSequence.java @@ -23,16 +23,14 @@ import java.io.ObjectOutput; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; public abstract class AbstractSequence implements Sequence, Externalizable{ protected final AtomicLong remaining=new AtomicLong(0l); protected final AtomicLong currPosition=new AtomicLong(0l); protected long blockAllocationSize; protected long incrementSteps; - protected final ReadWriteLock rwLock = new ReentrantReadWriteLock(); + protected final Lock updateLock=new ReentrantLock(); protected long startingValue; public AbstractSequence(){ @@ -48,27 +46,15 @@ public AbstractSequence(long blockAllocationSize,long incrementSteps,long starti } public long getNext() throws StandardException{ - rwLock.readLock().lock(); - try { - if (remaining.getAndDecrement() <= 0) { - allocateBlock(false); - } - return currPosition.getAndAdd(incrementSteps); - } finally { - rwLock.readLock().unlock(); - } + if(remaining.getAndDecrement()<=0) + allocateBlock(false); + return currPosition.getAndAdd(incrementSteps); } public long peekAtCurrentValue() throws StandardException { - rwLock.readLock().lock(); - try { - if (remaining.get() <= 0) { - allocateBlock(true); - } - return currPosition.get(); - } finally { - rwLock.readLock().unlock(); - } + if(remaining.get()<= 0) + allocateBlock(true); + return currPosition.get(); } protected abstract long getCurrentValue() throws IOException; @@ -77,16 +63,12 @@ public long peekAtCurrentValue() throws StandardException { public abstract void close() throws IOException; - // must be called with acquired read lock private void allocateBlock(boolean peek) throws StandardException{ boolean success=false; long absIncrement = incrementSteps < 0 ? -incrementSteps : incrementSteps; while(!success){ - if(remaining.getAndDecrement()>0) - return; - rwLock.readLock().unlock(); - rwLock.writeLock().lock(); + updateLock.lock(); try{ if(remaining.getAndDecrement()>0) return; @@ -104,9 +86,7 @@ private void allocateBlock(boolean peek) throws StandardException{ }catch(IOException e){ throw Exceptions.parseException(e); }finally{ - // downgrade to read lock - rwLock.readLock().lock(); - rwLock.writeLock().unlock(); + updateLock.unlock(); } } } diff --git a/splice_machine/src/test/java/com/splicemachine/derby/impl/sql/execute/operations/GeneratedColumnIT.java b/splice_machine/src/test/java/com/splicemachine/derby/impl/sql/execute/operations/GeneratedColumnIT.java index 71831bcec49..23cc0f715ef 100644 --- a/splice_machine/src/test/java/com/splicemachine/derby/impl/sql/execute/operations/GeneratedColumnIT.java +++ b/splice_machine/src/test/java/com/splicemachine/derby/impl/sql/execute/operations/GeneratedColumnIT.java @@ -41,7 +41,10 @@ import java.util.List; import java.util.Set; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * Test to validate that GENERATED columns work correctly. @@ -443,16 +446,7 @@ public void testMultiRowInsert() throws Exception { for(int i=0;i<13;i++){ rowCount+=s.executeUpdate(String.format("insert into %s(c2) values (1),(2),(3),(4),(5),(6),(7),(8),(9),(10)" ,tableRef)); - // DB-8936 - // Hint the insert statement to run on OLTP always to avoid a gap in sequence. This is fine - // at the moment because the IT seems to run on a single region server with only one thread - // generating values for column c1. - // In future, if this is not true anymore, i.e., multiple region servers can insert values - // for the same insert statement, the gap is bound to happen based on current implementation. - // To make the test green again, change the assertion 'lastValue + 1 == next' to - // 'lastValue < next'. - // Check comment in OperationConfiguration.java for how sequence is implemented currently. - rowCount+=s.executeUpdate(String.format("insert into %s(c2) select c1 from %s --splice-properties useSpark=false",tableRef,tableRef)); + rowCount+=s.executeUpdate(String.format("insert into %s(c2) select c1 from %s",tableRef,tableRef)); } @@ -463,11 +457,10 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - // ensure sequence is sorted (ORDER BY clause) and has no gap (next == lastValue + 1) - assertEquals("Bad sequence returned", lastValue + 1, next); + assertTrue("Returned data out of order!",next==lastValue+1); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } - assertEquals("Incorrect returned row count",rowCount,count); + assertEquals("Incorrect returned row count!",rowCount,count); } try(ResultSet rs=s.executeQuery(String.format("SELECT max(c1) FROM %s",tableRef))){ assertTrue("No rows returned!",rs.next()); @@ -495,7 +488,7 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertEquals("Bad sequence returned", lastValue + 3000, next); //ensure sequence is sorted (ORDER BY clause) + assertTrue("Returned data out of order!",next==lastValue+3000); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } assertEquals("Incorrect returned row count!",rowCount,count); @@ -523,7 +516,7 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertEquals("Bad sequence returned", lastValue - 9999, next); //ensure sequence is sorted (ORDER BY clause) + assertTrue("Returned data out of order!",next==lastValue-9999); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } assertEquals("Incorrect returned row count!",rowCount,count); @@ -550,7 +543,7 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertEquals("Bad sequence returned", lastValue - 10000, next); //ensure sequence is sorted (ORDER BY clause) + assertTrue("Returned data out of order!",next==lastValue-10000); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } assertEquals("Incorrect returned row count!",rowCount,count); @@ -577,7 +570,7 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertEquals("Bad sequence returned", lastValue - 23000, next); //ensure sequence is sorted (ORDER BY clause) + assertTrue("Returned data out of order!",next==lastValue-23000); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } assertEquals("Incorrect returned row count!",rowCount,count);