Skip to content

Commit

Permalink
GB+Tree tests run with adversarial page cache
Browse files Browse the repository at this point in the history
also included are fixes for issues exposed by this added testing:

- Removed Layout#read/writeMetaData because there's no need for it
  and it was implemented in the wrong way w/ regards to shouldRetry
- KeySearch#search doesn't throw exception on unexpected end result,
  instead uses one of the high bits to signal success/failure.
- GSPP#pointerState no longer throws exception on unexpected generation
  because it was done inside shouldRetry loop. Now instead reports
  BROKEN state.
- SeekCursor asserts shouldRetry is true after unexpected read,
  otherwise throws exception about tree inconsistency.
- Fixes a bug in AdversarialWritePageCursor#copyTo where this wasn't
  allowed previously, due to an instanceof check.
  • Loading branch information
tinwelint committed Jan 9, 2017
1 parent 2fa6c8b commit b13478e
Show file tree
Hide file tree
Showing 13 changed files with 249 additions and 96 deletions.
Expand Up @@ -39,6 +39,7 @@
import org.neo4j.index.IndexWriter; import org.neo4j.index.IndexWriter;
import org.neo4j.index.ValueMerger; import org.neo4j.index.ValueMerger;
import org.neo4j.index.ValueMergers; import org.neo4j.index.ValueMergers;
import org.neo4j.io.pagecache.CursorException;
import org.neo4j.io.pagecache.IOLimiter; import org.neo4j.io.pagecache.IOLimiter;
import org.neo4j.io.pagecache.PageCache; import org.neo4j.io.pagecache.PageCache;
import org.neo4j.io.pagecache.PageCursor; import org.neo4j.io.pagecache.PageCursor;
Expand Down Expand Up @@ -439,7 +440,15 @@ private void readMeta( File indexFile, Layout<KEY,VALUE> layout, PagedFile paged
} }
while ( metaCursor.shouldRetry() ); while ( metaCursor.shouldRetry() );
checkOutOfBounds( metaCursor ); checkOutOfBounds( metaCursor );
metaCursor.checkAndClearCursorException();
} }
catch ( CursorException e )
{
throw new MetadataMismatchException( format(
"Tried to open %s, but caught an error while reading meta data. " +
"File is expected to be corrupt, try to rebuild.", indexFile ), e );
}

if ( formatVersion != FORMAT_VERSION ) if ( formatVersion != FORMAT_VERSION )
{ {
throw new MetadataMismatchException( "Tried to open %s with a different format version than " + throw new MetadataMismatchException( "Tried to open %s with a different format version than " +
Expand Down
Expand Up @@ -97,23 +97,22 @@ private static void assertPointerOnWrite( long pointer )
} }
} }


public static long readGeneration( PageCursor cursor ) static long readGeneration( PageCursor cursor )
{ {
return getUnsignedInt( cursor ); return getUnsignedInt( cursor );
} }


public static long readPointer( PageCursor cursor ) static long readPointer( PageCursor cursor )
{ {
long result = get6BLong( cursor ); return get6BLong( cursor );
return result;
} }


public static short readChecksum( PageCursor cursor ) static short readChecksum( PageCursor cursor )
{ {
return cursor.getShort(); return cursor.getShort();
} }


public static boolean verifyChecksum( PageCursor cursor, long generation, long pointer ) static boolean verifyChecksum( PageCursor cursor, long generation, long pointer )
{ {
short checksum = cursor.getShort(); short checksum = cursor.getShort();
return checksum == checksumOf( generation, pointer ); return checksum == checksumOf( generation, pointer );
Expand Down
Expand Up @@ -354,8 +354,7 @@ static byte pointerState( long stableGeneration, long unstableGeneration,
} }
if ( generation < MIN_GENERATION ) if ( generation < MIN_GENERATION )
{ {
throw new TreeInconsistencyException( "Generation was less than MIN_GENERATION " + MIN_GENERATION + return BROKEN;
" but checksum was correct. Pointer was " + generation + "," + pointer );
} }
if ( generation <= stableGeneration ) if ( generation <= stableGeneration )
{ {
Expand Down
Expand Up @@ -28,7 +28,6 @@


import static org.neo4j.index.gbptree.KeySearch.isHit; import static org.neo4j.index.gbptree.KeySearch.isHit;
import static org.neo4j.index.gbptree.KeySearch.positionOf; import static org.neo4j.index.gbptree.KeySearch.positionOf;
import static org.neo4j.index.gbptree.KeySearch.search;
import static org.neo4j.index.gbptree.PageCursorUtil.goTo; import static org.neo4j.index.gbptree.PageCursorUtil.goTo;


/** /**
Expand Down Expand Up @@ -258,7 +257,7 @@ private void moveToCorrectLeaf( PageCursor cursor, KEY key, long stableGeneratio
{ {
// We still need to go down further, but we're on the right path // We still need to go down further, but we're on the right path
int keyCount = bTreeNode.keyCount( cursor ); int keyCount = bTreeNode.keyCount( cursor );
int searchResult = search( cursor, bTreeNode, key, readKey, keyCount ); int searchResult = search( cursor, key, readKey, keyCount );
int childPos = positionOf( searchResult ); int childPos = positionOf( searchResult );
if ( isHit( searchResult ) ) if ( isHit( searchResult ) )
{ {
Expand Down Expand Up @@ -373,6 +372,13 @@ void insert( PageCursor cursor, StructurePropagation<KEY> structurePropagation,
} }
} }


private int search( PageCursor cursor, KEY key, KEY readKey, int keyCount )
{
int searchResult = KeySearch.search( cursor, bTreeNode, key, readKey, keyCount );
KeySearch.assertSuccess( searchResult );
return searchResult;
}

/** /**
* Asserts that cursor is where it's expected to be at, compared to current level. * Asserts that cursor is where it's expected to be at, compared to current level.
* *
Expand Down Expand Up @@ -409,7 +415,7 @@ private void insertInInternal( PageCursor cursor, StructurePropagation<KEY> stru
if ( keyCount < bTreeNode.internalMaxKeyCount() ) if ( keyCount < bTreeNode.internalMaxKeyCount() )
{ {
// No overflow // No overflow
int pos = positionOf( search( cursor, bTreeNode, primKey, readKey, keyCount ) ); int pos = positionOf( search( cursor, primKey, readKey, keyCount ) );


bTreeNode.insertKeyAt( cursor, primKey, pos, keyCount ); bTreeNode.insertKeyAt( cursor, primKey, pos, keyCount );
// NOTE pos+1 since we never insert a new child before child(0) because its key is really // NOTE pos+1 since we never insert a new child before child(0) because its key is really
Expand Down Expand Up @@ -450,7 +456,7 @@ private void splitInternal( PageCursor cursor, StructurePropagation<KEY> structu
long newRight = idProvider.acquireNewId( stableGeneration, unstableGeneration ); long newRight = idProvider.acquireNewId( stableGeneration, unstableGeneration );


// Find position to insert new key // Find position to insert new key
int pos = positionOf( search( cursor, bTreeNode, primKey, readKey, keyCount ) ); int pos = positionOf( search( cursor, primKey, readKey, keyCount ) );


int keyCountAfterInsert = keyCount + 1; int keyCountAfterInsert = keyCount + 1;
int middlePos = middle( keyCountAfterInsert ); int middlePos = middle( keyCountAfterInsert );
Expand Down Expand Up @@ -598,7 +604,7 @@ private void insertInLeaf( PageCursor cursor, StructurePropagation<KEY> structur
long stableGeneration, long unstableGeneration ) throws IOException long stableGeneration, long unstableGeneration ) throws IOException
{ {
int keyCount = bTreeNode.keyCount( cursor ); int keyCount = bTreeNode.keyCount( cursor );
int search = search( cursor, bTreeNode, key, readKey, keyCount ); int search = search( cursor, key, readKey, keyCount );
int pos = positionOf( search ); int pos = positionOf( search );
if ( isHit( search ) ) if ( isHit( search ) )
{ {
Expand Down Expand Up @@ -704,7 +710,7 @@ private void splitLeaf( PageCursor cursor, StructurePropagation<KEY> structurePr
// 5. Write new key/values into L // 5. Write new key/values into L


// Position where newKey / newValue is to be inserted // Position where newKey / newValue is to be inserted
int pos = positionOf( search( cursor, bTreeNode, newKey, readKey, keyCount ) ); int pos = positionOf( search( cursor, newKey, readKey, keyCount ) );
int keyCountAfterInsert = keyCount + 1; int keyCountAfterInsert = keyCount + 1;
int middlePos = middle( keyCountAfterInsert ); int middlePos = middle( keyCountAfterInsert );


Expand Down Expand Up @@ -876,7 +882,7 @@ private boolean removeFromLeaf( PageCursor cursor, StructurePropagation<KEY> str
int keyCount = bTreeNode.keyCount( cursor ); int keyCount = bTreeNode.keyCount( cursor );


// No overflow, insert key and value // No overflow, insert key and value
int search = search( cursor, bTreeNode, key, readKey, keyCount ); int search = search( cursor, key, readKey, keyCount );
int pos = positionOf( search ); int pos = positionOf( search );
boolean hit = isHit( search ); boolean hit = isHit( search );
if ( !hit ) if ( !hit )
Expand Down
Expand Up @@ -28,8 +28,13 @@
*/ */
class KeySearch class KeySearch
{ {
private static final int POSITION_MASK = 0x7FFFFFFF; private static final int POSITION_MASK = 0x3FFFFFFF;
private static final int HIT_MASK = 0x80000000; private static final int HIT_FLAG = 0x80000000;
private static final int NO_HIT_FLAG = 0x00000000;
private static final int HIT_MASK = HIT_FLAG | NO_HIT_FLAG;
private static final int SUCCESS_FLAG = 0x00000000;
private static final int NO_SUCCESS_FLAG = 0x40000000;
private static final int SUCCESS_MASK = SUCCESS_FLAG | NO_SUCCESS_FLAG;


/** /**
* Search for left most pos such that keyAtPos obeys key <= keyAtPos. * Search for left most pos such that keyAtPos obeys key <= keyAtPos.
Expand Down Expand Up @@ -106,8 +111,7 @@ else if ( (comparison = comparator.compare( key, bTreeNode.keyAt( cursor, readKe
} }
if ( lower != higher ) if ( lower != higher )
{ {
throw new TreeInconsistencyException( "The binary search terminated in an unexpected way. " + return NO_SUCCESS_FLAG;
"Expected lower and higher to be equal but was " + "lower=" + lower + ", higher=" + higher );
} }
pos = lower; pos = lower;


Expand All @@ -118,7 +122,7 @@ else if ( (comparison = comparator.compare( key, bTreeNode.keyAt( cursor, readKe


private static int searchResult( int pos, boolean hit ) private static int searchResult( int pos, boolean hit )
{ {
return (pos & POSITION_MASK) | ((hit ? 1 : 0) << 31); return (pos & POSITION_MASK) | (hit ? HIT_FLAG : NO_HIT_FLAG);
} }


/** /**
Expand All @@ -141,6 +145,19 @@ static int positionOf( int searchResult )
*/ */
static boolean isHit( int searchResult ) static boolean isHit( int searchResult )
{ {
return (searchResult & HIT_MASK) != 0; return (searchResult & HIT_MASK) == HIT_FLAG;
}

static boolean isSuccess( int searchResult )
{
return (searchResult & SUCCESS_MASK) == SUCCESS_FLAG;
}

static void assertSuccess( int searchResult )
{
if ( !isSuccess( searchResult ) )
{
throw new TreeInconsistencyException( "Search terminated in unexpected way" );
}
} }
} }
Expand Up @@ -128,9 +128,10 @@ public interface Layout<KEY, VALUE> extends Comparator<KEY>
* Reads meta data specific to this layout instance from {@code cursor} at its current offset. * Reads meta data specific to this layout instance from {@code cursor} at its current offset.
* The read meta data must also be verified against meta data provided in constructor of this Layout. * The read meta data must also be verified against meta data provided in constructor of this Layout.
* Constructor-provided meta data can be {@code null} to skip this verification. * Constructor-provided meta data can be {@code null} to skip this verification.
* if read meta data doesn't match with the meta data provided in constructor
* {@link PageCursor#setCursorException(String)} should be called with appropriate error message.
* *
* @param cursor {@link PageCursor} to read from, at its current offset. * @param cursor {@link PageCursor} to read from, at its current offset.
* @throws MetadataMismatchException if read meta data doesn't match with the meta data provided in constructor.
*/ */
void readMetaData( PageCursor cursor ); void readMetaData( PageCursor cursor );


Expand Down
Expand Up @@ -228,9 +228,9 @@ class SeekCursor<KEY,VALUE> implements RawCursor<Hit<KEY,VALUE>,IOException>, Hi
private int pos; private int pos;


/** /**
* Number of keys in the current leaf, this value is cached and only re-read every time there's * Number of keys in the current leaf, this value is cached and only re-read every time there's
* a {@link PageCursor#shouldRetry() retry due to concurrent write}. * a {@link PageCursor#shouldRetry() retry due to concurrent write}.
*/ */
private int keyCount; private int keyCount;


/** /**
Expand Down Expand Up @@ -394,21 +394,32 @@ private void traverseDownToFirstLeaf() throws IOException
do do
{ {
// Read // Read
boolean fullRead;
int searchResult = 0;
do do
{ {
fullRead = false;

// Where we are // Where we are
if ( !readHeader() ) if ( !readHeader() )
{ {
continue; continue;
} }


// Where we're going searchResult = searchKey( fromInclusive );
pos = searchKey( fromInclusive ); if ( !KeySearch.isSuccess( searchResult ) )
{
continue;
}
pos = positionOf( searchResult );

if ( isInternal ) if ( isInternal )
{ {
pointerId = bTreeNode.childAt( cursor, pos, stableGeneration, unstableGeneration ); pointerId = bTreeNode.childAt( cursor, pos, stableGeneration, unstableGeneration );
pointerGen = readPointerGenOnSuccess( pointerId ); pointerGen = readPointerGenOnSuccess( pointerId );
} }

fullRead = true;
} }
while ( cursor.shouldRetry() ); while ( cursor.shouldRetry() );
checkOutOfBounds( cursor ); checkOutOfBounds( cursor );
Expand All @@ -421,6 +432,15 @@ private void traverseDownToFirstLeaf() throws IOException
continue; continue;
} }


if ( !fullRead )
{
throw new TreeInconsistencyException( "Read inconsistent tree node %d%n" +
" nodeType:%d%n currentNodeGen:%d%n newGen:%d%n newGenGen:%d%n isInternal:%b%n" +
" keyCount:%d%n maxKeyCount:%d%n searchResult:%d%n pos:%d%n childId:%d%n childIdGen:%d",
cursor.getCurrentPageId(), nodeType, currentNodeGen, newGen, newGenGen,
isInternal, keyCount, maxKeyCount, searchResult, pos, pointerId, pointerGen );
}

if ( goToNewGen() ) if ( goToNewGen() )
{ {
continue; continue;
Expand Down Expand Up @@ -449,9 +469,13 @@ public boolean next() throws IOException
while ( true ) while ( true )
{ {
pos += stride; pos += stride;
boolean fullRead;
int searchResult = 0;
// Read // Read
do do
{ {
fullRead = false;

// Where we are // Where we are
if ( !readHeader() ) if ( !readHeader() )
{ {
Expand All @@ -468,7 +492,13 @@ public boolean next() throws IOException
{ {
// Keys could have been moved so we need to make sure we are not missing any keys by // Keys could have been moved so we need to make sure we are not missing any keys by
// moving position back until we find previously returned key // moving position back until we find previously returned key
pos = searchKey( first ? fromInclusive : prevKey ); searchResult = searchKey( first ? fromInclusive : prevKey );
if ( !KeySearch.isSuccess( searchResult ) )
{
continue;
}
pos = positionOf( searchResult );

if ( !seekForward && pos >= keyCount ) if ( !seekForward && pos >= keyCount )
{ {
// We may need to go to previous sibling to find correct place to start seeking from // We may need to go to previous sibling to find correct place to start seeking from
Expand All @@ -490,6 +520,8 @@ public boolean next() throws IOException
bTreeNode.keyAt( cursor, mutableKey, pos ); bTreeNode.keyAt( cursor, mutableKey, pos );
bTreeNode.valueAt( cursor, mutableValue, pos ); bTreeNode.valueAt( cursor, mutableValue, pos );
} }

fullRead = true;
} }
while ( concurrentWriteHappened = cursor.shouldRetry() ); while ( concurrentWriteHappened = cursor.shouldRetry() );
checkOutOfBounds( cursor ); checkOutOfBounds( cursor );
Expand All @@ -503,6 +535,16 @@ public boolean next() throws IOException
continue; continue;
} }


if ( !fullRead )
{
throw new TreeInconsistencyException( "Read inconsistent tree node %d%n" +
" nodeType:%d%n currentNodeGen:%d%n newGen:%d%n newGenGen:%d%n" +
" keyCount:%d%n maxKeyCount:%d%n searchResult:%d%n pos:%d%n" +
" rightSibling:%d%n rightSiblingGen:%d",
cursor.getCurrentPageId(), nodeType, currentNodeGen, newGen, newGenGen,
keyCount, maxKeyCount, searchResult, pos, pointerId, pointerGen );
}

if ( !verifyFirstKeyInNodeIsExpectedAfterGoTo() ) if ( !verifyFirstKeyInNodeIsExpectedAfterGoTo() )
{ {
continue; continue;
Expand Down Expand Up @@ -673,11 +715,15 @@ private long readNextSibling()
*/ */
private int searchKey( KEY key ) private int searchKey( KEY key )
{ {
int search = KeySearch.search( cursor, bTreeNode, key, mutableKey, keyCount ); return KeySearch.search( cursor, bTreeNode, key, mutableKey, keyCount );
int pos = KeySearch.positionOf( search ); }

private int positionOf( int searchResult )
{
int pos = KeySearch.positionOf( searchResult );


// Assuming unique keys // Assuming unique keys
if ( isInternal && KeySearch.isHit( search ) ) if ( isInternal && KeySearch.isHit( searchResult ) )
{ {
pos++; pos++;
} }
Expand Down

0 comments on commit b13478e

Please sign in to comment.