Small improvements

neo4j · Jan 9, 2017 · 7ca0ab5 · 7ca0ab5
1 parent 4cbe20e
commit 7ca0ab5
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 11 deletions.
diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/CommonAbstractStore.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/CommonAbstractStore.java
@@ -1018,15 +1018,10 @@ public RECORD newRecord()
     @Override
     public RECORD getRecord( long id, RECORD record, RecordLoad mode )
     {
-        // Mark the record with this id regardless of whether or not we load the contents of it.
-        // This is done in this method since there are multiple call sites and they all want the id
-        // on that record, so it's to ensure it isn't forgotten.
-        record.setId( id );
         long pageId = pageIdForRecord( id );
-        int offset = offsetForId( id );
         try ( PageCursor cursor = storeFile.io( pageId, PF_SHARED_READ_LOCK ) )
         {
-            readIntoRecord( id, record, mode, pageId, offset, cursor );
+            readIntoRecord( id, record, mode, pageId, cursor );
             return record;
         }
         catch ( IOException e )
@@ -1035,11 +1030,16 @@ public RECORD getRecord( long id, RECORD record, RecordLoad mode )
         }
     }
 
-    private void readIntoRecord( long id, RECORD record, RecordLoad mode, long pageId, int offset, PageCursor cursor )
+    private void readIntoRecord( long id, RECORD record, RecordLoad mode, long pageId, PageCursor cursor )
             throws IOException
     {
+        // Mark the record with this id regardless of whether or not we load the contents of it.
+        // This is done in this method since there are multiple call sites and they all want the id
+        // on that record, so it's to ensure it isn't forgotten.
+        record.setId( id );
         if ( cursor.next( pageId ) )
         {
+            int offset = offsetForId( id );
             // There is a page in the store that covers this record, go read it
             do
             {
@@ -1171,10 +1171,7 @@ public boolean next( long id, RECORD record, RecordLoad mode )
 
                 try
                 {
-                    record.setId( id );
-                    long pageId = pageIdForRecord( id );
-                    int offset = offsetForId( id );
-                    readIntoRecord( currentId, record, mode, pageId, offset, pageCursor );
+                    readIntoRecord( id, record, mode, pageIdForRecord( id ), pageCursor );
                     return record.inUse();
                 }
                 catch ( IOException e )

diff --git a/...ty/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java b/...ty/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java
@@ -47,6 +47,7 @@
 import org.neo4j.test.rule.ConfigurablePageCacheRule;
 import org.neo4j.test.rule.fs.EphemeralFileSystemRule;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.neo4j.helpers.collection.MapUtil.stringMap;
@@ -258,6 +259,60 @@ public void pageCursorErrorsMustNotLingerInRecordCursor() throws Exception
         assertTrue( cursor.next( 2, new IntRecord( 2 ), NORMAL ) );
     }
 
+    @Test
+    public void shouldReadTheCorrectRecordWhenGivenAnExplicitIdAndNotUseTheCurrentIdPointer() throws Exception
+    {
+        createStore();
+        IntRecord record42 = new IntRecord( 42 );
+        record42.value = 0x42;
+        store.updateRecord( record42 );
+        IntRecord record43 = new IntRecord( 43 );
+        record43.value = 0x43;
+        store.updateRecord( record43 );
+
+        RecordCursor<IntRecord> cursor = store.newRecordCursor( new IntRecord( 1 ) ).acquire( 42, FORCE );
+
+        // we need to read record 43 not 42!
+        assertTrue( cursor.next( 43 ) );
+        assertEquals( record43, cursor.get() );
+
+        IntRecord record = new IntRecord( -1 );
+        assertTrue( cursor.next( 43, record, NORMAL ) );
+        assertEquals( record43, record );
+
+        // next with id does not affect the old pointer either, so 42 is read now
+        assertTrue( cursor.next() );
+        assertEquals( record42, cursor.get() );
+    }
+
+    @Test
+    public void shouldJumpAroundPageIds() throws Exception
+    {
+        createStore();
+        IntRecord record42 = new IntRecord( 42 );
+        record42.value = 0x42;
+        store.updateRecord( record42 );
+
+        int idOnAnotherPage = 43 + (2 * store.getRecordsPerPage() );
+        IntRecord record43 = new IntRecord( idOnAnotherPage );
+        record43.value = 0x43;
+        store.updateRecord( record43 );
+
+        RecordCursor<IntRecord> cursor = store.newRecordCursor( new IntRecord( 1 ) ).acquire( 42, FORCE );
+
+        // we need to read record 43 not 42!
+        assertTrue( cursor.next( idOnAnotherPage ) );
+        assertEquals( record43, cursor.get() );
+
+        IntRecord record = new IntRecord( -1 );
+        assertTrue( cursor.next( idOnAnotherPage, record, NORMAL ) );
+        assertEquals( record43, record );
+
+        // next with id does not affect the old pointer either, so 42 is read now
+        assertTrue( cursor.next() );
+        assertEquals( record42, cursor.get() );
+    }
+
     @Test
     public void rebuildIdGeneratorSlowMustThrowOnPageOverflow() throws Exception
     {