From 236140814058622d75fa272dbdaee623682510fe Mon Sep 17 00:00:00 2001 From: Zhen Li Date: Tue, 13 Jul 2021 18:10:28 +0200 Subject: [PATCH 1/2] DB-8936 Fix potential gaps in sequence generation --- .../execute/sequence/AbstractSequence.java | 34 +++++++++++++++---- .../execute/operations/GeneratedColumnIT.java | 29 ++++++++++------ 2 files changed, 45 insertions(+), 18 deletions(-) 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 5c70857d0fc..85e89b9d381 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,14 +23,16 @@ 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 Lock updateLock=new ReentrantLock(); + protected final ReadWriteLock rwLock = new ReentrantReadWriteLock(); protected long startingValue; public AbstractSequence(){ @@ -46,15 +48,27 @@ public AbstractSequence(long blockAllocationSize,long incrementSteps,long starti } public long getNext() throws StandardException{ - if(remaining.getAndDecrement()<=0) + rwLock.readLock().lock(); + if (remaining.getAndDecrement() <= 0) { allocateBlock(false); - return currPosition.getAndAdd(incrementSteps); + } + try { + return currPosition.getAndAdd(incrementSteps); + } finally { + rwLock.readLock().unlock(); + } } public long peekAtCurrentValue() throws StandardException { - if(remaining.get()<= 0) + rwLock.readLock().lock(); + if (remaining.get() <= 0) { allocateBlock(true); - return currPosition.get(); + } + try { + return currPosition.get(); + } finally { + rwLock.readLock().unlock(); + } } protected abstract long getCurrentValue() throws IOException; @@ -63,12 +77,16 @@ 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){ - updateLock.lock(); + if(remaining.getAndDecrement()>0) + return; + rwLock.readLock().unlock(); + rwLock.writeLock().lock(); try{ if(remaining.getAndDecrement()>0) return; @@ -86,7 +104,9 @@ private void allocateBlock(boolean peek) throws StandardException{ }catch(IOException e){ throw Exceptions.parseException(e); }finally{ - updateLock.unlock(); + // downgrade to read lock + rwLock.readLock().lock(); + rwLock.writeLock().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 23cc0f715ef..71831bcec49 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,10 +41,7 @@ import java.util.List; import java.util.Set; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import static org.junit.Assert.*; /** * Test to validate that GENERATED columns work correctly. @@ -446,7 +443,16 @@ 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)); - rowCount+=s.executeUpdate(String.format("insert into %s(c2) select c1 from %s",tableRef,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)); } @@ -457,10 +463,11 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertTrue("Returned data out of order!",next==lastValue+1); //ensure sequence is sorted (ORDER BY clause) + // ensure sequence is sorted (ORDER BY clause) and has no gap (next == lastValue + 1) + assertEquals("Bad sequence returned", lastValue + 1, next); 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()); @@ -488,7 +495,7 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertTrue("Returned data out of order!",next==lastValue+3000); //ensure sequence is sorted (ORDER BY clause) + assertEquals("Bad sequence returned", lastValue + 3000, next); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } assertEquals("Incorrect returned row count!",rowCount,count); @@ -516,7 +523,7 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertTrue("Returned data out of order!",next==lastValue-9999); //ensure sequence is sorted (ORDER BY clause) + assertEquals("Bad sequence returned", lastValue - 9999, next); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } assertEquals("Incorrect returned row count!",rowCount,count); @@ -543,7 +550,7 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertTrue("Returned data out of order!",next==lastValue-10000); //ensure sequence is sorted (ORDER BY clause) + assertEquals("Bad sequence returned", lastValue - 10000, next); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } assertEquals("Incorrect returned row count!",rowCount,count); @@ -570,7 +577,7 @@ public void testMultiRowInsert() throws Exception { count++; int next=rs.getInt(1); Assert.assertFalse("Returned null!",rs.wasNull()); //ensure sequence is never null - assertTrue("Returned data out of order!",next==lastValue-23000); //ensure sequence is sorted (ORDER BY clause) + assertEquals("Bad sequence returned", lastValue - 23000, next); //ensure sequence is sorted (ORDER BY clause) lastValue=next; } assertEquals("Incorrect returned row count!",rowCount,count); From a95da90a24dc5b4fd44980a5e7d94a5b8089b847 Mon Sep 17 00:00:00 2001 From: Zhen Li Date: Thu, 15 Jul 2021 23:32:33 +0200 Subject: [PATCH 2/2] DB-8936 Fix SpotBugs --- .../impl/sql/execute/sequence/AbstractSequence.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 85e89b9d381..543c2516073 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 @@ -49,10 +49,10 @@ public AbstractSequence(long blockAllocationSize,long incrementSteps,long starti public long getNext() throws StandardException{ rwLock.readLock().lock(); - if (remaining.getAndDecrement() <= 0) { - allocateBlock(false); - } try { + if (remaining.getAndDecrement() <= 0) { + allocateBlock(false); + } return currPosition.getAndAdd(incrementSteps); } finally { rwLock.readLock().unlock(); @@ -61,10 +61,10 @@ public long getNext() throws StandardException{ public long peekAtCurrentValue() throws StandardException { rwLock.readLock().lock(); - if (remaining.get() <= 0) { - allocateBlock(true); - } try { + if (remaining.get() <= 0) { + allocateBlock(true); + } return currPosition.get(); } finally { rwLock.readLock().unlock();