From 78611dd482e12aa243f2692a876943da5e58e53a Mon Sep 17 00:00:00 2001 From: Chris Vest Date: Wed, 16 Mar 2016 11:12:17 +0100 Subject: [PATCH] Fix rare inconsistent read problems surfaced by the more devious PageCacheRule --- .../org/neo4j/io/pagecache/PageCursor.java | 16 + .../impl/muninn/MuninnPageCursor.java | 1 + .../impl/muninn/MuninnReadPageCursor.java | 5 +- .../impl/muninn/MuninnWritePageCursor.java | 4 + .../pagecache/AdversarialReadPageCursor.java | 226 +++++++--- .../org/neo4j/io/pagecache/PageCacheTest.java | 103 +++++ community/kernel/pom.xml | 2 +- .../api/store/StorePropertyPayloadCursor.java | 3 +- .../impl/store/AbstractDynamicStore.java | 4 +- .../kernel/impl/store/LongerShortString.java | 5 + .../kernel/impl/store/PropertyStore.java | 7 + .../neo4j/kernel/impl/store/PropertyType.java | 34 +- .../format/standard/PropertyRecordFormat.java | 4 +- .../impl/store/record/DynamicRecord.java | 2 +- .../impl/store/record/PropertyBlock.java | 3 +- .../impl/store/record/PropertyRecord.java | 17 +- .../command/PhysicalLogCommandReaderV2_0.java | 4 +- .../command/PhysicalLogCommandReaderV2_1.java | 4 +- .../command/PhysicalLogCommandReaderV2_2.java | 4 +- .../PhysicalLogCommandReaderV2_2_4.java | 4 +- .../command/PhysicalLogCommandReaderV3_0.java | 5 +- .../impl/store/TestIdGeneratorRebuilding.java | 2 +- .../RecordBoundaryCheckingPagedFile.java | 399 ------------------ .../impl/store/format/RecordFormatTest.java | 28 +- .../highlimit/BaseHighLimitRecordFormat.java | 18 +- .../format/highlimit/DynamicRecordFormat.java | 18 +- .../format/highlimit/NodeRecordFormat.java | 6 +- .../RelationshipGroupRecordFormat.java | 10 +- .../highlimit/RelationshipRecordFormat.java | 16 +- 29 files changed, 408 insertions(+), 546 deletions(-) delete mode 100644 community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordBoundaryCheckingPagedFile.java diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java index 92a89d63d69c..4aebf03de697 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java @@ -249,6 +249,14 @@ public abstract class PageCursor implements AutoCloseable * ready to be processed. Returns false if there are no more pages to be * processed. For instance, if the cursor was requested with PF_NO_GROW * and the page most recently processed was the last page in the file. + *

+ * NOTE: When using read locks, read operations can be inconsistent + * and may return completely random data. The data returned from a + * read-locked page cursor should not be interpreted until after + * {@link #shouldRetry()} has returned {@code false}. + * Not interpreting the data also implies that you cannot throw exceptions + * from data validation errors until after {@link #shouldRetry()} has told + * you that your read was consistent. */ public abstract boolean next() throws IOException; @@ -257,6 +265,14 @@ public abstract class PageCursor implements AutoCloseable * and returns true when it is ready to be processed. Returns false if * for instance, the cursor was requested with PF_NO_GROW and the page * most recently processed was the last page in the file. + *

+ * NOTE: When using read locks, read operations can be inconsistent + * and may return completely random data. The data returned from a + * read-locked page cursor should not be interpreted until after + * {@link #shouldRetry()} has returned {@code false}. + * Not interpreting the data also implies that you cannot throw exceptions + * from data validation errors until after {@link #shouldRetry()} has told + * you that your read was consistent. */ public abstract boolean next( long pageId ) throws IOException; diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java index 10bea27fb815..b7546fb2ef9c 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java @@ -158,6 +158,7 @@ void clearPageState() pointer = victimPage; // make all future page access go to the victim page pageSize = 0; // make all future bound checks fail page = null; // make all future page navigation fail + currentPageId = UNBOUND_PAGE_ID; } @Override diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java index b2f0fd39c697..702df1b46fba 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java @@ -53,7 +53,7 @@ public boolean next() throws IOException { unpinCurrentPage(); long lastPageId = assertPagedFileStillMappedAndGetIdOfLastPage(); - if ( nextPageId > lastPageId ) + if ( nextPageId > lastPageId | nextPageId < 0 ) { return false; } @@ -97,7 +97,8 @@ protected void releaseCursor() @Override public boolean shouldRetry() throws IOException { - boolean needsRetry = !page.validateReadLock( lockStamp ); + MuninnPage p = page; + boolean needsRetry = p != null && !p.validateReadLock( lockStamp ); if ( needsRetry ) { startRetry(); diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java index af6db1d03c26..96060efb96eb 100644 --- a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java +++ b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java @@ -54,6 +54,10 @@ public boolean next() throws IOException { unpinCurrentPage(); long lastPageId = assertPagedFileStillMappedAndGetIdOfLastPage(); + if ( nextPageId < 0 ) + { + return false; + } if ( nextPageId > lastPageId ) { if ( (pf_flags & PagedFile.PF_NO_GROW) != 0 ) diff --git a/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialReadPageCursor.java b/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialReadPageCursor.java index 99341a90731e..3cc3a82e2bfd 100644 --- a/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialReadPageCursor.java +++ b/community/io/src/test/java/org/neo4j/adversaries/pagecache/AdversarialReadPageCursor.java @@ -22,6 +22,8 @@ import java.io.File; import java.io.FileNotFoundException; import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -41,25 +43,130 @@ * Depending on the adversary each read operation can produce an inconsistent read and require caller to retry using * while loop with {@link PageCursor#shouldRetry()} as a condition. *

+ * Inconsistent reads are injected by first having a retry-round (the set of operations on the cursor up until the + * {@link #shouldRetry()} call) that counts the number of operations performed on the cursor, and otherwise delegates + * the read operations to the real page cursor without corrupting them. Then the {@code shouldRetry} will choose a + * random operation, and from that point on in the next retry-round, all read operations will return random data. The + * {@code shouldRetry} method returns {@code true} for "yes, you should retry" and the round with the actual read + * inconsistencies begins. After that round, the client will be told to retry again, and in this third round there will + * be no inconsistencies, and there will be no need to retry unless the real page cursor says so. + *

* Write operations will always throw an {@link IllegalStateException} because this is a read cursor. * See {@link org.neo4j.io.pagecache.PagedFile#PF_SHARED_READ_LOCK} flag. */ @SuppressWarnings( "unchecked" ) class AdversarialReadPageCursor extends PageCursor { - private final PageCursor delegate; - private final Adversary adversary; - private PageCursor linkedCursor; + private static class State implements Adversary + { + private final Adversary adversary; + + private boolean currentReadIsPreparingInconsistent; + private boolean currentReadIsInconsistent; + private int callCounter; + + // This field for meant to be inspected with the debugger. + @SuppressWarnings( "MismatchedQueryAndUpdateOfCollection" ) + private List inconsistentReadHistory; + + private State( Adversary adversary ) + { + this.adversary = adversary; + inconsistentReadHistory = new ArrayList<>(); + } + + private Number inconsistently( T value, PageCursor delegate ) + { + if ( currentReadIsPreparingInconsistent ) + { + callCounter++; + return value; + } + if ( currentReadIsInconsistent && (--callCounter) <= 0 ) + { + ThreadLocalRandom rng = ThreadLocalRandom.current(); + long x = value.longValue(); + if ( x != 0 & rng.nextBoolean() ) + { + x = ~x; + } + else + { + x = rng.nextLong(); + } + inconsistentReadHistory.add( new NumberValue( value.getClass(), x, delegate.getOffset(), value ) ); + return x; + } + return value; + } + + private void inconsistently( byte[] data ) + { + if ( currentReadIsPreparingInconsistent ) + { + callCounter++; + } + else if ( currentReadIsInconsistent ) + { + ThreadLocalRandom.current().nextBytes( data ); + inconsistentReadHistory.add( Arrays.copyOf( data, data.length ) ); + } + } + + private void reset( boolean currentReadIsPreparingInconsistent ) + { + callCounter = 0; + this.currentReadIsPreparingInconsistent = currentReadIsPreparingInconsistent; + } - private boolean currentReadIsPreparingInconsistent; - private boolean currentReadIsInconsistent; - private int callCounter; - private List inconsistentReadHistory; + public void injectFailure( Class... failureTypes ) + { + adversary.injectFailure( failureTypes ); + } + + public boolean injectFailureOrMischief( Class... failureTypes ) + { + return adversary.injectFailureOrMischief( failureTypes ); + } + + private boolean hasPreparedInconsistentRead() + { + if ( currentReadIsPreparingInconsistent ) + { + currentReadIsPreparingInconsistent = false; + currentReadIsInconsistent = true; + callCounter = ThreadLocalRandom.current().nextInt( callCounter + 1 ); + inconsistentReadHistory = new ArrayList<>(); + return true; + } + return false; + } + + private boolean hasInconsistentRead() + { + if ( currentReadIsInconsistent ) + { + currentReadIsInconsistent = false; + return true; + } + return false; + } + } + + private final PageCursor delegate; + private AdversarialReadPageCursor linkedCursor; + private State state; AdversarialReadPageCursor( PageCursor delegate, Adversary adversary ) { this.delegate = Objects.requireNonNull( delegate ); - this.adversary = Objects.requireNonNull( adversary ); + this.state = new State( Objects.requireNonNull( adversary ) ); + } + + private AdversarialReadPageCursor( PageCursor delegate, State state ) + { + this.delegate = Objects.requireNonNull( delegate ); + this.state = state; } @Override @@ -70,40 +177,12 @@ public byte getByte() private Number inconsistently( T value ) { - if ( currentReadIsPreparingInconsistent ) - { - callCounter++; - return value; - } - if ( currentReadIsInconsistent && (--callCounter) <= 0 ) - { - ThreadLocalRandom rng = ThreadLocalRandom.current(); - long x = value.longValue(); - if ( x != 0 & rng.nextBoolean() ) - { - x = ~x; - } - else - { - x = rng.nextLong(); - } - inconsistentReadHistory.add( new NumberValue( value.getClass(), x, delegate.getOffset() ) ); - return x; - } - return value; + return state.inconsistently( value, delegate ); } private void inconsistently( byte[] data ) { - if ( currentReadIsPreparingInconsistent ) - { - callCounter++; - } - else if ( currentReadIsInconsistent ) - { - ThreadLocalRandom.current().nextBytes( data ); - inconsistentReadHistory.add( Arrays.copyOf( data, data.length ) ); - } + state.inconsistently( data ); } @Override @@ -225,7 +304,7 @@ public void putShort( int offset, short value ) @Override public void setOffset( int offset ) { - adversary.injectFailure( IndexOutOfBoundsException.class ); + state.injectFailure( IndexOutOfBoundsException.class ); delegate.setOffset( offset ); } @@ -262,18 +341,18 @@ public void rewind() @Override public boolean next() throws IOException { - currentReadIsPreparingInconsistent = adversary.injectFailureOrMischief( FileNotFoundException.class, IOException.class, + boolean currentReadIsPreparingInconsistent = state.injectFailureOrMischief( FileNotFoundException.class, IOException.class, SecurityException.class, IllegalStateException.class ); - callCounter = 0; + state.reset( currentReadIsPreparingInconsistent ); return delegate.next(); } @Override public boolean next( long pageId ) throws IOException { - currentReadIsPreparingInconsistent = adversary.injectFailureOrMischief( FileNotFoundException.class, IOException.class, + boolean currentReadIsPreparingInconsistent = state.injectFailureOrMischief( FileNotFoundException.class, IOException.class, SecurityException.class, IllegalStateException.class ); - callCounter = 0; + state.reset( currentReadIsPreparingInconsistent ); return delegate.next( pageId ); } @@ -287,21 +366,16 @@ public void close() @Override public boolean shouldRetry() throws IOException { - adversary.injectFailure( FileNotFoundException.class, IOException.class, SecurityException.class, + state.injectFailure( FileNotFoundException.class, IOException.class, SecurityException.class, IllegalStateException.class ); - if ( currentReadIsPreparingInconsistent ) + if ( state.hasPreparedInconsistentRead() ) { - currentReadIsPreparingInconsistent = false; - currentReadIsInconsistent = true; - callCounter = ThreadLocalRandom.current().nextInt( callCounter + 1 ); - inconsistentReadHistory = new ArrayList<>(); delegate.shouldRetry(); delegate.setOffset( 0 ); return true; } - if ( currentReadIsInconsistent ) + if ( state.hasInconsistentRead() ) { - currentReadIsInconsistent = false; delegate.shouldRetry(); delegate.setOffset( 0 ); return true; @@ -313,12 +387,12 @@ public boolean shouldRetry() throws IOException @Override public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, int lengthInBytes ) { - adversary.injectFailure( IndexOutOfBoundsException.class ); - if ( currentReadIsPreparingInconsistent ) + state.injectFailure( IndexOutOfBoundsException.class ); + if ( state.currentReadIsPreparingInconsistent ) { - callCounter++; + state.callCounter++; } - if ( !currentReadIsInconsistent ) + if ( !state.currentReadIsInconsistent ) { delegate.copyTo( sourceOffset, targetCursor, targetOffset, lengthInBytes ); } @@ -340,7 +414,24 @@ public void raiseOutOfBounds() @Override public PageCursor openLinkedCursor( long pageId ) { - return linkedCursor = new AdversarialReadPageCursor( delegate.openLinkedCursor( pageId ), adversary ); + return linkedCursor = new AdversarialReadPageCursor( delegate.openLinkedCursor( pageId ), state ); + } + + @Override + public String toString() + { + State s = this.state; + StringBuilder sb = new StringBuilder(); + for ( Object o : s.inconsistentReadHistory ) + { + sb.append( o.toString() ).append( '\n' ); + if ( o instanceof NumberValue ) + { + NumberValue v = (NumberValue) o; + v.printStackTrace( sb ); + } + } + return sb.toString(); } private static class NumberValue @@ -348,13 +439,15 @@ private static class NumberValue private final Class type; private final Long value; private final int offset; + private final Number insteadOf; private final Exception trace; - public NumberValue( Class type, Long value, int offset ) + NumberValue( Class type, Long value, int offset, Number insteadOf ) { this.type = type; this.value = value; this.offset = offset; + this.insteadOf = insteadOf; trace = new Exception( toString() ); trace.fillInStackTrace(); } @@ -365,17 +458,22 @@ public String toString() String typeName = type.getCanonicalName(); switch ( typeName ) { - case "java.lang.Byte": return "(byte)" + value.byteValue() + " at offset " + offset; - case "java.lang.Short": return "(short)" + value.shortValue() + " at offset " + offset; - case "java.lang.Integer": return "(int)" + value.intValue() + " at offset " + offset; - case "java.lang.Long": return "(long)" + value + " at offset " + offset; + case "java.lang.Byte": return "(byte)" + value.byteValue() + " at offset " + offset + " (instead of " + insteadOf + ")"; + case "java.lang.Short": return "(short)" + value.shortValue() + " at offset " + offset + " (instead of " + insteadOf + ")"; + case "java.lang.Integer": return "(int)" + value.intValue() + " at offset " + offset + " (instead of " + insteadOf + ")"; + case "java.lang.Long": return "(long)" + value + " at offset " + offset + " (instead of " + insteadOf + ")"; } - return "(" + typeName + ")" + value + " at offset " + offset; + return "(" + typeName + ")" + value + " at offset " + offset + " (instead of " + insteadOf + ")"; } - public void printStackTrace() + public void printStackTrace( StringBuilder sb ) { - trace.printStackTrace(); + StringWriter w = new StringWriter(); + PrintWriter pw = new PrintWriter( w ); + trace.printStackTrace( pw ); + pw.flush(); + sb.append( w ); + sb.append( '\n' ); } } } diff --git a/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTest.java b/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTest.java index 5625faa3076a..9fe05d2c7d38 100644 --- a/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTest.java +++ b/community/io/src/test/java/org/neo4j/io/pagecache/PageCacheTest.java @@ -532,6 +532,49 @@ public void closingWithoutCallingNextMustLeavePageUnpinnedAndUntouched() throws } } + @Test + public void nextWithNegativeInitialPageIdMustReturnFalse() throws Exception + { + File file = file( "a" ); + generateFileWithRecords( file, recordCount, recordSize ); + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + try ( PagedFile pf = pageCache.map( file, filePageSize ) ) + { + try ( PageCursor cursor = pf.io( -1, PF_SHARED_WRITE_LOCK ) ) + { + assertFalse( cursor.next() ); + } + try ( PageCursor cursor = pf.io( -1, PF_SHARED_READ_LOCK ) ) + { + assertFalse( cursor.next() ); + } + } + } + + @Test + public void nextWithNegativePageIdMustReturnFalse() throws Exception + { + File file = file( "a" ); + generateFileWithRecords( file, recordCount, recordSize ); + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + try ( PagedFile pf = pageCache.map( file, filePageSize ) ) + { + long pageId = 12; + try ( PageCursor cursor = pf.io( pageId, PF_SHARED_WRITE_LOCK ) ) + { + assertTrue( cursor.next() ); + assertFalse( cursor.next( -1 ) ); + assertThat( cursor.getCurrentPageId(), is( PageCursor.UNBOUND_PAGE_ID ) ); + } + try ( PageCursor cursor = pf.io( pageId, PF_SHARED_READ_LOCK ) ) + { + assertTrue( cursor.next() ); + assertFalse( cursor.next( -1 ) ); + assertThat( cursor.getCurrentPageId(), is( PageCursor.UNBOUND_PAGE_ID ) ); + } + } + } + @Test( timeout = SEMI_LONG_TIMEOUT_MILLIS ) public void rewindMustStartScanningOverFromTheBeginning() throws IOException { @@ -2509,6 +2552,66 @@ public void readingAndRetryingOnPageWithOptimisticReadLockingAfterUnmappingMustN } } + @Test + public void shouldRetryFromUnboundReadCursorMustNotThrow() throws Exception + { + File file = file( "a" ); + generateFileWithRecords( file, recordsPerFilePage, recordSize ); + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + try ( PagedFile pf = pageCache.map( file, filePageSize ); + PageCursor cursor = pf.io( 0, PF_SHARED_READ_LOCK ) ) + { + assertFalse( cursor.shouldRetry() ); + } + } + + @Test + public void shouldRetryFromUnboundWriteCursorMustNotThrow() throws Exception + { + File file = file( "a" ); + generateFileWithRecords( file, recordsPerFilePage, recordSize ); + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + try ( PagedFile pf = pageCache.map( file, filePageSize ); + PageCursor cursor = pf.io( 0, PF_SHARED_WRITE_LOCK ) ) + { + assertFalse( cursor.shouldRetry() ); + } + } + + @Test + public void shouldRetryFromUnboundLinkedReadCursorMustNotThrow() throws Exception + { + File file = file( "a" ); + generateFileWithRecords( file, recordsPerFilePage * 2, recordSize ); + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + try ( PagedFile pf = pageCache.map( file, filePageSize ); + PageCursor cursor = pf.io( 0, PF_SHARED_READ_LOCK ) ) + { + assertTrue( cursor.next() ); + try ( PageCursor linked = cursor.openLinkedCursor( 1 ) ) + { + assertFalse( cursor.shouldRetry() ); + } + } + } + + @Test + public void shouldRetryFromUnboundLinkedWriteCursorMustNotThrow() throws Exception + { + File file = file( "a" ); + generateFileWithRecords( file, recordsPerFilePage * 2, recordSize ); + getPageCache( fs, maxPages, pageCachePageSize, PageCacheTracer.NULL ); + try ( PagedFile pf = pageCache.map( file, filePageSize ); + PageCursor cursor = pf.io( 0, PF_SHARED_WRITE_LOCK ) ) + { + assertTrue( cursor.next() ); + try ( PageCursor linked = cursor.openLinkedCursor( 1 ) ) + { + assertFalse( cursor.shouldRetry() ); + } + } + } + private interface PageCursorAction { void apply( PageCursor cursor ); diff --git a/community/kernel/pom.xml b/community/kernel/pom.xml index b2e429579e5d..6079f8a52a83 100644 --- a/community/kernel/pom.xml +++ b/community/kernel/pom.xml @@ -80,7 +80,7 @@ the relevant Commercial Agreement. maven-failsafe-plugin - -Xmx300m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=target/test-data -Dorg.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.DIRTY_MEMORY=true -XX:+UnlockExperimentalVMOptions -XX:+TrustFinalNonStaticFields + -Xmx300m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=target/test-data -Dorg.neo4j.unsafe.impl.internal.dragons.UnsafeUtil.DIRTY_MEMORY=true -XX:+UnlockExperimentalVMOptions -XX:+TrustFinalNonStaticFields -XX:-OmitStackTraceInFastThrow diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java index 88d16fba8923..9b65aa86ae82 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/StorePropertyPayloadCursor.java @@ -124,7 +124,8 @@ else if ( position < numberOfBlocks ) PropertyType type() { - return PropertyType.getPropertyType( currentHeader(), true ); + long propBlock = currentHeader(); + return PropertyType.getPropertyTypeOrNull( propBlock ); } int propertyKeyId() diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/AbstractDynamicStore.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/AbstractDynamicStore.java index f0aea9672ee8..407783fc6584 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/AbstractDynamicStore.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/AbstractDynamicStore.java @@ -26,6 +26,7 @@ import java.util.Iterator; import java.util.List; +import org.neo4j.helpers.collection.Iterables; import org.neo4j.helpers.collection.Iterators; import org.neo4j.helpers.collection.Pair; import org.neo4j.io.pagecache.PageCache; @@ -165,7 +166,8 @@ public static Pair readFullByteArrayFromHeavyRecords( totalSize += (record.getData().length - offset); } byte[] bArray = new byte[totalSize]; - assert header != null : "header should be non-null since records should not be empty"; + assert header != null : + "header should be non-null since records should not be empty: " + Iterables.toString( records, ", " ); int sourceOffset = header.length; int offset = 0; for ( byte[] currentArray : byteList ) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/LongerShortString.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/LongerShortString.java index 8a06926cfa8f..795819a43e15 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/LongerShortString.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/LongerShortString.java @@ -779,6 +779,8 @@ public static String decode( long[] blocks, int offset, int length ) if ( encoding == ENCODING_LATIN1 ) return decodeLatin1( blocks, offset, stringLength ); LongerShortString table = getEncodingTable( encoding ); + assert table != null: "We only decode LongerShortStrings after we have consistently read the PropertyBlock " + + "data from the page cache. Thus, we should never have an invalid encoding header here."; char[] result = new char[stringLength]; // encode shifts in the bytes with the first char at the MSB, therefore // we must "unshift" in the reverse order @@ -818,6 +820,9 @@ private static void decode( char[] result, long[] blocks, int offset, LongerShor } } + /** + * Get encoding table for the given encoding header, or {@code null} if the encoding header is invalid. + */ private static LongerShortString getEncodingTable( int encodingHeader ) { if ( encodingHeader < 0 | ENCODINGS_BY_ENCODING.length <= encodingHeader ) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyStore.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyStore.java index b469f408bf0d..7133fe370a29 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyStore.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyStore.java @@ -394,4 +394,11 @@ public PropertyRecord newRecord() { return new PropertyRecord( -1 ); } + + @Override + protected void verifyAfterReading( PropertyRecord record, RecordLoad mode ) + { + super.verifyAfterReading( record, mode ); + record.verifyRecordIsWellFormed(); + } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyType.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyType.java index c77c875394fc..4695cabf4b13 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyType.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/PropertyType.java @@ -292,11 +292,14 @@ public int calculateNumberOfBlocksUsed( long firstBlock ) } }; - private final int type; + public static final byte[] EMPTY_BYTE_ARRAY = new byte[0]; + public static final int BLOCKS_USED_FOR_BAD_TYPE_OR_ENCODING = -1; // TODO In wait of a better place private static int payloadSize = PropertyRecordFormat.DEFAULT_PAYLOAD_SIZE; + private final int type; + PropertyType( int type ) { this.type = type; @@ -328,10 +331,10 @@ public byte byteValue() public abstract DefinedProperty readProperty( int propertyKeyId, PropertyBlock block, Supplier store ); - public static PropertyType getPropertyType( long propBlock, boolean nullOnIllegal ) + public static PropertyType getPropertyTypeOrNull( long propBlock ) { // [][][][][ ,tttt][kkkk,kkkk][kkkk,kkkk][kkkk,kkkk] - int type = (int)((propBlock&0x000000000F000000L)>>24); + int type = typeIdentifier( propBlock ); switch ( type ) { case 1: @@ -359,13 +362,23 @@ public static PropertyType getPropertyType( long propBlock, boolean nullOnIllega case 12: return SHORT_ARRAY; default: - if ( nullOnIllegal ) - { - return null; - } - throw new InvalidRecordException( "Unknown property type for type " - + type ); + return null; + } + } + + private static int typeIdentifier( long propBlock ) + { + return (int)((propBlock&0x000000000F000000L)>>24); + } + + public static PropertyType getPropertyTypeOrThrow( long propBlock ) + { + PropertyType type = getPropertyTypeOrNull( propBlock ); + if ( type == null ) + { + throw new InvalidRecordException( "Unknown property type for type " + typeIdentifier( propBlock ) ); } + return type; } // TODO In wait of a better place @@ -399,7 +412,4 @@ public byte[] readDynamicRecordHeader( byte[] recordBytes ) { throw new UnsupportedOperationException(); } - - public static final byte[] EMPTY_BYTE_ARRAY = new byte[0]; - public static final int BLOCKS_USED_FOR_BAD_TYPE_OR_ENCODING = -1; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/format/standard/PropertyRecordFormat.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/format/standard/PropertyRecordFormat.java index 7165b0c08ce9..23df1df95bc6 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/format/standard/PropertyRecordFormat.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/format/standard/PropertyRecordFormat.java @@ -69,7 +69,7 @@ public String read( PropertyRecord record, PageCursor cursor, RecordLoad mode, i while ( cursor.getOffset() - offsetAtBeginning < RECORD_SIZE ) { long block = cursor.getLong(); - PropertyType type = PropertyType.getPropertyType( block, true ); + PropertyType type = PropertyType.getPropertyTypeOrNull( block ); if ( type == null ) { // We assume that storage is defragged @@ -157,7 +157,7 @@ public boolean isInUse( PageCursor cursor ) for ( int i = 0; i < blocks; i++ ) { long block = cursor.getLong(); - if ( PropertyType.getPropertyType( block, true ) != null ) + if ( PropertyType.getPropertyTypeOrNull( block ) != null ) { return true; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/DynamicRecord.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/DynamicRecord.java index 6630131ed96f..309660d3ba80 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/DynamicRecord.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/DynamicRecord.java @@ -148,7 +148,7 @@ public String toString() .append( getId() ) .append( ",used=" ).append(inUse() ).append( "," ) .append("(" ).append( length ).append( "),type=" ); - PropertyType type = PropertyType.getPropertyType( this.type << 24, true ); + PropertyType type = PropertyType.getPropertyTypeOrNull( (long) (this.type << 24) ); if ( type == null ) buf.append( this.type ); else buf.append( type.name() ); buf.append( ",data=" ); if ( type == PropertyType.STRING && data.length <= MAX_CHARS_IN_TO_STRING ) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/PropertyBlock.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/PropertyBlock.java index 85dc24d1bed0..ff7c9c40ad21 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/PropertyBlock.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/PropertyBlock.java @@ -52,7 +52,8 @@ public PropertyType forceGetType() private PropertyType getType( boolean force ) { - return valueBlocks == null ? null : PropertyType.getPropertyType( valueBlocks[0], force ); + return valueBlocks == null ? null : force ? PropertyType.getPropertyTypeOrNull( valueBlocks[0] ) + : PropertyType.getPropertyTypeOrThrow( valueBlocks[0] ); } public int getKeyIndexId() diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/PropertyRecord.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/PropertyRecord.java index 008c58b21f99..68f661b53682 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/PropertyRecord.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/record/PropertyRecord.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.NoSuchElementException; +import org.neo4j.kernel.impl.store.InvalidRecordException; import org.neo4j.kernel.impl.store.PropertyType; import static org.neo4j.kernel.impl.store.record.Record.NO_NEXT_PROPERTY; @@ -64,6 +65,7 @@ public class PropertyRecord extends AbstractBaseRecord implements Iterable deletedRecords; + private String malformedMessage; // state for the Iterator aspect of this class. private int blockRecordsIteratorCursor; @@ -245,7 +247,7 @@ private void ensureBlocksLoaded() int index = 0; while ( index < blocksCursor ) { - PropertyType type = PropertyType.getPropertyType( blocks[index], false ); + PropertyType type = PropertyType.getPropertyTypeOrThrow( blocks[index] ); PropertyBlock block = new PropertyBlock(); int length = type.calculateNumberOfBlocksUsed( blocks[index] ); block.setValueBlocks( Arrays.copyOfRange( blocks, index, index + length ) ); @@ -256,6 +258,19 @@ private void ensureBlocksLoaded() } } + public void verifyRecordIsWellFormed() + { + if ( malformedMessage != null ) + { + throw new InvalidRecordException( malformedMessage ); + } + } + + public void setMalformedMessage( String message ) + { + malformedMessage = message; + } + public void setPropertyBlock( PropertyBlock block ) { removePropertyBlock( block.getKeyIndexId() ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_0.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_0.java index 3a70da9bf988..64d9266e5c5c 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_0.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_0.java @@ -413,9 +413,9 @@ private PropertyBlock readPropertyBlock( ReadableChannel channel ) throws IOExce long[] blocks = readLongs( channel, blockSize / 8 ); assert blocks.length == blockSize / 8 : blocks.length + " longs were read in while i asked for what corresponds to " + blockSize; - assert PropertyType.getPropertyType( blocks[0], false ).calculateNumberOfBlocksUsed( + assert PropertyType.getPropertyTypeOrThrow( blocks[0] ).calculateNumberOfBlocksUsed( blocks[0] ) == blocks.length : blocks.length + " is not a valid number of blocks for type " - + PropertyType.getPropertyType( blocks[0], false ); + + PropertyType.getPropertyTypeOrThrow( blocks[0] ); /* * Ok, now we may be ready to return, if there are no DynamicRecords. So * we start building the Object diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_1.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_1.java index ddfca78ea51d..06a4c8d28c23 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_1.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_1.java @@ -435,9 +435,9 @@ private PropertyBlock readPropertyBlock( ReadableChannel channel ) throws IOExce long[] blocks = readLongs( channel, blockSize / 8 ); assert blocks.length == blockSize / 8 : blocks.length + " longs were read in while i asked for what corresponds to " + blockSize; - assert PropertyType.getPropertyType( blocks[0], false ).calculateNumberOfBlocksUsed( + assert PropertyType.getPropertyTypeOrThrow( blocks[0] ).calculateNumberOfBlocksUsed( blocks[0] ) == blocks.length : blocks.length + " is not a valid number of blocks for type " - + PropertyType.getPropertyType( blocks[0], false ); + + PropertyType.getPropertyTypeOrThrow( blocks[0] ); /* * Ok, now we may be ready to return, if there are no DynamicRecords. So * we start building the Object diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_2.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_2.java index fa40de2ba473..154d6b819f44 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_2.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_2.java @@ -497,9 +497,9 @@ private PropertyBlock readPropertyBlock( ReadableChannel channel ) throws IOExce long[] blocks = readLongs( channel, blockSize / 8 ); assert blocks.length == blockSize / 8 : blocks.length + " longs were read in while i asked for what corresponds to " + blockSize; - assert PropertyType.getPropertyType( blocks[0], false ).calculateNumberOfBlocksUsed( + assert PropertyType.getPropertyTypeOrThrow( blocks[0] ).calculateNumberOfBlocksUsed( blocks[0] ) == blocks.length : blocks.length + " is not a valid number of blocks for type " - + PropertyType.getPropertyType( blocks[0], false ); + + PropertyType.getPropertyTypeOrThrow( blocks[0] ); /* * Ok, now we may be ready to return, if there are no DynamicRecords. So * we start building the Object diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_2_4.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_2_4.java index d3b0fe39bff0..d3f84a0b9add 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_2_4.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV2_2_4.java @@ -497,9 +497,9 @@ private PropertyBlock readPropertyBlock( ReadableChannel channel ) throws IOExce long[] blocks = readLongs( channel, blockSize / 8 ); assert blocks.length == blockSize / 8 : blocks.length + " longs were read in while i asked for what corresponds to " + blockSize; - assert PropertyType.getPropertyType( blocks[0], false ).calculateNumberOfBlocksUsed( + assert PropertyType.getPropertyTypeOrThrow( blocks[0] ).calculateNumberOfBlocksUsed( blocks[0] ) == blocks.length : blocks.length + " is not a valid number of blocks for type " - + PropertyType.getPropertyType( blocks[0], false ); + + PropertyType.getPropertyTypeOrThrow( blocks[0] ); /* * Ok, now we may be ready to return, if there are no DynamicRecords. So * we start building the Object diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV3_0.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV3_0.java index e81f149761fd..8a463772f9c6 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV3_0.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/PhysicalLogCommandReaderV3_0.java @@ -591,9 +591,10 @@ private PropertyBlock readPropertyBlock( ReadableChannel channel ) throws IOExce long[] blocks = readLongs( channel, blockSize / 8 ); assert blocks.length == blockSize / 8 : blocks.length + " longs were read in while i asked for what corresponds to " + blockSize; - assert PropertyType.getPropertyType( blocks[0], false ).calculateNumberOfBlocksUsed( + + assert PropertyType.getPropertyTypeOrThrow( blocks[0] ).calculateNumberOfBlocksUsed( blocks[0] ) == blocks.length : blocks.length + " is not a valid number of blocks for type " - + PropertyType.getPropertyType( blocks[0], false ); + + PropertyType.getPropertyTypeOrThrow( blocks[0] ); /* * Ok, now we may be ready to return, if there are no DynamicRecords. So * we start building the Object diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/TestIdGeneratorRebuilding.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/TestIdGeneratorRebuilding.java index 41b1a723c8e8..a0a080fc913e 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/TestIdGeneratorRebuilding.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/TestIdGeneratorRebuilding.java @@ -52,7 +52,7 @@ public class TestIdGeneratorRebuilding { @ClassRule - public static PageCacheRule pageCacheRule = new PageCacheRule(true); + public static PageCacheRule pageCacheRule = new PageCacheRule(); @Rule public EphemeralFileSystemRule fsRule = new EphemeralFileSystemRule(); private EphemeralFileSystemAbstraction fs; diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordBoundaryCheckingPagedFile.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordBoundaryCheckingPagedFile.java deleted file mode 100644 index b78e748347d2..000000000000 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordBoundaryCheckingPagedFile.java +++ /dev/null @@ -1,399 +0,0 @@ -/* - * Copyright (c) 2002-2016 "Neo Technology," - * Network Engine for Objects in Lund AB [http://neotechnology.com] - * - * This file is part of Neo4j. - * - * Neo4j is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ -package org.neo4j.kernel.impl.store.format; - -import java.io.File; -import java.io.IOException; -import java.nio.channels.ReadableByteChannel; -import java.nio.channels.WritableByteChannel; - -import org.neo4j.io.pagecache.IOLimiter; -import org.neo4j.io.pagecache.PageCursor; -import org.neo4j.io.pagecache.PagedFile; - -public class RecordBoundaryCheckingPagedFile implements PagedFile -{ - private final PagedFile actual; - private final int recordSize; - private int ioCalls; - private int nextCalls; - private int setOffsetCalls; - private int unusedBytes; - private int retries; - - public RecordBoundaryCheckingPagedFile( PagedFile actual, int enforcedRecordSize ) - { - this.actual = actual; - this.recordSize = enforcedRecordSize; - } - - @Override - public int pageSize() - { - return actual.pageSize(); - } - - @Override - public PageCursor io( long pageId, int pf_flags ) throws IOException - { - ioCalls++; - return new RecordBoundaryCheckingPageCursor( actual.io( pageId, pf_flags ) ); - } - - @Override - public long getLastPageId() throws IOException - { - return actual.getLastPageId(); - } - - @Override - public void flushAndForce() throws IOException - { - actual.flushAndForce(); - } - - @Override - public void flushAndForce( IOLimiter limiter ) throws IOException - { - actual.flushAndForce( limiter ); - } - - @Override - public void close() throws IOException - { - actual.close(); - } - - @Override - public ReadableByteChannel openReadableByteChannel() throws IOException - { - return actual.openReadableByteChannel(); - } - - @Override - public WritableByteChannel openWritableByteChannel() throws IOException - { - return actual.openWritableByteChannel(); - } - - public int ioCalls() - { - return ioCalls; - } - - public int nextCalls() - { - return nextCalls; - } - - public int unusedBytes() - { - return unusedBytes; - } - - public void resetMeasurements() - { - ioCalls = unusedBytes = nextCalls = 0; - } - - class RecordBoundaryCheckingPageCursor extends PageCursor - { - private final PageCursor actual; - private int start = -10_000; - private boolean shouldReport; - - RecordBoundaryCheckingPageCursor( PageCursor actual ) - { - this.actual = actual; - } - - private void checkBoundary( int size ) - { - shouldReport = true; // since the cursor is moving - if ( size > recordSize ) - { - throw new IllegalStateException( "Tried to go beyond record boundaries. We seem to be on the " + - (nextCalls == 1 ? "first" : "second") + " page start offset:" + start + " record size:" + - recordSize + " and tried to go to " + size ); - } - } - - private void checkRelativeBoundary( int add ) - { - checkBoundary( getOffset() - start + add ); - } - - private void checkAbsoluteBoundary( int offset ) - { - checkBoundary( offset - start ); - } - - @Override - public byte getByte() - { - checkRelativeBoundary( Byte.BYTES ); - return actual.getByte(); - } - - @Override - public byte getByte( int offset ) - { - checkAbsoluteBoundary( Byte.BYTES ); - return actual.getByte( offset ); - } - - @Override - public void putByte( byte value ) - { - checkRelativeBoundary( Byte.BYTES ); - actual.putByte( value ); - } - - @Override - public void putByte( int offset, byte value ) - { - checkAbsoluteBoundary( Byte.BYTES ); - actual.putByte( offset, value ); - } - - @Override - public long getLong() - { - checkRelativeBoundary( Long.BYTES ); - return actual.getLong(); - } - - @Override - public long getLong( int offset ) - { - checkAbsoluteBoundary( Long.BYTES ); - return actual.getLong( offset ); - } - - @Override - public void putLong( long value ) - { - checkRelativeBoundary( Long.BYTES ); - actual.putLong( value ); - } - - @Override - public void putLong( int offset, long value ) - { - checkAbsoluteBoundary( Long.BYTES ); - actual.putLong( offset, value ); - } - - @Override - public int getInt() - { - checkRelativeBoundary( Integer.BYTES ); - return actual.getInt(); - } - - @Override - public int getInt( int offset ) - { - checkAbsoluteBoundary( Integer.BYTES ); - return actual.getInt( offset ); - } - - @Override - public void putInt( int value ) - { - checkRelativeBoundary( Integer.BYTES ); - actual.putInt( value ); - } - - @Override - public void putInt( int offset, int value ) - { - checkAbsoluteBoundary( Integer.BYTES ); - actual.putInt( offset, value ); - } - - @Override - public void getBytes( byte[] data ) - { - checkRelativeBoundary( data.length ); - actual.getBytes( data ); - } - - @Override - public void getBytes( byte[] data, int arrayOffset, int length ) - { - checkRelativeBoundary( length ); - actual.getBytes( data, arrayOffset, length ); - } - - @Override - public void putBytes( byte[] data ) - { - checkRelativeBoundary( data.length ); - actual.putBytes( data ); - } - - @Override - public void putBytes( byte[] data, int arrayOffset, int length ) - { - checkRelativeBoundary( length ); - actual.putBytes( data, arrayOffset, length ); - } - - @Override - public short getShort() - { - checkRelativeBoundary( Short.BYTES ); - return actual.getShort(); - } - - @Override - public short getShort( int offset ) - { - checkAbsoluteBoundary( Short.BYTES ); - return actual.getShort( offset ); - } - - @Override - public void putShort( short value ) - { - checkRelativeBoundary( Short.BYTES ); - actual.putShort( value ); - } - - @Override - public void putShort( int offset, short value ) - { - checkAbsoluteBoundary( Short.BYTES ); - actual.putShort( offset, value ); - } - - @Override - public int copyTo( int sourceOffset, PageCursor targetCursor, int targetOffset, int lengthInBytes ) - { - return actual.copyTo( sourceOffset, targetCursor, targetOffset, lengthInBytes ); - } - - @Override - public boolean checkAndClearBoundsFlag() - { - return actual.checkAndClearBoundsFlag(); - } - - @Override - public void raiseOutOfBounds() - { - actual.raiseOutOfBounds(); - } - - @Override - public PageCursor openLinkedCursor( long pageId ) - { - return new RecordBoundaryCheckingPageCursor( actual.openLinkedCursor( pageId ) ); - } - - @Override - public void setOffset( int offset ) - { - if ( offset < start || offset >= start + recordSize ) - { - reportBeforeLeavingRecord(); - start = offset; - } - setOffsetCalls++; - actual.setOffset( offset ); - } - - private void reportBeforeLeavingRecord() - { - if ( shouldReport ) - { - int currentUnused = recordSize - (getOffset() - start); - unusedBytes += currentUnused; - shouldReport = false; - } - } - - @Override - public int getOffset() - { - return actual.getOffset(); - } - - @Override - public long getCurrentPageId() - { - return actual.getCurrentPageId(); - } - - @Override - public int getCurrentPageSize() - { - return actual.getCurrentPageSize(); - } - - @Override - public File getCurrentFile() - { - return actual.getCurrentFile(); - } - - @Override - public void rewind() - { - actual.rewind(); - start = getOffset(); - } - - @Override - public boolean next() throws IOException - { - reportBeforeLeavingRecord(); - nextCalls++; - return actual.next(); - } - - @Override - public boolean next( long pageId ) throws IOException - { - reportBeforeLeavingRecord(); - nextCalls++; - return actual.next( pageId ); - } - - @Override - public void close() - { - reportBeforeLeavingRecord(); - actual.close(); - } - - @Override - public boolean shouldRetry() throws IOException - { - boolean result = actual.shouldRetry(); - if ( result ) - { - retries++; - } - return result; - } - } -} diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordFormatTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordFormatTest.java index 7fbadffa6d6d..71d339b0c9a5 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordFormatTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/format/RecordFormatTest.java @@ -153,7 +153,7 @@ private void verifyWriteAndRead( { // GIVEN PageCache pageCache = pageCacheRule.getPageCache( fsRule.get() ); - try ( PagedFile dontUseStoreFile = pageCache.map( new File( "store" ), PAGE_SIZE, CREATE ) ) + try ( PagedFile storeFile = pageCache.map( new File( "store" ), PAGE_SIZE, CREATE ) ) { long totalUnusedBytesPrimary = 0; long totalUnusedBytesSecondary = 0; @@ -162,12 +162,9 @@ private void verifyWriteAndRead( RecordKey key = keySupplier.get(); Generator generator = generatorSupplier.get(); int recordSize = format.getRecordSize( new IntStoreHeader( DATA_SIZE ) ); - RecordBoundaryCheckingPagedFile storeFile = - new RecordBoundaryCheckingPagedFile( dontUseStoreFile, recordSize ); BatchingIdSequence idSequence = new BatchingIdSequence( random.nextBoolean() ? idSureToBeOnTheNextPage( PAGE_SIZE, recordSize ) : 10 ); long smallestUnusedBytesPrimary = recordSize; - long smallestUnusedBytesSecondary = recordSize; // WHEN long time = currentTimeMillis(); @@ -180,30 +177,7 @@ private void verifyWriteAndRead( try { writeRecord( written, format, storeFile, recordSize, idSequence ); - - long recordsUsedForWriting = storeFile.nextCalls(); - long unusedBytes = storeFile.unusedBytes(); - storeFile.resetMeasurements(); - readAndVerifyRecord( written, read, format, key, storeFile, recordSize ); - - if ( written.inUse() ) - { - // unused access don't really count for "wasted space" - if ( recordsUsedForWriting == 1 ) - { - totalUnusedBytesPrimary += unusedBytes; - smallestUnusedBytesPrimary = Math.min( smallestUnusedBytesPrimary, unusedBytes ); - } - else - { - totalUnusedBytesSecondary += unusedBytes; - smallestUnusedBytesSecondary = Math.min( smallestUnusedBytesSecondary, unusedBytes ); - } - totalRecordsRequiringSecondUnit += (recordsUsedForWriting > 1 ? 1 : 0); - } - - storeFile.resetMeasurements(); idSequence.reset(); } catch ( Throwable t ) diff --git a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/BaseHighLimitRecordFormat.java b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/BaseHighLimitRecordFormat.java index efc2139daf69..fb83e7bd87bc 100644 --- a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/BaseHighLimitRecordFormat.java +++ b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/BaseHighLimitRecordFormat.java @@ -116,12 +116,12 @@ public String read( RECORD record, PageCursor primaryCursor, RecordLoad mode, in long pageId = pageIdForRecord( secondaryId, storeFile.pageSize(), recordSize ); int offset = offsetForId( secondaryId, storeFile.pageSize(), recordSize ); PageCursor secondaryCursor = primaryCursor.openLinkedCursor( pageId ); - if ( !secondaryCursor.next() ) + if ( (!secondaryCursor.next()) | offset < 0 ) { // We must have made an inconsistent read of the secondary record unit reference. // No point in trying to read this. record.clear(); - return "Secondary record placed on inaccessible page"; + return illegalSecondaryReferenceMessage( pageId ); } secondaryCursor.setOffset( offset + HEADER_BYTE); int primarySize = recordSize - (primaryCursor.getOffset() - primaryStartOffset); @@ -132,8 +132,9 @@ public String read( RECORD record, PageCursor primaryCursor, RecordLoad mode, in int secondarySize = recordSize - HEADER_BYTE; PageCursor composite = CompositePageCursor.compose( primaryCursor, primarySize, secondaryCursor, secondarySize ); + String result = doReadInternal( record, composite, recordSize, headerByte, inUse ); record.setSecondaryUnitId( secondaryId ); - return doReadInternal( record, composite, recordSize, headerByte, inUse ); + return result; } else { @@ -141,6 +142,11 @@ public String read( RECORD record, PageCursor primaryCursor, RecordLoad mode, in } } + private String illegalSecondaryReferenceMessage( long secondaryId ) + { + return "Illegal secondary record reference: " + secondaryId; + } + protected abstract String doReadInternal( RECORD record, PageCursor cursor, int recordSize, long inUseByte, boolean inUse ); @@ -254,14 +260,14 @@ protected static int length( long reference, long nullValue ) return reference == nullValue ? 0 : length( reference ); } - protected static long decode( PageCursor cursor ) + protected static long decodeCompressedReference( PageCursor cursor ) { return Reference.decode( cursor ); } - protected static long decode( PageCursor cursor, long headerByte, int headerBitMask, long nullValue ) + protected static long decodeCompressedReference( PageCursor cursor, long headerByte, int headerBitMask, long nullValue ) { - return has( headerByte, headerBitMask ) ? decode( cursor ) : nullValue; + return has( headerByte, headerBitMask ) ? decodeCompressedReference( cursor ) : nullValue; } protected static void encode( PageCursor cursor, long reference ) throws IOException diff --git a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/DynamicRecordFormat.java b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/DynamicRecordFormat.java index 785e4be0d61e..cb6ec134ca22 100644 --- a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/DynamicRecordFormat.java +++ b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/DynamicRecordFormat.java @@ -27,6 +27,7 @@ import org.neo4j.kernel.impl.store.record.DynamicRecord; import org.neo4j.kernel.impl.store.record.RecordLoad; +import static java.lang.String.format; import static org.neo4j.kernel.impl.store.format.standard.DynamicRecordFormat.payloadTooBigErrorMessage; import static org.neo4j.kernel.impl.store.format.standard.DynamicRecordFormat.readData; @@ -69,9 +70,9 @@ public String read( DynamicRecord record, PageCursor cursor, RecordLoad mode, in if ( mode.shouldLoad( inUse ) ) { int length = cursor.getShort() | cursor.getByte() << 16; - if ( length > recordSize ) + if ( length > recordSize | length < 0 ) { - return payloadTooBigErrorMessage( record, recordSize, length ); + return payloadLengthErrorMessage( record, recordSize, length ); } long next = cursor.getLong(); boolean isStartRecord = (headerByte & START_RECORD_BIT) != 0; @@ -81,6 +82,19 @@ public String read( DynamicRecord record, PageCursor cursor, RecordLoad mode, in return null; } + private String payloadLengthErrorMessage( DynamicRecord record, int recordSize, int length ) + { + return length < 0 ? + negativePayloadErrorMessage( record, length ) : + payloadTooBigErrorMessage( record, recordSize, length ); + } + + private String negativePayloadErrorMessage( DynamicRecord record, int length ) + { + return format( "DynamicRecord[%s] claims to have a negative payload of %s bytes.", + record.getId(), length ); + } + @Override public void write( DynamicRecord record, PageCursor cursor, int recordSize, PagedFile storeFile ) throws IOException { diff --git a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/NodeRecordFormat.java b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/NodeRecordFormat.java index d6b010f011bd..17e046b562a2 100644 --- a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/NodeRecordFormat.java +++ b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/NodeRecordFormat.java @@ -72,9 +72,9 @@ protected String doReadInternal( NodeRecord record, PageCursor cursor, int recor // Now read the rest of the data. The adapter will take care of moving the cursor over to the // other unit when we've exhausted the first one. - long nextRel = decode( cursor, headerByte, HAS_RELATIONSHIP_BIT, NULL ); - long nextProp = decode( cursor, headerByte, HAS_PROPERTY_BIT, NULL ); - long labelField = decode( cursor, headerByte, HAS_LABELS_BIT, NULL_LABELS ); + long nextRel = decodeCompressedReference( cursor, headerByte, HAS_RELATIONSHIP_BIT, NULL ); + long nextProp = decodeCompressedReference( cursor, headerByte, HAS_PROPERTY_BIT, NULL ); + long labelField = decodeCompressedReference( cursor, headerByte, HAS_LABELS_BIT, NULL_LABELS ); record.initialize( inUse, nextProp, dense, nextRel, labelField ); return null; } diff --git a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/RelationshipGroupRecordFormat.java b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/RelationshipGroupRecordFormat.java index 679022da0fd6..b0d50430e2be 100644 --- a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/RelationshipGroupRecordFormat.java +++ b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/RelationshipGroupRecordFormat.java @@ -70,11 +70,11 @@ protected String doReadInternal( RelationshipGroupRecord record, PageCursor curs { record.initialize( inUse, cursor.getShort() & 0xFFFF, - decode( cursor, headerByte, HAS_OUTGOING_BIT, NULL ), - decode( cursor, headerByte, HAS_INCOMING_BIT, NULL ), - decode( cursor, headerByte, HAS_LOOP_BIT, NULL ), - decode( cursor ), - decode( cursor, headerByte, HAS_NEXT_BIT, NULL ) ); + decodeCompressedReference( cursor, headerByte, HAS_OUTGOING_BIT, NULL ), + decodeCompressedReference( cursor, headerByte, HAS_INCOMING_BIT, NULL ), + decodeCompressedReference( cursor, headerByte, HAS_LOOP_BIT, NULL ), + decodeCompressedReference( cursor ), + decodeCompressedReference( cursor, headerByte, HAS_NEXT_BIT, NULL ) ); return null; } diff --git a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/RelationshipRecordFormat.java b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/RelationshipRecordFormat.java index 0695ae5aa3fd..51f29b796849 100644 --- a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/RelationshipRecordFormat.java +++ b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/store/format/highlimit/RelationshipRecordFormat.java @@ -71,15 +71,15 @@ public RelationshipRecord newRecord() } @Override - protected String doReadInternal( RelationshipRecord record, PageCursor cursor, int recordSize, long headerByte, - boolean inUse ) + protected String doReadInternal( + RelationshipRecord record, PageCursor cursor, int recordSize, long headerByte, boolean inUse ) { int type = cursor.getShort() & 0xFFFF; long recordId = record.getId(); record.initialize( inUse, - decode( cursor, headerByte, HAS_PROPERTY_BIT, NULL ), - decode( cursor ), - decode( cursor ), + decodeCompressedReference( cursor, headerByte, HAS_PROPERTY_BIT, NULL ), + decodeCompressedReference( cursor ), + decodeCompressedReference( cursor ), type, decodeAbsoluteOrRelative( cursor, headerByte, FIRST_IN_FIRST_CHAIN_BIT, recordId ), decodeAbsoluteIfPresent( cursor, headerByte, HAS_FIRST_CHAIN_NEXT_BIT, recordId ), @@ -92,7 +92,9 @@ protected String doReadInternal( RelationshipRecord record, PageCursor cursor, i private long decodeAbsoluteOrRelative( PageCursor cursor, long headerByte, int firstInStartBit, long recordId ) { - return has( headerByte, firstInStartBit ) ? decode( cursor ) : toAbsolute( decode( cursor ), recordId ); + return has( headerByte, firstInStartBit ) ? + decodeCompressedReference( cursor ) : + toAbsolute( decodeCompressedReference( cursor ), recordId ); } @Override @@ -162,6 +164,6 @@ private int getRelativeReferenceLength( long absoluteReference, long recordId ) private long decodeAbsoluteIfPresent( PageCursor cursor, long headerByte, int conditionBit, long recordId ) { - return has( headerByte, conditionBit ) ? toAbsolute( decode( cursor ), recordId ) : NULL; + return has( headerByte, conditionBit ) ? toAbsolute( decodeCompressedReference( cursor ), recordId ) : NULL; } }