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

Jump to bottom

DB-8936 Fix potential gaps in sequence generation #5677

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

DB-8936 Fix potential gaps in sequence generation #5677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5005cc7
DB-8936 Fix potential gaps in sequence generation
ascend1 Jul 13, 2021
8b25478
DB-8936 Fix SpotBugs
ascend1 Jul 15, 2021
fc9e98e
Merge branch 'master' into DB-8936
ascend1 Jul 21, 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to get more understanding on why we can't simply upgrade a read lock to write lock, i.e. why the readlock().unlock() is necessary, I came across this SO thread which made me worried this might block forever? Did you double-check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not possible to directly upgrading a read-write lock without unlocking the read lock first. I think this is common behavior not only in Java but kind of any read-write lock implementations. The problem is that multiple threads could try to upgrade simultaneously, making the scenario very complex to be correct. Downgrading is simple because the current thread knows it's the only thread doing so.

For the behavior described in the SO thread, I wouldn't worry too much until we actually see it. It looks like a bug in Java that happens rarely. If this problem happens very often, ReentrantReadWriteLock implementation is basically buggy and unusable, which doesn't see to be the case in practice.

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