Considers all deleted relationships when patching chain positions

Previously when multiple consecutive relationships in a chain got deleted
the current chain positions for cached nodes (NodeImpl) got patched to go
the next relationship in that chain. Problem was that the chain
positions got patched by looking at each deleted relationship individually
and so depending on the order the relationships existed in the chain, the
chain positions might have gotten patched to another deleted relationship.

This commit collects deleted relationships and patches each chain position
knowing all deleted relationships and so these "holes" can be detected and
the next relationship after the hole selected.

community/kernel/src/main/java/org/neo4j/kernel/impl/api/TransactionRepresentationStoreApplier.java

@@ -120,5 +120,4 @@ private NeoCommandHandler getCountsStoreApplier( long transactionId, Transaction
         assert counts.acceptTx( transactionId );
         return new CountsStoreApplier( counts, neoStore.getNodeStore() );
     }
-
 }

community/kernel/src/main/java/org/neo4j/kernel/impl/api/store/PersistenceCache.java

@@ -51,14 +51,12 @@
 import org.neo4j.kernel.impl.core.RelationshipLoader;
 import org.neo4j.kernel.impl.core.RelationshipTypeTokenHolder;
 import org.neo4j.kernel.impl.core.Token;
+import org.neo4j.kernel.impl.transaction.command.RelationshipHoles;
 import org.neo4j.kernel.impl.util.RelIdArray;
 import org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper;
 import org.neo4j.kernel.impl.util.RelIdArrayWithLoops;
 
 import static org.neo4j.kernel.impl.api.store.CacheUpdateListener.NO_UPDATES;
-import static org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper.BOTH;
-import static org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper.INCOMING;
-import static org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper.OUTGOING;
 
 /**
  * This is a cache for the {@link KernelAPI}. Currently it piggy-backs on NodeImpl/RelationshipImpl
@@ -492,23 +490,12 @@ public void cacheLabel( Token token )
         labelTokenHolder.addToken( token );
     }
 
-    public void patchDeletedRelationshipNodes( long relId, int type, long firstNodeId, long firstNodeNextRelId,
-            long secondNodeId, long secondNodeNextRelId )
-    {
-        boolean loop = firstNodeId == secondNodeId;
-        invalidateNode( firstNodeId, loop ? BOTH : OUTGOING, type, relId, firstNodeNextRelId );
-        if ( !loop )
-        {
-            invalidateNode( secondNodeId, INCOMING, type, relId, secondNodeNextRelId );
-        }
-    }
-
-    private void invalidateNode( long nodeId, DirectionWrapper direction, int type, long relIdDeleted, long nextRelId )
+    public void patchDeletedRelationshipNodes( long nodeId, RelationshipHoles holes )
     {
         NodeImpl node = nodeCache.getIfCached( nodeId );
         if ( node != null )
         {
-            node.updateRelationshipChainPosition( direction, type, relIdDeleted, nextRelId );
+            node.updateRelationshipChainPosition( holes );
         }
     }
 

community/kernel/src/main/java/org/neo4j/kernel/impl/cache/BridgingCacheAccess.java

@@ -28,6 +28,7 @@
 import org.neo4j.kernel.impl.core.CacheAccessBackDoor;
 import org.neo4j.kernel.impl.core.Token;
 import org.neo4j.kernel.impl.store.record.SchemaRule;
+import org.neo4j.kernel.impl.transaction.command.RelationshipHoles;
 
 public class BridgingCacheAccess implements CacheAccessBackDoor
 {
@@ -114,11 +115,9 @@ public void addPropertyKeyToken( Token propertyKey )
     }
 
     @Override
-    public void patchDeletedRelationshipNodes( long relId, int type, long firstNodeId, long firstNodeNextRelId,
-            long secondNodeId, long secondNodeNextRelId )
+    public void patchDeletedRelationshipNodes( long nodeId, RelationshipHoles holes )
     {
-        persistenceCache.patchDeletedRelationshipNodes( relId, type, firstNodeId, firstNodeNextRelId, secondNodeId,
-                secondNodeNextRelId );
+        persistenceCache.patchDeletedRelationshipNodes( nodeId, holes );
     }
 
     @Override

community/kernel/src/main/java/org/neo4j/kernel/impl/core/CacheAccessBackDoor.java

@@ -23,6 +23,7 @@
 
 import org.neo4j.kernel.api.labelscan.NodeLabelUpdate;
 import org.neo4j.kernel.impl.store.record.SchemaRule;
+import org.neo4j.kernel.impl.transaction.command.RelationshipHoles;
 
 public interface CacheAccessBackDoor
 {
@@ -64,6 +65,5 @@ public interface CacheAccessBackDoor
      * @param secondNodeId The relId of the second node
      * @param secondNodeNextRelId The next relationship relId of the second node in its relationship chain
      */
-    void patchDeletedRelationshipNodes( long relId, int type,
-            long firstNodeId, long firstNodeNextRelId, long secondNodeId, long secondNodeNextRelId );
+    void patchDeletedRelationshipNodes( long nodeId, RelationshipHoles holes );
 }

community/kernel/src/main/java/org/neo4j/kernel/impl/core/DenseNodeChainPosition.java

@@ -27,6 +27,8 @@
 import org.neo4j.collection.primitive.PrimitiveIntCollections;
 import org.neo4j.collection.primitive.PrimitiveIntObjectMap;
 import org.neo4j.collection.primitive.PrimitiveIntObjectVisitor;
+import org.neo4j.function.primitive.FunctionFromPrimitiveLongLongToPrimitiveLong;
+import org.neo4j.function.primitive.PrimitiveLongPredicate;
 import org.neo4j.kernel.impl.store.record.Record;
 import org.neo4j.kernel.impl.store.record.RelationshipGroupRecord;
 import org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper;
@@ -139,9 +141,25 @@ public boolean atPosition( DirectionWrapper direction, int type, long position )
     }
 
     @Override
-    public void compareAndAdvance( DirectionWrapper direction, int type, long relIdDeleted, long nextRelId )
+    public boolean atPosition( final PrimitiveLongPredicate predicate )
     {
-        getTypePosition( type ).compareAndAdvance( direction, type, relIdDeleted, nextRelId );
+        AnyPositionVisitor visitor = new AnyPositionVisitor( predicate );
+        positions.visitEntries( visitor );
+        return visitor.hit;
+    }
+
+    @Override
+    public void patchPosition( final long nodeId, final FunctionFromPrimitiveLongLongToPrimitiveLong<RuntimeException> next )
+    {
+        positions.visitEntries( new PrimitiveIntObjectVisitor<RelationshipLoadingPosition,RuntimeException>()
+        {
+            @Override
+            public boolean visited( int key, RelationshipLoadingPosition value )
+            {
+                value.patchPosition( nodeId, next );
+                return false;
+            }
+        } );
     }
 
     @Override
@@ -158,6 +176,28 @@ public String toString()
         return builder.toString();
     }
 
+    private static final class AnyPositionVisitor implements
+            PrimitiveIntObjectVisitor<RelationshipLoadingPosition,RuntimeException>
+    {
+        private final PrimitiveLongPredicate predicate;
+        private boolean hit;
+
+        private AnyPositionVisitor( PrimitiveLongPredicate predicate )
+        {
+            this.predicate = predicate;
+        }
+
+        @Override
+        public boolean visited( int key, RelationshipLoadingPosition value )
+        {
+            if ( value.atPosition( predicate ) )
+            {
+                hit = true;
+            }
+            return false;
+        }
+    }
+
     private static class TypePosition implements RelationshipLoadingPosition
     {
         private final EnumMap<DirectionWrapper, RelationshipLoadingPosition> directions =
@@ -250,9 +290,25 @@ public boolean hasMore( DirectionWrapper direction, int[] types )
         }
 
         @Override
-        public void compareAndAdvance( DirectionWrapper direction, int type, long relIdDeleted, long nextRelId )
+        public boolean atPosition( PrimitiveLongPredicate predicate )
+        {
+            for ( RelationshipLoadingPosition position : directions.values() )
+            {
+                if ( position.atPosition( predicate ) )
+                {
+                    return true;
+                }
+            }
+            return false;
+        }
+
+        @Override
+        public void patchPosition( long nodeId, FunctionFromPrimitiveLongLongToPrimitiveLong<RuntimeException> next )
         {
-            directions.get( direction ).compareAndAdvance( direction, type, relIdDeleted, nextRelId );
+            for ( RelationshipLoadingPosition position : directions.values() )
+            {
+                position.patchPosition( nodeId, next );
+            }
         }
 
         @Override

community/kernel/src/main/java/org/neo4j/kernel/impl/core/NodeImpl.java

@@ -43,6 +43,7 @@
 import org.neo4j.kernel.impl.api.store.CacheUpdateListener;
 import org.neo4j.kernel.impl.store.InvalidRecordException;
 import org.neo4j.kernel.impl.store.record.Record;
+import org.neo4j.kernel.impl.transaction.command.RelationshipHoles;
 import org.neo4j.kernel.impl.util.ArrayMap;
 import org.neo4j.kernel.impl.util.RelIdArray;
 import org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper;
@@ -693,16 +694,18 @@ void setRelChainPosition( RelationshipLoadingPosition position )
         this.relChainPosition = position;
     }
 
-    public void updateRelationshipChainPosition( DirectionWrapper direction, int type, long relIdDeleted, long nextRelId )
+    public void updateRelationshipChainPosition( RelationshipHoles holes )
     {
         if ( relChainPosition != null )
         {
-            if ( relChainPosition.atPosition( direction, type, relIdDeleted ) )
+            if ( relChainPosition.atPosition( holes ) )
             {
                 synchronized ( this )
                 {
-                    // Double-checked locking. The second check will happen inside compareAndAdvance
-                    relChainPosition.compareAndAdvance( direction, type, relIdDeleted, nextRelId );
+                    if ( relChainPosition.atPosition( holes ) )
+                    {
+                        relChainPosition.patchPosition( getId(), holes );
+                    }
                 }
             }
         }

community/kernel/src/main/java/org/neo4j/kernel/impl/core/RelationshipLoadingPosition.java

@@ -19,6 +19,8 @@
  */
 package org.neo4j.kernel.impl.core;
 
+import org.neo4j.function.primitive.FunctionFromPrimitiveLongLongToPrimitiveLong;
+import org.neo4j.function.primitive.PrimitiveLongPredicate;
 import org.neo4j.helpers.CloneableInPublic;
 import org.neo4j.kernel.impl.store.record.Record;
 import org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper;
@@ -84,16 +86,27 @@ public interface RelationshipLoadingPosition extends CloneableInPublic
     boolean atPosition( DirectionWrapper direction, int type, long position );
 
     /**
-     * Used when a relationship has been deleted, so that if there's any chain that is currently at position
-     * {@code relIdDeleted}, then its position is moved to {@code nextRelId} instead so that loading is able
-     * to continue the next time that will happen.
+     * Checks whether or not the chain is at a position such that the supplied {@code predicate} returns
+     * {@code true}.
      *
-     * @param direction direction of the deleted relationship.
-     * @param type type of the deleted relationship.
-     * @param relIdDeleted relationship id that has been deleted.
-     * @param nextRelId relationship id to move the position to instead.
+     * @param predicate {@link PrimitiveLongPredicate} capable of deciding whether or not a position
+     * (relationship id) is affected.
+     * @return {@code true} if the chain is at a position such that the supplied {@code predicate}
+     * {@code true}.
      */
-    void compareAndAdvance( DirectionWrapper direction, int type, long relIdDeleted, long nextRelId );
+    boolean atPosition( PrimitiveLongPredicate predicate );
+
+    /**
+     * Used when relationships gets deleted in the middle of traversing their chain(s). Should only be called if
+     * {@link #atPosition(PrimitiveLongPredicate)} returns {@code true}. Current positions can here be
+     * moved to the next in use relationship if the current position happens to point to a deleted relationship.
+     * If a current position isn't at a deleted relationship then the {@code next} function returns whatever
+     * was passed in.
+     *
+     * @param nodeId node id of this chain position. Used for passing back into {@code next}.
+     * @param next function for getting the next in use relationship after a currently deleted position.
+     */
+    void patchPosition( long nodeId, FunctionFromPrimitiveLongLongToPrimitiveLong<RuntimeException> next );
 
     @Override
     RelationshipLoadingPosition clone();
@@ -125,7 +138,13 @@ public boolean atPosition( DirectionWrapper direction, int type, long position )
         }
 
         @Override
-        public void compareAndAdvance( DirectionWrapper direction, int type, long relIdDeleted, long nextRelId )
+        public boolean atPosition( PrimitiveLongPredicate predicate )
+        {
+            return false;
+        }
+
+        @Override
+        public void patchPosition( long nodeId, FunctionFromPrimitiveLongLongToPrimitiveLong<RuntimeException> next )
         {
         }
 

community/kernel/src/main/java/org/neo4j/kernel/impl/core/SingleChainPosition.java

@@ -19,6 +19,8 @@
  */
 package org.neo4j.kernel.impl.core;
 
+import org.neo4j.function.primitive.FunctionFromPrimitiveLongLongToPrimitiveLong;
+import org.neo4j.function.primitive.PrimitiveLongPredicate;
 import org.neo4j.kernel.impl.store.record.Record;
 import org.neo4j.kernel.impl.util.RelIdArray.DirectionWrapper;
 
@@ -57,12 +59,15 @@ public boolean atPosition( DirectionWrapper direction, int type, long position )
     }
 
     @Override
-    public void compareAndAdvance( DirectionWrapper direction, int type, long relIdDeleted, long nextRelId )
+    public boolean atPosition( PrimitiveLongPredicate predicate )
     {
-        if ( position == relIdDeleted )
-        {
-            position = nextRelId;
-        }
+        return predicate.accept( position );
+    }
+
+    @Override
+    public void patchPosition( long nodeId, FunctionFromPrimitiveLongLongToPrimitiveLong<RuntimeException> next )
+    {
+        position = next.apply( nodeId, position );
     }
 
     @Override

community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/command/NeoStoreTransactionApplier.java

@@ -49,6 +49,7 @@ public class NeoStoreTransactionApplier extends NeoCommandHandler.Adapter
     private final LockService lockService;
     private final LockGroup lockGroup;
     private final long transactionId;
+    private RelationshipHoles relationshipHoles;
 
     public NeoStoreTransactionApplier( NeoStore store, CacheAccessBackDoor cacheAccess,
                                        LockService lockService, LockGroup lockGroup, long transactionId )
@@ -99,8 +100,11 @@ public boolean visitRelationshipCommand( Command.RelationshipCommand command ) t
                 !record.inUse() && (record.getFirstNode() != -1 || record.getSecondNode() != -1);
         if ( relationshipHasBeenDeletedButNotPreviouslyAppliedToStore )
         {   // ... then we can use the fields in that record to patch the cache
-            cacheAccess.patchDeletedRelationshipNodes( command.getKey(), record.getType(), record.getFirstNode(),
-                    record.getFirstNextRel(), record.getSecondNode(), record.getSecondNextRel() );
+            if ( relationshipHoles == null )
+            {
+                relationshipHoles = new RelationshipHoles();
+            }
+            relationshipHoles.deleted( record );
         }
         return false;
     }
@@ -201,6 +205,15 @@ public boolean visitNeoStoreCommand( Command.NeoStoreCommand command ) throws IO
         return false;
     }
 
+    @Override
+    public void apply()
+    {
+        if ( relationshipHoles != null )
+        {
+            relationshipHoles.apply( cacheAccess );
+        }
+    }
+
     private boolean nodeHasBeenUpgradedToDense( NodeCommand command )
     {
         final NodeRecord before = command.getBefore();