From cb05246c5dddfa54dacd499e0b4320b104d6a997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Finn=C3=A9?= Date: Fri, 15 Jun 2018 15:22:11 +0200 Subject: [PATCH] Removes RecordCursor entirely Since it was no longer used on main execution paths, just remaining in some tests and very few odd places which could easily be changed to use the current default way of reading records, which is getting a PageCursor --- .../impl/newapi/DefaultPropertyCursor.java | 2 - .../recordstorage/RecordStorageReader.java | 5 +- .../impl/store/CommonAbstractStore.java | 28 ++- .../neo4j/kernel/impl/store/NodeStore.java | 6 - .../kernel/impl/store/PropertyStore.java | 50 ++---- .../neo4j/kernel/impl/store/RecordCursor.java | 165 ------------------ .../neo4j/kernel/impl/store/RecordStore.java | 37 ++-- .../kernel/impl/store/StoreRecordCursor.java | 128 -------------- .../batchimport/DeleteDuplicateNodesStep.java | 1 - .../impl/store/AbstractDynamicStoreTest.java | 6 +- .../CommonAbstractStoreBehaviourTest.java | 80 --------- .../impl/store/CommonAbstractStoreTest.java | 121 +------------ .../kernel/impl/store/MetaDataStoreTest.java | 6 +- .../java/org/neo4j/test/MockedNeoStores.java | 23 --- .../csv/CsvInputEstimateCalculationIT.java | 18 +- 15 files changed, 56 insertions(+), 620 deletions(-) delete mode 100644 community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordCursor.java delete mode 100644 community/kernel/src/main/java/org/neo4j/kernel/impl/store/StoreRecordCursor.java diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultPropertyCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultPropertyCursor.java index 95a64b982120b..442cea9ae7b4f 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultPropertyCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultPropertyCursor.java @@ -20,7 +20,6 @@ package org.neo4j.kernel.impl.newapi; import java.util.Iterator; -import java.util.function.IntPredicate; import java.util.regex.Pattern; import org.neo4j.internal.kernel.api.PropertyCursor; @@ -132,7 +131,6 @@ private boolean innerNext() } } - IntPredicate predicate = propertyKey -> propertiesState != null && propertiesState.isPropertyChangedOrRemoved( propertyKey ); while ( storeCursor.next() ) { boolean skip = propertiesState != null && propertiesState.isPropertyChangedOrRemoved( storeCursor.propertyKey() ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageReader.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageReader.java index e0685dfdaa88b..e67257cccff62 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageReader.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/storageengine/impl/recordstorage/RecordStorageReader.java @@ -376,7 +376,10 @@ public void close() { assert !closed; closeSchemaResources(); - commandCreationContext.close(); + if ( commandCreationContext != null ) + { + commandCreationContext.close(); + } closed = true; } 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 index 01f9d90ce62e8..449266cd7f5d7 100644 --- 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 @@ -25,7 +25,8 @@ import java.nio.file.OpenOption; import java.nio.file.StandardOpenOption; import java.util.ArrayList; -import java.util.Collection; +import java.util.Collections; +import java.util.List; import org.neo4j.graphdb.config.Setting; import org.neo4j.graphdb.factory.GraphDatabaseSettings; @@ -1046,7 +1047,7 @@ public void getRecordByCursor( long id, RECORD record, RecordLoad mode, PageCurs } } - void readIntoRecord( long id, RECORD record, RecordLoad mode, PageCursor cursor ) throws IOException + private void readIntoRecord( long id, RECORD record, RecordLoad mode, 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 @@ -1177,9 +1178,14 @@ public void scanAllRecords( Visitor getRecords( long firstId, RecordLoad mode ) + public List getRecords( long firstId, RecordLoad mode ) { - Collection records = new ArrayList<>(); + if ( Record.NULL_REFERENCE.is( firstId ) ) + { + return Collections.emptyList(); + } + + List records = new ArrayList<>(); long id = firstId; try ( PageCursor cursor = openPageCursorForReading( firstId ) ) { @@ -1188,23 +1194,15 @@ public Collection getRecords( long firstId, RecordLoad mode ) { record = newRecord(); getRecordByCursor( id, record, mode, cursor ); - if ( record.inUse() ) - { - records.add( record ); - } + // Even unused records gets added and returned + records.add( record ); id = getNextRecordReference( record ); } - while ( record.inUse() && !Record.NULL_REFERENCE.is( id ) ); + while ( !Record.NULL_REFERENCE.is( id ) ); } return records; } - @Override - public RecordCursor newRecordCursor( final RECORD record ) - { - return new StoreRecordCursor<>( record, this ); - } - private void verifyAfterNotRead( RECORD record, RecordLoad mode ) { record.clear(); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/NodeStore.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/NodeStore.java index 1ebead7d964bc..5265b25566968 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/NodeStore.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/NodeStore.java @@ -59,12 +59,6 @@ public static Long readOwnerFromDynamicLabelsRecord( DynamicRecord record ) return bits.getLong( requiredBits ); } - public RecordCursor newLabelCursor() - { - return dynamicLabelStore.newRecordCursor( dynamicLabelStore.newRecord() ).acquire( getNumberOfReservedLowIds(), - RecordLoad.NORMAL ); - } - public NodeStore( File fileName, Config config, 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 a24c8bcf4652d..1757ddb58224c 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 @@ -19,8 +19,6 @@ */ package org.neo4j.kernel.impl.store; -import org.eclipse.collections.api.map.primitive.LongObjectMap; - import java.io.File; import java.nio.ByteBuffer; import java.nio.ByteOrder; @@ -31,7 +29,6 @@ import java.util.List; import java.util.function.ToIntFunction; -import org.neo4j.cursor.Cursor; import org.neo4j.helpers.collection.Iterables; import org.neo4j.helpers.collection.Pair; import org.neo4j.io.pagecache.PageCache; @@ -270,19 +267,14 @@ public void ensureHeavy( PropertyBlock block ) PropertyType type = block.getType(); RecordStore dynamicStore = dynamicStoreForValueType( type ); - if ( dynamicStore == null ) - { - return; - } - - try ( Cursor dynamicRecords = dynamicStore.newRecordCursor( dynamicStore.newRecord() ) - .acquire( block.getSingleValueLong(), NORMAL ) ) + if ( dynamicStore != null ) { - while ( dynamicRecords.next() ) + List dynamicRecords = dynamicStore.getRecords( block.getSingleValueLong(), NORMAL ); + for ( DynamicRecord dynamicRecord : dynamicRecords ) { - dynamicRecords.get().setType( type.intValue() ); - block.addValueRecord( dynamicRecords.get().clone() ); + dynamicRecord.setType( type.intValue() ); } + block.setValueRecords( dynamicRecords ); } } @@ -301,14 +293,12 @@ public Value getValue( PropertyBlock propertyBlock ) return propertyBlock.getType().value( propertyBlock, this ); } - public static void allocateStringRecords( Collection target, byte[] chars, - DynamicRecordAllocator allocator ) + private static void allocateStringRecords( Collection target, byte[] chars, DynamicRecordAllocator allocator ) { AbstractDynamicStore.allocateRecordsFromBytes( target, chars, allocator ); } - public static void allocateArrayRecords( Collection target, Object array, - DynamicRecordAllocator allocator, boolean allowStorePoints ) + private static void allocateArrayRecords( Collection target, Object array, DynamicRecordAllocator allocator, boolean allowStorePoints ) { DynamicArrayStore.allocateRecords( target, array, allocator, allowStorePoints ); } @@ -655,26 +645,26 @@ public static String decodeString( byte[] byteArray ) return UTF8.decode( byteArray ); } - public String getStringFor( PropertyBlock propertyBlock ) + String getStringFor( PropertyBlock propertyBlock ) { ensureHeavy( propertyBlock ); return getStringFor( propertyBlock.getValueRecords() ); } - public String getStringFor( Collection dynamicRecords ) + private String getStringFor( Collection dynamicRecords ) { Pair source = stringStore.readFullByteArray( dynamicRecords, PropertyType.STRING ); // A string doesn't have a header in the data array return decodeString( source.other() ); } - public Value getArrayFor( PropertyBlock propertyBlock ) + Value getArrayFor( PropertyBlock propertyBlock ) { ensureHeavy( propertyBlock ); return getArrayFor( propertyBlock.getValueRecords() ); } - public Value getArrayFor( Iterable records ) + private Value getArrayFor( Iterable records ) { return getRightArray( arrayStore.readFullByteArray( records, PropertyType.ARRAY ) ); } @@ -699,24 +689,6 @@ public Collection getPropertyRecordChain( long firstRecordId ) return toReturn; } - public Collection getPropertyRecordChain( long firstRecordId, - LongObjectMap propertyLookup ) - { - long nextProp = firstRecordId; - List toReturn = new ArrayList<>(); - while ( nextProp != Record.NO_NEXT_PROPERTY.intValue() ) - { - PropertyRecord propRecord = propertyLookup.get( nextProp ); - if ( propRecord == null ) - { - getRecord( nextProp, propRecord = newRecord(), RecordLoad.NORMAL ); - } - toReturn.add( propRecord ); - nextProp = propRecord.getNextProp(); - } - return toReturn; - } - @Override public PropertyRecord newRecord() { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordCursor.java deleted file mode 100644 index 4ee8ce4c2509a..0000000000000 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordCursor.java +++ /dev/null @@ -1,165 +0,0 @@ -/* - * Copyright (c) 2002-2018 "Neo4j," - * Neo4j Sweden AB [http://neo4j.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; - -import java.util.ArrayList; -import java.util.List; - -import org.neo4j.cursor.Cursor; -import org.neo4j.io.pagecache.PageCursor; -import org.neo4j.kernel.impl.store.record.AbstractBaseRecord; -import org.neo4j.kernel.impl.store.record.RecordLoad; - -/** - * {@link Cursor} over {@link AbstractBaseRecord} read from a {@link RecordStore}. - * Reading multiple records from a store will be more efficient with a {@link RecordCursor} - * than calling {@link RecordStore#getRecord(long, AbstractBaseRecord, RecordLoad)} one by one - * since a {@link PageCursor} is held open until {@link #close()} is called and one and the same - * record instances is used in every call to {@link #next()}. - * - * @param type of {@link AbstractBaseRecord}. - */ -public interface RecordCursor extends Cursor -{ - /** - * Acquires this cursor, placing it at record {@code id} {@link RecordLoad} for loading record data. - * - * @param id record id to start at. - * @param mode {@link RecordLoad} for loading. - * @return this record cursor. - */ - RecordCursor acquire( long id, RecordLoad mode ); - - /** - * Moves this cursor to the specified {@code id} with the specified {@link RecordLoad mode} without actually - * fetching the record. {@link #next()} and {@link #get()} could be used next to fetch the record. - * - * @param id the id of the record. - * @param mode the mode for subsequent loading. - */ - void placeAt( long id, RecordLoad mode ); - - /** - * Moves to the next record and reads it. If this is the first call since {@link #acquire(long, RecordLoad)} - * the record specified in acquire will be read, otherwise the next record in the chain, - * {@link RecordStore#getNextRecordReference(AbstractBaseRecord)}. - * The read record can be accessed using {@link #get()}. - * - * @return whether or not that record is in use. - */ - @Override - boolean next(); - - /** - * An additional way of placing this cursor at an arbitrary record id. This is useful when stride, - * as opposed to following record chains, is controlled from the outside. - * The read record can be accessed using {@link #get()}. - * - * @param id record id to place cursor at. - * @return whether or not that record is in use. - */ - boolean next( long id ); - - /** - * An additional way of placing this cursor at an arbitrary record id. - * Calling this method will not advance the "current id" as to change which {@link #next()} will load next. - * This method is useful when there's an opportunity to load a record from an already acquired - * {@link PageCursor} and potentially even an already pinned page. - * - * @param id record id to place cursor at. - * @param record record to load the record data into. - * @param mode {@link RecordLoad} mode temporarily overriding the default provided in - * {@link #acquire(long, RecordLoad)}. - * @return whether or not that record is in use. - */ - boolean next( long id, R record, RecordLoad mode ); - - /** - * Read all records in the chain starting from the id this cursor is positioned at using either - * {@link #acquire(long, RecordLoad)} or {@link #placeAt(long, RecordLoad)}. Each next record in the chain is - * determined by {@link RecordStore#getNextRecordReference(AbstractBaseRecord)}. Each record placed in the - * resulting list is a clone of the reused record. - * - * @return records of the chain in list. - */ - @SuppressWarnings( "unchecked" ) - default List getAll() - { - List recordList = new ArrayList<>(); - while ( next() ) - { - recordList.add( (R) get().clone() ); - } - return recordList; - } - - class Delegator implements RecordCursor - { - private final RecordCursor actual; - - public Delegator( RecordCursor actual ) - { - this.actual = actual; - } - - @Override - public R get() - { - return actual.get(); - } - - @Override - public boolean next() - { - return actual.next(); - } - - @Override - public void placeAt( long id, RecordLoad mode ) - { - actual.placeAt( id, mode ); - } - - @Override - public void close() - { - actual.close(); - } - - @Override - public RecordCursor acquire( long id, RecordLoad mode ) - { - actual.acquire( id, mode ); - return this; - } - - @Override - public boolean next( long id ) - { - return actual.next( id ); - } - - @Override - public boolean next( long id, R record, RecordLoad mode ) - { - return actual.next( id, record, mode ); - } - } -} diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordStore.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordStore.java index 24bdef1f1974b..97454edb817c0 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordStore.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/RecordStore.java @@ -21,6 +21,7 @@ import java.io.File; import java.util.Collection; +import java.util.List; import java.util.function.Predicate; import org.neo4j.graphdb.ResourceIterable; @@ -50,12 +51,11 @@ * There are two ways of getting records, either one-by-one using * {@link #getRecord(long, AbstractBaseRecord, RecordLoad)}, passing in record retrieved from {@link #newRecord()}. * This to make a conscious decision about who will create the record instance and in that process figure out - * ways to reduce number of record instances created. The other way is to use a {@link RecordCursor}, created - * by {@link #newRecordCursor(AbstractBaseRecord)} and placed at a certain record using - * {@link RecordCursor#placeAt(long, RecordLoad)}. A {@link RecordCursor} will keep underlying - * {@link PageCursor} open until until the {@link RecordCursor} is closed and so will be efficient if multiple - * records are retrieved from it. A {@link RecordCursor} will follow {@link #getNextRecordReference(AbstractBaseRecord)} - * references to get to {@link RecordCursor#next()} record. + * ways to reduce number of record instances created. + *

+ * The other way is to use {@link #openPageCursorForReading(long)} to open a cursor and use it to read records using + * {@link #getRecordByCursor(long, AbstractBaseRecord, RecordLoad, PageCursor)}. A {@link PageCursor} can be ket open + * to read multiple records before closing it. * * @param type of {@link AbstractBaseRecord}. */ @@ -87,8 +87,7 @@ public interface RecordStore extends IdSequen void setHighestPossibleIdInUse( long highestIdInUse ); /** - * @return a new record instance for receiving data by {@link #getRecord(long, AbstractBaseRecord, RecordLoad)} - * and {@link #newRecordCursor(AbstractBaseRecord)}. + * @return a new record instance for receiving data by {@link #getRecord(long, AbstractBaseRecord, RecordLoad)}. */ RECORD newRecord(); @@ -172,20 +171,10 @@ public interface RecordStore extends IdSequen * @return {@link Collection} of records in the loaded chain. * @throws InvalidRecordException if some record not in use and the {@code mode} is allows for throwing. */ - Collection getRecords( long firstId, RecordLoad mode ) throws InvalidRecordException; + List getRecords( long firstId, RecordLoad mode ) throws InvalidRecordException; /** - * Instantiates a new record cursor capable of iterating over records in this store. A {@link RecordCursor} - * gets created with one record and will use every time it reads records. - * - * @param record instance to use when reading record data. - * @return a new {@link RecordCursor} instance capable of reading records in this store. - */ - RecordCursor newRecordCursor( RECORD record ); - - /** - * Returns another record id which the given {@code record} references and which a {@link RecordCursor} - * would follow and read next. + * Returns another record id which the given {@code record} references, if it exists in a chain of records. * * @param record to read the "next" reference from. * @return record id of "next" record that the given {@code record} references, or {@link Record#NULL_REFERENCE} @@ -333,17 +322,11 @@ public void nextRecordByCursor( R target, RecordLoad mode, PageCursor cursor ) t } @Override - public Collection getRecords( long firstId, RecordLoad mode ) throws InvalidRecordException + public List getRecords( long firstId, RecordLoad mode ) throws InvalidRecordException { return actual.getRecords( firstId, mode ); } - @Override - public RecordCursor newRecordCursor( R record ) - { - return actual.newRecordCursor( record ); - } - @Override public long getNextRecordReference( R record ) { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/StoreRecordCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/store/StoreRecordCursor.java deleted file mode 100644 index 063cf451ab06a..0000000000000 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/store/StoreRecordCursor.java +++ /dev/null @@ -1,128 +0,0 @@ -/* - * Copyright (c) 2002-2018 "Neo4j," - * Neo4j Sweden AB [http://neo4j.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; - -import java.io.IOException; - -import org.neo4j.io.pagecache.PageCursor; -import org.neo4j.kernel.impl.store.record.AbstractBaseRecord; -import org.neo4j.kernel.impl.store.record.RecordLoad; - -import static org.neo4j.io.pagecache.PagedFile.PF_SHARED_READ_LOCK; -import static org.neo4j.kernel.impl.store.record.Record.NULL_REFERENCE; - -class StoreRecordCursor implements RecordCursor -{ - private final RECORD record; - private CommonAbstractStore store; - private long currentId; - private RecordLoad mode; - private PageCursor pageCursor; - - StoreRecordCursor( RECORD record, CommonAbstractStore store ) - { - this.record = record; - this.store = store; - } - - @Override - public boolean next() - { - try - { - return next( currentId, record, mode ); - } - finally - { - // This will get the next reference: - // inUse ==> actual next reference - // !inUse && mode == CHECK ==> NULL - // !inUse && mode == NORMAL ==> NULL (+InvalidRecordException thrown in try) - // !inUse && mode == FORCE ==> actual next reference - currentId = store.getNextRecordReference( record ); - } - } - - @Override - public boolean next( long id ) - { - return next( id, record, mode ); - } - - @Override - public boolean next( long id, RECORD record, RecordLoad mode ) - { - assert pageCursor != null : "Not initialized"; - if ( NULL_REFERENCE.is( id ) ) - { - record.clear(); - record.setId( NULL_REFERENCE.intValue() ); - return false; - } - - try - { - store.readIntoRecord( id, record, mode, pageCursor ); - return record.inUse(); - } - catch ( IOException e ) - { - throw new UnderlyingStorageException( e ); - } - } - - @Override - public void placeAt( long id, RecordLoad mode ) - { - this.currentId = id; - this.mode = mode; - } - - @Override - public void close() - { - assert pageCursor != null; - this.pageCursor.close(); - this.pageCursor = null; - } - - @Override - public RECORD get() - { - return record; - } - - @Override - public RecordCursor acquire( long id, RecordLoad mode ) - { - assert this.pageCursor == null; - this.currentId = id; - this.mode = mode; - try - { - this.pageCursor = store.storeFile.io( store.pageIdForRecord( id ), PF_SHARED_READ_LOCK ); - } - catch ( IOException e ) - { - throw new UnderlyingStorageException( e ); - } - return this; - } -} diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/DeleteDuplicateNodesStep.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/DeleteDuplicateNodesStep.java index 09f19a27f24cc..eb584634adcb2 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/DeleteDuplicateNodesStep.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/DeleteDuplicateNodesStep.java @@ -24,7 +24,6 @@ import org.neo4j.io.pagecache.PageCursor; import org.neo4j.kernel.impl.store.NodeStore; import org.neo4j.kernel.impl.store.PropertyStore; -import org.neo4j.kernel.impl.store.RecordCursor; import org.neo4j.kernel.impl.store.record.DynamicRecord; import org.neo4j.kernel.impl.store.record.NodeRecord; import org.neo4j.kernel.impl.store.record.PropertyRecord; diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/AbstractDynamicStoreTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/AbstractDynamicStoreTest.java index 0e2b83107a63f..cd5dbcdd8da7f 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/AbstractDynamicStoreTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/AbstractDynamicStoreTest.java @@ -122,8 +122,10 @@ public void dynamicRecordCursorReadsNotInUseRecords() Iterator records = store.getRecords( 1, FORCE ).iterator(); assertTrue( records.hasNext() ); assertEquals( first, records.next() ); - assertFalse( records.hasNext() ); - assertEquals( second, records.next() ); + assertTrue( records.hasNext() ); + DynamicRecord secondReadRecord = records.next(); + assertEquals( second, secondReadRecord ); + assertFalse( secondReadRecord.inUse() ); // because mode == FORCE we can still move through the chain assertTrue( records.hasNext() ); assertEquals( third, records.next() ); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java index 3a9c053294a43..2d997913c3860 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreBehaviourTest.java @@ -48,7 +48,6 @@ import org.neo4j.test.rule.PageCacheRule; 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.io.pagecache.PageCache.PAGE_SIZE; @@ -238,84 +237,6 @@ public void getRecordMustNotThrowOnCursorErrorWithForceLoadMode() store.getRecord( 5, new IntRecord( 5 ), FORCE ); } - @Test - public void recordCursorNextMustThrowOnPageOverflow() throws Exception - { - verifyExceptionOnOutOfBoundsAccess( () -> - { - try ( RecordCursor cursor = store.newRecordCursor( new IntRecord( 0 ) ).acquire( 5, NORMAL ) ) - { - cursor.next(); - } - } ); - } - - @Test - public void pageCursorErrorsMustNotLingerInRecordCursor() - { - createStore(); - RecordCursor cursor = store.newRecordCursor( new IntRecord( 1 ) ).acquire( 1, FORCE ); - cursorErrorOnRecord = 1; - // This will encounter a decoding error, which is ignored because FORCE - assertTrue( cursor.next() ); - // Then this should not fail because of the previous decoding error, even though we stay on the same page - assertTrue( cursor.next( 2, new IntRecord( 2 ), NORMAL ) ); - } - - @Test - public void shouldReadTheCorrectRecordWhenGivenAnExplicitIdAndNotUseTheCurrentIdPointer() - { - createStore(); - IntRecord record42 = new IntRecord( 42 ); - record42.value = 0x42; - store.updateRecord( record42 ); - IntRecord record43 = new IntRecord( 43 ); - record43.value = 0x43; - store.updateRecord( record43 ); - - RecordCursor 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() - { - 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 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 { @@ -481,7 +402,6 @@ private class MyStore extends CommonAbstractStore @Override public void accept( Processor processor, IntRecord record ) - throws FAILURE { throw new UnsupportedOperationException(); } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreTest.java index 6b18fe583477a..a763cda0bd820 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/CommonAbstractStoreTest.java @@ -30,7 +30,6 @@ import java.io.File; import java.io.IOException; import java.nio.file.OpenOption; -import java.util.Arrays; import java.util.function.LongSupplier; import org.neo4j.graphdb.factory.GraphDatabaseSettings; @@ -38,10 +37,6 @@ import org.neo4j.io.pagecache.PageCache; import org.neo4j.io.pagecache.PageCursor; import org.neo4j.io.pagecache.PagedFile; -import org.neo4j.io.pagecache.tracing.ConfigurablePageCursorTracerSupplier; -import org.neo4j.io.pagecache.tracing.recording.Event; -import org.neo4j.io.pagecache.tracing.recording.RecordingPageCacheTracer; -import org.neo4j.io.pagecache.tracing.recording.RecordingPageCursorTracer; import org.neo4j.kernel.configuration.Config; import org.neo4j.kernel.configuration.Settings; import org.neo4j.kernel.impl.store.format.RecordFormat; @@ -56,24 +51,16 @@ import org.neo4j.kernel.impl.store.id.validation.NegativeIdException; import org.neo4j.kernel.impl.store.id.validation.ReservedIdException; import org.neo4j.kernel.impl.store.record.AbstractBaseRecord; -import org.neo4j.kernel.impl.store.record.NodeRecord; -import org.neo4j.kernel.impl.store.record.RecordLoad; import org.neo4j.kernel.impl.storemigration.StoreFileType; import org.neo4j.logging.LogProvider; import org.neo4j.logging.NullLogProvider; import org.neo4j.test.rule.ConfigurablePageCacheRule; -import org.neo4j.test.rule.PageCacheRule; import org.neo4j.test.rule.TestDirectory; import org.neo4j.test.rule.fs.DefaultFileSystemRule; import static java.nio.file.StandardOpenOption.DELETE_ON_CLOSE; -import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.is; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -81,16 +68,10 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.when; -import static org.neo4j.io.pagecache.tracing.recording.RecordingPageCursorTracer.Pin; -import static org.neo4j.kernel.impl.store.record.Record.NO_NEXT_PROPERTY; -import static org.neo4j.kernel.impl.store.record.Record.NO_NEXT_RELATIONSHIP; -import static org.neo4j.test.rule.PageCacheRule.config; import static org.neo4j.test.rule.TestDirectory.testDirectory; public class CommonAbstractStoreTest @@ -145,26 +126,6 @@ public void shouldCloseStoreFileFirstAndIdGeneratorAfter() throws Throwable inOrder.verify( idGenerator, times( 1 ) ).close(); } - @Test - public void recordCursorCallsNextOnThePageCursor() throws IOException - { - TheStore store = newStore(); - long recordId = 4; - long pageIdForRecord = store.pageIdForRecord( recordId ); - - when( pageCursor.getCurrentPageId() ).thenReturn( pageIdForRecord ); - when( pageCursor.next( anyLong() ) ).thenReturn( true ); - - RecordCursor cursor = store.newRecordCursor( newRecord( -1 ) ); - cursor.acquire( recordId, RecordLoad.FORCE ); - - cursor.next( recordId ); - - InOrder order = inOrder( pageCursor ); - order.verify( pageCursor ).next( pageIdForRecord ); - order.verify( pageCursor ).shouldRetry(); - } - @Test public void failStoreInitializationWhenHeaderRecordCantBeRead() throws IOException { @@ -191,67 +152,6 @@ public void failStoreInitializationWhenHeaderRecordCantBeRead() throws IOExcepti } } - @Test - public void recordCursorPinsEachPageItReads() - { - File storeFile = dir.file( "a" ); - RecordingPageCacheTracer tracer = new RecordingPageCacheTracer(); - RecordingPageCursorTracer pageCursorTracer = new RecordingPageCursorTracer( Pin.class ); - PageCacheRule.PageCacheConfig pageCacheConfig = config().withTracer( tracer ) - .withCursorTracerSupplier( pageCursorTracerSupplier( pageCursorTracer ) ); - PageCache pageCache = pageCacheRule.getPageCache( fileSystemRule.get(), pageCacheConfig, Config.defaults() ); - - try ( NodeStore store = new NodeStore( storeFile, Config.defaults(), new DefaultIdGeneratorFactory( fileSystemRule.get() ), - pageCache, NullLogProvider.getInstance(), null, Standard.LATEST_RECORD_FORMATS ) ) - { - store.initialise( true ); - assertNull( tracer.tryObserve( Event.class ) ); - - long nodeId1 = insertNodeRecordAndObservePinEvent( pageCursorTracer, store ); - long nodeId2 = insertNodeRecordAndObservePinEvent( pageCursorTracer, store ); - - try ( RecordCursor cursor = store.newRecordCursor( store.newRecord() ) ) - { - cursor.acquire( 0, RecordLoad.NORMAL ); - assertTrue( cursor.next( nodeId1 ) ); - assertTrue( cursor.next( nodeId2 ) ); - } - // Because both nodes hit the same page, the code will only pin the page once and thus only emit one pin - // event. This pin event will not be observable until after we have closed the cursor. We could - // alternatively have chosen nodeId2 to be on a different page than nodeId1. In that case, the pin event - // for nodeId1 would have been visible after our call to cursor.next( nodeId2 ). - assertNotNull( pageCursorTracer.tryObserve( Pin.class ) ); - assertNull( pageCursorTracer.tryObserve( Event.class ) ); - } - } - - @Test - public void recordCursorGetAllForEmptyCursor() throws IOException - { - TheStore store = newStore(); - long recordId = 4; - long pageIdForRecord = store.pageIdForRecord( recordId ); - - when( pageCursor.getCurrentPageId() ).thenReturn( pageIdForRecord ); - when( pageCursor.next( anyInt() ) ).thenReturn( false ); - - RecordCursor cursor = store.newRecordCursor( newRecord( -1 ) ); - cursor.acquire( recordId, RecordLoad.FORCE ); - - assertThat( cursor.getAll(), is( empty() ) ); - } - - @Test - public void recordCursorGetAll() - { - TheStore store = newStore(); - RecordCursor cursor = spy( store.newRecordCursor( store.newRecord() ) ); - doReturn( true ).doReturn( true ).doReturn( true ).doReturn( false ).when( cursor ).next(); - doReturn( newRecord( 1 ) ).doReturn( newRecord( 5 ) ).doReturn( newRecord( 42 ) ).when( cursor ).get(); - - assertEquals( Arrays.asList( newRecord( 1 ), newRecord( 5 ), newRecord( 42 ) ), cursor.getAll() ); - } - @Test public void throwsWhenRecordWithNegativeIdIsUpdated() { @@ -342,25 +242,6 @@ private TheRecord newRecord( long id ) return new TheRecord( id ); } - private long insertNodeRecordAndObservePinEvent( RecordingPageCursorTracer tracer, NodeStore store ) - { - long nodeId = store.nextId(); - NodeRecord record = store.newRecord(); - record.setId( nodeId ); - record.initialize( true, NO_NEXT_PROPERTY.intValue(), false, NO_NEXT_RELATIONSHIP.intValue(), 42 ); - store.prepareForCommit( record ); - store.updateRecord( record ); - assertNotNull( tracer.tryObserve( Pin.class ) ); - assertNull( tracer.tryObserve( Event.class ) ); - return nodeId; - } - - private static ConfigurablePageCursorTracerSupplier pageCursorTracerSupplier( - RecordingPageCursorTracer pageCursorTracer ) - { - return new ConfigurablePageCursorTracerSupplier( pageCursorTracer ); - } - private static class TheStore extends CommonAbstractStore { TheStore( File fileName, Config configuration, IdType idType, IdGeneratorFactory idGeneratorFactory, @@ -389,7 +270,7 @@ public long scanForHighId() } @Override - public void accept( Processor processor, TheRecord record ) throws FAILURE + public void accept( Processor processor, TheRecord record ) { } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/MetaDataStoreTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/MetaDataStoreTest.java index 8343dd5acda97..e8ac87ffa2e84 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/store/MetaDataStoreTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/store/MetaDataStoreTest.java @@ -660,13 +660,13 @@ public void mustSupportScanningAllRecordsWithRecordCursor() throws Exception try ( MetaDataStore store = newMetaDataStore() ) { MetaDataRecord record = store.newRecord(); - try ( RecordCursor cursor = store.newRecordCursor( record ) ) + try ( PageCursor cursor = store.openPageCursorForReading( 0 ) ) { - cursor.acquire( 0, RecordLoad.NORMAL ); long highId = store.getHighId(); for ( long id = 0; id < highId; id++ ) { - if ( cursor.next( id ) ) + store.getRecordByCursor( id, record, RecordLoad.NORMAL, cursor ); + if ( record.inUse() ) { actualValues.add( record.getValue() ); } diff --git a/community/kernel/src/test/java/org/neo4j/test/MockedNeoStores.java b/community/kernel/src/test/java/org/neo4j/test/MockedNeoStores.java index c1707ba27f63d..9de2c62da59c5 100644 --- a/community/kernel/src/test/java/org/neo4j/test/MockedNeoStores.java +++ b/community/kernel/src/test/java/org/neo4j/test/MockedNeoStores.java @@ -26,14 +26,9 @@ import org.neo4j.kernel.impl.store.NeoStores; import org.neo4j.kernel.impl.store.NodeStore; import org.neo4j.kernel.impl.store.PropertyStore; -import org.neo4j.kernel.impl.store.RecordCursor; import org.neo4j.kernel.impl.store.RelationshipGroupStore; import org.neo4j.kernel.impl.store.RelationshipStore; -import org.neo4j.kernel.impl.store.record.AbstractBaseRecord; -import org.neo4j.kernel.impl.store.record.RecordLoad; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -48,55 +43,37 @@ public static NeoStores basicMockedNeoStores() { NeoStores neoStores = mock( NeoStores.class ); - // Cursor, absolutely mocked and cannot be used at all as it is - RecordCursor cursor = mockedRecordCursor(); - // NodeStore - DynamicLabelStore NodeStore nodeStore = mock( NodeStore.class ); - when( nodeStore.newRecordCursor( any() ) ).thenReturn( cursor ); when( neoStores.getNodeStore() ).thenReturn( nodeStore ); // NodeStore - DynamicLabelStore DynamicArrayStore dynamicLabelStore = mock( DynamicArrayStore.class ); - when( dynamicLabelStore.newRecordCursor( any() ) ).thenReturn( cursor ); when( nodeStore.getDynamicLabelStore() ).thenReturn( dynamicLabelStore ); // RelationshipStore RelationshipStore relationshipStore = mock( RelationshipStore.class ); - when( relationshipStore.newRecordCursor( any() ) ).thenReturn( cursor ); when( neoStores.getRelationshipStore() ).thenReturn( relationshipStore ); // RelationshipGroupStore RelationshipGroupStore relationshipGroupStore = mock( RelationshipGroupStore.class ); - when( relationshipGroupStore.newRecordCursor( any() ) ).thenReturn( cursor ); when( neoStores.getRelationshipGroupStore() ).thenReturn( relationshipGroupStore ); // PropertyStore PropertyStore propertyStore = mock( PropertyStore.class ); - when( propertyStore.newRecordCursor( any() ) ).thenReturn( cursor ); when( neoStores.getPropertyStore() ).thenReturn( propertyStore ); // PropertyStore -- DynamicStringStore DynamicStringStore propertyStringStore = mock( DynamicStringStore.class ); - when( propertyStringStore.newRecordCursor( any() ) ).thenReturn( cursor ); when( propertyStore.getStringStore() ).thenReturn( propertyStringStore ); // PropertyStore -- DynamicArrayStore DynamicArrayStore propertyArrayStore = mock( DynamicArrayStore.class ); - when( propertyArrayStore.newRecordCursor( any() ) ).thenReturn( cursor ); when( propertyStore.getArrayStore() ).thenReturn( propertyArrayStore ); return neoStores; } - @SuppressWarnings( "unchecked" ) - public static RecordCursor mockedRecordCursor() - { - RecordCursor cursor = mock( RecordCursor.class ); - when( cursor.acquire( anyLong(), any( RecordLoad.class ) ) ).thenReturn( cursor ); - return cursor; - } - public static TokenHolders mockedTokenHolders() { return new TokenHolders( diff --git a/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputEstimateCalculationIT.java b/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputEstimateCalculationIT.java index fecab3c313c29..6d6918ef1dc41 100644 --- a/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputEstimateCalculationIT.java +++ b/community/kernel/src/test/java/org/neo4j/unsafe/impl/batchimport/input/csv/CsvInputEstimateCalculationIT.java @@ -37,6 +37,7 @@ import org.neo4j.io.fs.DefaultFileSystemAbstraction; import org.neo4j.io.fs.FileSystemAbstraction; import org.neo4j.io.pagecache.PageCache; +import org.neo4j.io.pagecache.PageCursor; import org.neo4j.io.pagecache.tracing.PageCacheTracer; import org.neo4j.io.pagecache.tracing.cursor.PageCursorTracerSupplier; import org.neo4j.io.pagecache.tracing.cursor.context.EmptyVersionContextSupplier; @@ -44,16 +45,15 @@ import org.neo4j.kernel.configuration.Config; import org.neo4j.kernel.impl.logging.NullLogService; import org.neo4j.kernel.impl.pagecache.ConfiguringPageCacheFactory; -import org.neo4j.kernel.impl.storageengine.impl.recordstorage.RecordStorageReader; import org.neo4j.kernel.impl.store.NeoStores; import org.neo4j.kernel.impl.store.PropertyValueRecordSizeCalculator; import org.neo4j.kernel.impl.store.StoreFactory; import org.neo4j.kernel.impl.store.StoreType; import org.neo4j.kernel.impl.store.format.RecordFormats; import org.neo4j.kernel.impl.store.id.DefaultIdGeneratorFactory; +import org.neo4j.kernel.impl.store.record.PropertyRecord; import org.neo4j.logging.NullLog; import org.neo4j.logging.NullLogProvider; -import org.neo4j.storageengine.api.StoragePropertyCursor; import org.neo4j.test.rule.RandomRule; import org.neo4j.test.rule.TestDirectory; import org.neo4j.unsafe.impl.batchimport.AdditionalInitialIds; @@ -78,9 +78,11 @@ import static org.junit.Assert.assertThat; import static org.neo4j.csv.reader.CharSeekers.charSeeker; import static org.neo4j.csv.reader.Readables.wrap; +import static org.neo4j.helpers.collection.Iterables.count; import static org.neo4j.kernel.impl.store.MetaDataStore.DEFAULT_NAME; import static org.neo4j.kernel.impl.store.NoStoreHeader.NO_STORE_HEADER; import static org.neo4j.kernel.impl.store.format.standard.Standard.LATEST_RECORD_FORMATS; +import static org.neo4j.kernel.impl.store.record.RecordLoad.CHECK; import static org.neo4j.unsafe.impl.batchimport.ImportLogic.NO_MONITOR; import static org.neo4j.unsafe.impl.batchimport.input.RandomEntityDataGenerator.convert; import static org.neo4j.unsafe.impl.batchimport.input.csv.Configuration.COMMAS; @@ -105,7 +107,7 @@ public void shouldCalculateCorrectEstimates() throws Exception Input input = generateData(); RecordFormats format = LATEST_RECORD_FORMATS; Input.Estimates estimates = input.calculateEstimates( new PropertyValueRecordSizeCalculator( - LATEST_RECORD_FORMATS.property().getRecordSize( NO_STORE_HEADER ), + format.property().getRecordSize( NO_STORE_HEADER ), parseInt( GraphDatabaseSettings.string_block_size.getDefaultValue() ), 0, parseInt( GraphDatabaseSettings.array_block_size.getDefaultValue() ), 0 ) ); @@ -192,16 +194,16 @@ private Input generateData() throws IOException private long calculateNumberOfProperties( NeoStores stores ) { long count = 0; - try ( RecordStorageReader reader = new RecordStorageReader( stores ); - StoragePropertyCursor cursor = reader.allocatePropertyCursor() ) + PropertyRecord record = stores.getPropertyStore().newRecord(); + try ( PageCursor cursor = stores.getPropertyStore().openPageCursorForReading( 0 ) ) { long highId = stores.getPropertyStore().getHighId(); for ( long id = 0; id < highId; id++ ) { - cursor.init( id ); - while ( cursor.next() ) + stores.getPropertyStore().getRecordByCursor( id, record, CHECK, cursor ); + if ( record.inUse() ) { - count++; + count += count( record ); } } }