From 9f81e14ebb08fa5824c362a0063078ff4f1a19a9 Mon Sep 17 00:00:00 2001 From: fickludd Date: Fri, 26 Jan 2018 12:58:46 +0100 Subject: [PATCH] Specialize NodeProxy.getRelationships for dense nodes Also improve traversal & group cursor robustness against concurrent modification by record loading using RecordLoad.FORCE. Now if a record is deleted after the cursor got the reference, the whole record is loaded anyway, and we can continue to the next link in the chain by reading the now unused record. This works because deleting a record R redirects the previous and next records in the chain and sets R.inUse = false, but doesn't modify any other contents of R. --- .../kernel/impl/api/state/NodeStateImpl.java | 8 +- .../org/neo4j/kernel/impl/core/NodeProxy.java | 79 ++++++++++++++++--- .../impl/core/RelationshipConversion.java | 36 ++------- .../kernel/impl/newapi/AllStoreHolder.java | 22 +++++- .../DefaultRelationshipGroupCursor.java | 15 ++-- .../DefaultRelationshipTraversalCursor.java | 6 +- .../org/neo4j/kernel/impl/newapi/Read.java | 2 + .../impl/core/RelationshipConversionTest.java | 9 +-- ...T.java => RelationshipCreateDeleteIT.java} | 46 ++++++----- .../neo4j/kernel/impl/newapi/MockStore.java | 6 ++ 10 files changed, 150 insertions(+), 79 deletions(-) rename community/kernel/src/test/java/org/neo4j/kernel/impl/locking/{RelationshipCreateDeleteLockOrderingIT.java => RelationshipCreateDeleteIT.java} (65%) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/NodeStateImpl.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/NodeStateImpl.java index 6314b527cd074..590a9a307fe07 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/NodeStateImpl.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/NodeStateImpl.java @@ -372,25 +372,25 @@ public boolean isPropertyRemoved( int propertyKeyId ) @Override public PrimitiveLongIterator getAddedRelationships( Direction direction ) { - return null; + return PrimitiveLongCollections.emptyIterator(); } @Override public PrimitiveLongIterator getAddedRelationships( Direction direction, int[] relTypes ) { - return null; + return PrimitiveLongCollections.emptyIterator(); } @Override public PrimitiveLongIterator getAddedRelationships() { - return null; + return PrimitiveLongCollections.emptyIterator(); } @Override public PrimitiveLongIterator getAddedRelationships( RelationshipDirection direction, int relType ) { - return null; + return PrimitiveLongCollections.emptyIterator(); } }; diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeProxy.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeProxy.java index 0bb3271f1db3c..a60e46b896040 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeProxy.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeProxy.java @@ -42,6 +42,9 @@ import org.neo4j.internal.kernel.api.LabelSet; import org.neo4j.internal.kernel.api.NodeCursor; import org.neo4j.internal.kernel.api.PropertyCursor; +import org.neo4j.internal.kernel.api.RelationshipDenseSelectionCursor; +import org.neo4j.internal.kernel.api.RelationshipGroupCursor; +import org.neo4j.internal.kernel.api.RelationshipSparseSelectionCursor; import org.neo4j.internal.kernel.api.RelationshipTraversalCursor; import org.neo4j.internal.kernel.api.TokenRead; import org.neo4j.internal.kernel.api.exceptions.InvalidTransactionTypeKernelException; @@ -50,6 +53,8 @@ import org.neo4j.internal.kernel.api.exceptions.PropertyKeyIdNotFoundKernelException; import org.neo4j.internal.kernel.api.exceptions.explicitindex.AutoIndexingKernelException; import org.neo4j.internal.kernel.api.exceptions.schema.ConstraintValidationException; +import org.neo4j.internal.kernel.api.exceptions.schema.IllegalTokenNameException; +import org.neo4j.internal.kernel.api.exceptions.schema.TooManyLabelsException; import org.neo4j.kernel.api.KernelTransaction; import org.neo4j.kernel.api.ReadOperations; import org.neo4j.kernel.api.Statement; @@ -57,8 +62,6 @@ import org.neo4j.kernel.api.exceptions.EntityNotFoundException; import org.neo4j.kernel.api.exceptions.RelationshipTypeIdNotFoundKernelException; import org.neo4j.kernel.api.exceptions.Status; -import org.neo4j.internal.kernel.api.exceptions.schema.IllegalTokenNameException; -import org.neo4j.internal.kernel.api.exceptions.schema.TooManyLabelsException; import org.neo4j.kernel.impl.api.operations.KeyReadOperations; import org.neo4j.storageengine.api.EntityType; import org.neo4j.values.storable.Value; @@ -167,16 +170,74 @@ private ResourceIterable innerGetRelationships( final Direction di throw new NotFoundException( format( "Node %d not found", nodeId ) ); } - RelationshipTraversalCursor relationship = transaction.cursors().allocateRelationshipTraversalCursor(); - try + if ( node.isDense() ) { - node.allRelationships( relationship ); - return new RelationshipConversion( spi, relationship, direction, typeIds ); + RelationshipTraversalCursor relationship = transaction.cursors().allocateRelationshipTraversalCursor(); + RelationshipGroupCursor relationshipGroup = transaction.cursors().allocateRelationshipGroupCursor(); + try + { + node.relationships( relationshipGroup ); + RelationshipDenseSelectionCursor selectionCursor = new RelationshipDenseSelectionCursor(); + + switch ( direction ) + { + case OUTGOING: + selectionCursor.outgoing( relationshipGroup, relationship, typeIds ); + break; + + case INCOMING: + selectionCursor.incoming( relationshipGroup, relationship, typeIds ); + break; + + case BOTH: + selectionCursor.all( relationshipGroup, relationship, typeIds ); + break; + + default: + throw new IllegalStateException( "Unsupported direction: "+direction ); + } + + return new RelationshipConversion( spi, selectionCursor ); + } + catch ( Throwable e ) + { + relationshipGroup.close(); + throw e; + } } - catch ( Throwable e ) + else { - relationship.close(); - throw e; + RelationshipTraversalCursor relationship = transaction.cursors().allocateRelationshipTraversalCursor(); + try + { + node.allRelationships( relationship ); + RelationshipSparseSelectionCursor selectionCursor = new RelationshipSparseSelectionCursor(); + + switch ( direction ) + { + case OUTGOING: + selectionCursor.outgoing( relationship, typeIds ); + break; + + case INCOMING: + selectionCursor.incoming( relationship, typeIds ); + break; + + case BOTH: + selectionCursor.all( relationship, typeIds ); + break; + + default: + throw new IllegalStateException( "Unsupported direction: "+direction ); + } + + return new RelationshipConversion( spi, selectionCursor ); + } + catch ( Throwable e ) + { + relationship.close(); + throw e; + } } }; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/core/RelationshipConversion.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/core/RelationshipConversion.java index acd994f913c8b..4bb3d2d291902 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/core/RelationshipConversion.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/core/RelationshipConversion.java @@ -19,34 +19,23 @@ */ package org.neo4j.kernel.impl.core; -import org.apache.commons.lang3.ArrayUtils; - import java.util.NoSuchElementException; -import org.neo4j.graphdb.Direction; import org.neo4j.graphdb.Relationship; import org.neo4j.graphdb.ResourceIterator; -import org.neo4j.internal.kernel.api.RelationshipTraversalCursor; +import org.neo4j.internal.kernel.api.RelationshipSelectionCursor; public class RelationshipConversion implements ResourceIterator { private final EmbeddedProxySPI actions; - private final RelationshipTraversalCursor cursor; - private final Direction direction; - private final int[] typeIds; + private final RelationshipSelectionCursor cursor; private Relationship next; private boolean closed; - public RelationshipConversion( - EmbeddedProxySPI actions, - RelationshipTraversalCursor cursor, - Direction direction, - int[] typeIds) + RelationshipConversion( EmbeddedProxySPI actions, RelationshipSelectionCursor cursor ) { this.actions = actions; this.cursor = cursor; - this.direction = direction; - this.typeIds = typeIds; } @Override @@ -54,35 +43,20 @@ public boolean hasNext() { if ( next == null && !closed ) { - while ( cursor.next() ) + if ( cursor.next() ) { - if ( correctDirection() && correctType() ) - { - next = actions.newRelationshipProxy( + next = actions.newRelationshipProxy( cursor.relationshipReference(), cursor.sourceNodeReference(), cursor.label(), cursor.targetNodeReference() ); return true; - } } close(); } return next != null; } - private boolean correctDirection() - { - return direction == Direction.BOTH || - (direction == Direction.OUTGOING && cursor.originNodeReference() == cursor.sourceNodeReference()) || - (direction == Direction.INCOMING && cursor.originNodeReference() == cursor.targetNodeReference()); - } - - private boolean correctType() - { - return typeIds == null || ArrayUtils.contains( typeIds, cursor.label() ); - } - @Override public Relationship next() { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/AllStoreHolder.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/AllStoreHolder.java index 2b3f534e3388b..b0e24bf2f55d8 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/AllStoreHolder.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/AllStoreHolder.java @@ -317,21 +317,37 @@ void node( NodeRecord record, long reference, PageCursor pageCursor ) @Override void relationship( RelationshipRecord record, long reference, PageCursor pageCursor ) { + // When scanning, we inspect RelationshipRecord.inUse(), so using RecordLoad.CHECK is fine relationships.getRecordByCursor( reference, record, RecordLoad.CHECK, pageCursor ); } + @Override + void relationshipFull( RelationshipRecord record, long reference, PageCursor pageCursor ) + { + // We need to load forcefully for relationship chain traversal since otherwise we cannot + // traverse over relationship records which have been concurrently deleted + // (flagged as inUse = false). + // see + // org.neo4j.kernel.impl.store.RelationshipChainPointerChasingTest + // org.neo4j.kernel.impl.locking.RelationshipCreateDeleteIT + relationships.getRecordByCursor( reference, record, RecordLoad.FORCE, pageCursor ); + } + @Override void property( PropertyRecord record, long reference, PageCursor pageCursor ) { - //We need to load forcefully here since otherwise we can have inconsistent reads - //for properties across blocks, see org.neo4j.graphdb.ConsistentPropertyReadsIT + // We need to load forcefully here since otherwise we can have inconsistent reads + // for properties across blocks, see org.neo4j.graphdb.ConsistentPropertyReadsIT properties.getRecordByCursor( reference, record, RecordLoad.FORCE, pageCursor ); } @Override void group( RelationshipGroupRecord record, long reference, PageCursor page ) { - groups.getRecordByCursor( reference, record, RecordLoad.NORMAL, page ); + // We need to load forcefully here since otherwise we cannot traverse over groups + // records which have been concurrently deleted (flagged as inUse = false). + // @see #org.neo4j.kernel.impl.store.RelationshipChainPointerChasingTest + groups.getRecordByCursor( reference, record, RecordLoad.FORCE, page ); } @Override diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultRelationshipGroupCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultRelationshipGroupCursor.java index d72227c1b69a6..0e20fd9712319 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultRelationshipGroupCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultRelationshipGroupCursor.java @@ -59,7 +59,7 @@ void buffer( long nodeReference, long relationshipReference, Read read ) BufferedGroup current = null; while ( relationshipReference != NO_ID ) { - read.relationship( edge, relationshipReference, edgePage ); + read.relationshipFull( edge, relationshipReference, edgePage ); // find the group BufferedGroup group = buffer.get( edge.getType() ); if ( group == null ) @@ -135,11 +135,16 @@ public boolean next() setFirstLoop( bufferedGroup.loops() ); return true; } - if ( getNext() == NO_ID ) + + do { - return false; - } - read.group( this, getNext(), page ); + if ( getNext() == NO_ID ) + { + return false; + } + read.group( this, getNext(), page ); + } while ( !inUse() ); + return true; } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultRelationshipTraversalCursor.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultRelationshipTraversalCursor.java index eeba139b2a48f..84b96db95c905 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultRelationshipTraversalCursor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/DefaultRelationshipTraversalCursor.java @@ -271,7 +271,7 @@ public boolean next() // - Return first relationship if it's not deleted // Subsequent relationships need to have same type and direction - read.relationship( this, next, pageCursor ); + read.relationshipFull( this, next, pageCursor ); setupFilterState(); hasChanges = hasChanges(); @@ -313,7 +313,7 @@ public boolean next() return false; } - read.relationship( this, next, pageCursor ); + read.relationshipFull( this, next, pageCursor ); computeNext(); } while ( ( filterStore && !correctTypeAndDirection() ) || @@ -445,7 +445,7 @@ else if ( target == originNodeReference ) } else { - throw new IllegalStateException( "NOT PART OF CHAIN" ); + throw new IllegalStateException( "NOT PART OF CHAIN! " + this ); } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Read.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Read.java index 5646737c1e201..ef038e606b70b 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Read.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/newapi/Read.java @@ -500,6 +500,8 @@ public final void futureRelationshipPropertyReferenceRead( long reference ) abstract void relationship( RelationshipRecord record, long reference, PageCursor pageCursor ); + abstract void relationshipFull( RelationshipRecord record, long reference, PageCursor pageCursor ); + abstract void property( PropertyRecord record, long reference, PageCursor pageCursor ); abstract void group( RelationshipGroupRecord record, long reference, PageCursor page ); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/core/RelationshipConversionTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/core/RelationshipConversionTest.java index f96eb9ae17b11..683d6cb098db0 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/core/RelationshipConversionTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/core/RelationshipConversionTest.java @@ -22,8 +22,7 @@ import org.junit.Before; import org.junit.Test; -import org.neo4j.graphdb.Direction; -import org.neo4j.internal.kernel.api.RelationshipTraversalCursor; +import org.neo4j.internal.kernel.api.RelationshipSelectionCursor; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -39,7 +38,7 @@ public class RelationshipConversionTest private EmbeddedProxySPI nodeActions = mock( EmbeddedProxySPI.class ); private RelationshipConversion relationshipConversion; - private RelationshipTraversalCursor cursor; + private RelationshipSelectionCursor cursor; @Before public void setUp() @@ -47,8 +46,8 @@ public void setUp() when( nodeActions.newRelationshipProxy( anyLong(), anyLong(), anyInt(), anyLong() ) ) .thenReturn( new RelationshipProxy( null, 1 ) ); - cursor = mock( RelationshipTraversalCursor.class ); - relationshipConversion = new RelationshipConversion( nodeActions, cursor, Direction.BOTH, null ); + cursor = mock( RelationshipSelectionCursor.class ); + relationshipConversion = new RelationshipConversion( nodeActions, cursor ); } @Test diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/RelationshipCreateDeleteLockOrderingIT.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/RelationshipCreateDeleteIT.java similarity index 65% rename from community/kernel/src/test/java/org/neo4j/kernel/impl/locking/RelationshipCreateDeleteLockOrderingIT.java rename to community/kernel/src/test/java/org/neo4j/kernel/impl/locking/RelationshipCreateDeleteIT.java index 03105a09df287..6a52096474757 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/RelationshipCreateDeleteLockOrderingIT.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/RelationshipCreateDeleteIT.java @@ -37,8 +37,10 @@ /** * Testing on database level that creating and deleting relationships over the the same two nodes * doesn't create unnecessary deadlock scenarios, i.e. that locking order is predictable and symmetrical + * + * Also test that relationship chains are consistently read during concurrent updates. */ -public class RelationshipCreateDeleteLockOrderingIT +public class RelationshipCreateDeleteIT { @Rule public final DatabaseRule db = new ImpermanentDatabaseRule(); @@ -46,7 +48,7 @@ public class RelationshipCreateDeleteLockOrderingIT public final RandomRule random = new RandomRule(); @Test - public void shouldNotDeadlockWhenConcurrentCreateAndDeleteRelationships() throws Throwable + public void shouldNotDeadlockOrCrashFromInconsistency() throws Throwable { // GIVEN (A) -[R]-> (B) final Node a; @@ -64,22 +66,25 @@ public void shouldNotDeadlockWhenConcurrentCreateAndDeleteRelationships() throws { race.addContestant( () -> { - try ( Transaction tx = db.beginTx() ) + for ( int j = 0; j < 10; j++ ) { - Node node = random.nextBoolean() ? a : b; - for ( Relationship relationship : node.getRelationships() ) + try ( Transaction tx = db.beginTx() ) { - try - { - relationship.delete(); - } - catch ( NotFoundException e ) + Node node = random.nextBoolean() ? a : b; + for ( Relationship relationship : node.getRelationships() ) { - // This is OK and expected since there are multiple threads deleting - assertTrue( e.getMessage().contains( "already deleted" ) ); + try + { + relationship.delete(); + } + catch ( NotFoundException e ) + { + // This is OK and expected since there are multiple threads deleting + assertTrue( e.getMessage().contains( "already deleted" ) ); + } } + tx.success(); } - tx.success(); } } ); } @@ -89,13 +94,16 @@ public void shouldNotDeadlockWhenConcurrentCreateAndDeleteRelationships() throws { race.addContestant( () -> { - try ( Transaction tx = db.beginTx() ) + for ( int j = 0; j < 10; j++ ) { - boolean order = random.nextBoolean(); - Node start = order ? a : b; - Node end = order ? b : a; - start.createRelationshipTo( end, MyRelTypes.TEST ); - tx.success(); + try ( Transaction tx = db.beginTx() ) + { + boolean order = random.nextBoolean(); + Node start = order ? a : b; + Node end = order ? b : a; + start.createRelationshipTo( end, MyRelTypes.TEST ); + tx.success(); + } } } ); } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/MockStore.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/MockStore.java index 2a118f8019c0b..59b43040085f1 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/MockStore.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/newapi/MockStore.java @@ -323,6 +323,12 @@ void relationship( RelationshipRecord record, long reference, PageCursor pageCur throw new UnsupportedOperationException( "not implemented" ); } + @Override + void relationshipFull( RelationshipRecord record, long reference, PageCursor pageCursor ) + { + throw new UnsupportedOperationException( "not implemented" ); + } + @Override void property( PropertyRecord record, long reference, PageCursor pageCursor ) {