Skip to content

Commit

Permalink
Fixes a read mis-interpretation in SeekCursor part 2
Browse files Browse the repository at this point in the history 
now there's a proper distinction between landing on an unexpected tree node,
where restarting from root is the remedy. That, separated from recognizing
an inconsistent tree node, where a TreeInconsistencyException is thrown
  • Loading branch information
@tinwelint
tinwelint committed Jan 11, 2017
1 parent 572e03c commit b4343ee
Showing 1 changed file with 22 additions and 40 deletions.
62 changes: 22 additions & 40 deletions community/index/src/main/java/org/neo4j/index/gbptree/SeekCursor.java
View file
Expand Up @@ -29,7 +29,6 @@

import static java.lang.Integer.max;
import static org.neo4j.index.gbptree.PageCursorUtil.checkOutOfBounds;
import static org.neo4j.index.gbptree.TreeNode.NODE_TYPE_TREE_NODE;

/**
* {@link RawCursor} over tree leaves, making keys/values accessible to user. Given a starting leaf
Expand Down Expand Up @@ -312,6 +311,11 @@ class SeekCursor<KEY,VALUE> implements RawCursor<Hit<KEY,VALUE>,IOException>, Hi
*/
private long pointerGen;

/**
* Result from {@link KeySearch#search(PageCursor, TreeNode, Object, Object, int)}.
*/
private int searchResult;

// ┌── Special variables for backwards seek ──┐
// v v

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

// Where we are
if ( !readHeader() )
{
Expand All @@ -418,21 +418,18 @@ private void traverseDownToFirstLeaf() throws IOException
pointerId = bTreeNode.childAt( cursor, pos, stableGeneration, unstableGeneration );
pointerGen = readPointerGenOnSuccess( pointerId );
}

fullRead = true;
}
while ( cursor.shouldRetry() );
checkOutOfBounds( cursor );

// Act
if ( notSaneRead() && fullRead )
if ( !endedUpOnExpectedNode() )
{
prepareToStartFromRoot();
isInternal = true;
continue;
}

if ( !fullRead )
else if ( !saneRead() )
{
throw new TreeInconsistencyException( "Read inconsistent tree node %d%n" +
" nodeType:%d%n currentNodeGen:%d%n newGen:%d%n newGenGen:%d%n isInternal:%b%n" +
Expand Down Expand Up @@ -469,13 +466,9 @@ public boolean next() throws IOException
while ( true )
{
pos += stride;
boolean fullRead;
int searchResult = 0;
// Read
do
{
fullRead = false;

// Where we are
if ( !readHeader() )
{
Expand Down Expand Up @@ -520,22 +513,19 @@ public boolean next() throws IOException
bTreeNode.keyAt( cursor, mutableKey, pos );
bTreeNode.valueAt( cursor, mutableValue, pos );
}

fullRead = true;
}
while ( concurrentWriteHappened = cursor.shouldRetry() );
checkOutOfBounds( cursor );

// Act
if ( notSaneRead() && fullRead )
if ( !endedUpOnExpectedNode() )
{
// This node has been reused for something else than a tree node. Restart seek from root.
prepareToStartFromRoot();
traverseDownToFirstLeaf();
continue;
}

if ( !fullRead )
else if ( !saneRead() )
{
throw new TreeInconsistencyException( "Read inconsistent tree node %d%n" +
" nodeType:%d%n currentNodeGen:%d%n newGen:%d%n newGenGen:%d%n" +
Expand Down Expand Up @@ -751,25 +741,16 @@ private boolean readHeader()
{
newGenGen = bTreeNode.pointerGen( cursor, newGen );
}
isInternal = bTreeNode.isInternal( cursor );
isInternal = TreeNode.isInternal( cursor );
// Find the left-most key within from-range
keyCount = bTreeNode.keyCount( cursor );

return keyCountIsSane( keyCount );
}

/**
* {@link TreeNode#keyCount(PageCursor) keyCount} is the only value read inside a do-shouldRetry loop
* which is used as data fed into another read. Because of that extra assertions are made around
* keyCount, both inside do-shouldRetry (requesting one more round in the loop) and outside
* (calling this method, which may throw exception).
*
* @param keyCount key count read from {@link TreeNode#keyCount(PageCursor)} and has "survived"
* a do-shouldRetry loop.
*/
private boolean saneKeyCountRead( int keyCount )
private boolean endedUpOnExpectedNode()
{
return keyCount <= maxKeyCount;
return nodeType == TreeNode.NODE_TYPE_TREE_NODE && verifyNodeGenInvariants();
}

/**
Expand Down Expand Up @@ -934,6 +915,11 @@ else if ( !first && !insidePrevKey() )
}

/**
* {@link TreeNode#keyCount(PageCursor) keyCount} is the only value read inside a do-shouldRetry loop
* which is used as data fed into another read. Because of that extra assertions are made around
* keyCount, both inside do-shouldRetry (requesting one more round in the loop) and outside
* (calling this method, which may throw exception).
*
* @param keyCount key count of a tree node.
* @return {@code true} if key count is sane, i.e. positive and within max expected key count on a tree node.
*/
Expand All @@ -944,6 +930,11 @@ private boolean keyCountIsSane( int keyCount )
return keyCount >= 0 && keyCount <= maxKeyCount;
}

private boolean saneRead()
{
return keyCountIsSane( keyCount ) && KeySearch.isSuccess( searchResult );
}

/**
* Perform a generation catchup, updates current root and update range to start from
* previously returned key. Should be followed by a call to {@link #traverseDownToFirstLeaf()}
Expand Down Expand Up @@ -1041,15 +1032,6 @@ private boolean generationCatchup()
return false;
}

/**
* @return {@code true} if read is sane and can be trusted, otherwise {@code false} meaning that
* seek should be restarted from root.
*/
private boolean notSaneRead()
{
return nodeType != NODE_TYPE_TREE_NODE || !saneKeyCountRead( keyCount ) || !verifyNodeGenInvariants();
}

@Override
public KEY key()
{
Expand Down

0 comments on commit b4343ee

Please sign in to comment.