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.

Sign up for GitHub

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

DB-8936 Fix potential gaps in sequence generation (3.0) #5679

Open
wants to merge 2 commits into
base: branch-3.0
Choose a base branch
from
Open

DB-8936 Fix potential gaps in sequence generation (3.0) #5679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b126ea1
DB-8936 Fix potential gaps in sequence generation
ascend1 Jul 13, 2021
2f376b2
DB-8936 Fix SpotBugs
ascend1 Jul 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions ...ine/src/main/java/com/splicemachine/derby/impl/sql/execute/sequence/AbstractSequence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(){
Expand All @@ -46,15 +48,27 @@ public AbstractSequence(long blockAllocationSize,long incrementSteps,long starti
}

public long getNext() throws StandardException{
if(remaining.getAndDecrement()<=0)
allocateBlock(false);
return currPosition.getAndAdd(incrementSteps);
rwLock.readLock().lock();
try {
if (remaining.getAndDecrement() <= 0) {
allocateBlock(false);
}
return currPosition.getAndAdd(incrementSteps);
} finally {
rwLock.readLock().unlock();
}
}

public long peekAtCurrentValue() throws StandardException {
if(remaining.get()<= 0)
allocateBlock(true);
return currPosition.get();
rwLock.readLock().lock();
try {
if (remaining.get() <= 0) {
allocateBlock(true);
}
return currPosition.get();
} finally {
rwLock.readLock().unlock();
}
}

protected abstract long getCurrentValue() throws IOException;
Expand All @@ -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;
Expand All @@ -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();
}
}
}
Expand Down
29 changes: 18 additions & 11 deletions .../src/test/java/com/splicemachine/derby/impl/sql/execute/operations/GeneratedColumnIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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));
}


Expand All @@ -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());
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down