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 ) {