Fixes issue with incremental backup failing with invalid high id

Special handling of relationship group records could result in creating
 transactions that could result in errors when applied as part of
 external transaction application. This commit fixes the underlying
 cause and adds testing specific to incremental backup to test
 against it.

community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/RecordAccess.java

@@ -80,6 +80,8 @@ public interface RecordProxy<KEY, RECORD, ADDITIONAL>
         RECORD getBefore();
 
         boolean isChanged();
+
+        boolean isCreated();
     }
 
     /**

community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/TransactionRecordState.java

@@ -50,6 +50,7 @@
 import org.neo4j.kernel.impl.transaction.state.RecordAccess.RecordProxy;
 import org.neo4j.kernel.impl.util.statistics.IntCounter;
 
+import static java.lang.String.format;
 import static org.neo4j.kernel.impl.store.NodeLabelsField.parseLabelsField;
 
 /**
@@ -160,6 +161,7 @@ public void extractCommands( Collection<Command> commands ) throws TransactionFa
 
         // Collect nodes, relationships, properties
         List<Command> nodeCommands = new ArrayList<>( context.getNodeRecords().changeSize() );
+        int skippedCommands = 0;
         for ( RecordProxy<Long, NodeRecord, Void> change : context.getNodeRecords().changes() )
         {
             NodeRecord record = change.forReadingLinkage();
@@ -191,6 +193,32 @@ public void extractCommands( Collection<Command> commands ) throws TransactionFa
         List<Command> relGroupCommands = new ArrayList<>( context.getRelGroupRecords().changeSize() );
         for ( RecordProxy<Long, RelationshipGroupRecord, Integer> change : context.getRelGroupRecords().changes() )
         {
+            if ( change.isCreated() && !change.forReadingLinkage().inUse() )
+            {
+                /*
+                 * This is an edge case that may come up and which we must handle properly. Relationship groups are
+                 * not managed by the tx state, since they are created as side effects rather than through
+                 * direct calls. However, they differ from say, dynamic records, in that their management can happen
+                 * through separate code paths. What we are interested in here is the following scenario.
+                 * 0. A node has one less relationship that is required to transition to dense node. The relationships
+                 *    it has belong to at least two different types
+                 * 1. In the same tx, a relationship is added making the node dense and all the relationships of a type
+                 *    are removed from that node. Regardless of the order these operations happen, the creation of the
+                 *    relationship (and the transition of the node to dense) will happen first.
+                 * 2. A relationship group will be created because of the transition to dense and then deleted because
+                 *    all the relationships it would hold are no longer there. This results in a relationship group
+                 *    command that appears in the tx as not in use. Depending on the final order of operations, this
+                 *    can end up using an id that is higher than the highest id seen so far. This may not be a problem
+                 *    for a single instance, but it can result in errors in cases where transactions are applied
+                 *    externally, such as backup or HA.
+                 *
+                 * The way we deal with this issue here is by not issuing a command for that offending record. This is
+                 * safe, since the record is not in use and never was, so the high id is not necessary to change and
+                 * the store remains consistent.
+                 */
+                skippedCommands++;
+                continue;
+            }
             Command.RelationshipGroupCommand command = new Command.RelationshipGroupCommand();
             command.init( change.forReadingData() );
             relGroupCommands.add( command );
@@ -217,8 +245,8 @@ public void extractCommands( Collection<Command> commands ) throws TransactionFa
             command.init( change.getBefore(), change.forChangingData(), change.getAdditionalData() );
             commands.add( command );
         }
-        assert commands.size() == noOfCommands : "Expected " + noOfCommands + " final commands, got "
-                + commands.size() + " instead";
+        assert commands.size() == noOfCommands - skippedCommands: format( "Expected %d final commands, got %d " +
+                "instead, with %d skipped", noOfCommands, commands.size(), skippedCommands );
 
         prepared = true;
     }

community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/DirectRecordAccess.java

@@ -118,6 +118,7 @@ private class DirectRecordProxy implements RecordProxy<KEY,RECORD,ADDITIONAL>
         private RECORD record;
         private ADDITIONAL additionalData;
         private boolean changed = false;
+        private final boolean created;
 
         public DirectRecordProxy( KEY key, RECORD record, ADDITIONAL additionalData, boolean created )
         {
@@ -128,6 +129,7 @@ public DirectRecordProxy( KEY key, RECORD record, ADDITIONAL additionalData, boo
             {
                 prepareChange();
             }
+            this.created = created;
         }
 
         @Override
@@ -204,6 +206,12 @@ public boolean isChanged()
         {
             return changed;
         }
+
+        @Override
+        public boolean isCreated()
+        {
+            return created;
+        }
     }
 
     @Override

community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/store/BatchingRecordAccess.java

@@ -155,5 +155,11 @@ public boolean isChanged()
         {
             return true;
         }
+
+        @Override
+        public boolean isCreated()
+        {
+            return true;
+        }
     }
 }

community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/state/TrackingRecordProxy.java

@@ -95,4 +95,10 @@ public boolean isChanged()
     {
         return changed;
     }
+
+    @Override
+    public boolean isCreated()
+    {
+        return created;
+    }
 }

community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/state/TransactionRecordStateTest.java

@@ -26,8 +26,11 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedList;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 
 import org.neo4j.graphdb.factory.GraphDatabaseSettings;
@@ -44,6 +47,7 @@
 import org.neo4j.kernel.impl.store.StoreFactory;
 import org.neo4j.kernel.impl.store.record.DynamicRecord;
 import org.neo4j.kernel.impl.store.record.NodeRecord;
+import org.neo4j.kernel.impl.store.record.RelationshipGroupRecord;
 import org.neo4j.kernel.impl.transaction.CommittedTransactionRepresentation;
 import org.neo4j.kernel.impl.transaction.TransactionRepresentation;
 import org.neo4j.kernel.impl.transaction.command.Command;
@@ -70,8 +74,10 @@
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.RETURNS_MOCKS;
 import static org.mockito.Mockito.mock;
 
+import static org.mockito.Mockito.when;
 import static org.neo4j.helpers.collection.IteratorUtil.single;
 import static org.neo4j.helpers.collection.MapUtil.stringMap;
 
@@ -197,6 +203,53 @@ public void shouldExtractUpdateCommandsInCorrectOrder() throws Exception
         assertFalse( commandIterator.hasNext() );
     }
 
+    @Test
+    public void shouldIgnoreRelationshipGroupCommandsForGroupThatIsCreatedAndDeletedInThisTx() throws Exception
+    {
+        /*
+         * This test verifies that there are no transaction commands generated for a state diff that contains a
+         * relationship group that is created and deleted in this tx. This case requires special handling because
+         * relationship groups can be created and then deleted from disjoint code paths. Look at
+         * TransactionRecordState.extractCommands() for more details.
+         *
+         * The test setup looks complicated but all it does is mock properly a NeoStoreTransactionContext to
+         * return an Iterable<RecordSet< that contains a RelationshipGroup record which has been created in this
+         * tx and also is set notInUse.
+         */
+        // Given
+        NeoStore neoStore = newNeoStore();
+        NeoStoreTransactionContext context = mock( NeoStoreTransactionContext.class, RETURNS_MOCKS );
+
+        RecordAccess<Long, RelationshipGroupRecord, Integer> relGroupRecordsMock = mock( RecordAccess.class );
+
+        Command.RelationshipGroupCommand theCommand = new Command.RelationshipGroupCommand();
+        RelationshipGroupRecord theRecord = new RelationshipGroupRecord( 1, 1 );
+        theRecord.setInUse( false ); // this is where we set the record to be not in use
+        theCommand.init( theRecord );
+
+        LinkedList<RecordAccess.RecordProxy<Long, RelationshipGroupRecord, Integer>> commands =
+                new LinkedList<>();
+
+        RecordAccess.RecordProxy<Long, RelationshipGroupRecord, Integer> theProxyMock = mock( RecordAccess.RecordProxy.class );
+        when( theProxyMock.isCreated() ).thenReturn( true ); // and this is where it is set to be created in this tx
+        when( theProxyMock.forReadingLinkage() ).thenReturn( theRecord );
+        commands.add( theProxyMock );
+
+        when( relGroupRecordsMock.changes() ).thenReturn( commands );
+        when( context.getRelGroupRecords() ).thenReturn( relGroupRecordsMock );
+        when( relGroupRecordsMock.changeSize() ).thenReturn( 1 ); // necessary for passingan assertion in recordState
+
+        TransactionRecordState recordState =
+                new TransactionRecordState( neoStore, mock( IntegrityValidator.class ), context );
+
+        // When
+        Set<Command> resultingCommands = new HashSet<>();
+        recordState.extractCommands( resultingCommands );
+
+        // Then
+        assertTrue( resultingCommands.isEmpty() );
+    }
+
     @Test
     public void shouldExtractDeleteCommandsInCorrectOrder() throws Exception
     {

enterprise/backup/src/test/java/org/neo4j/backup/IncrementalBackupTests.java

@@ -27,6 +27,7 @@
 
 import java.io.File;
 
+import org.neo4j.graphdb.Direction;
 import org.neo4j.graphdb.DynamicRelationshipType;
 import org.neo4j.graphdb.GraphDatabaseService;
 import org.neo4j.graphdb.Node;
@@ -76,7 +77,7 @@ public void shutItDown() throws Exception
     @Test
     public void shouldDoIncrementalBackup() throws Exception
     {
-        DbRepresentation initialDataSetRepresentation = createInitialDataSet2( serverPath );
+        DbRepresentation initialDataSetRepresentation = createInitialDataSet( serverPath );
         server = startServer( serverPath, "127.0.0.1:6362" );
 
         // START SNIPPET: onlineBackup
@@ -98,7 +99,50 @@ public void shouldDoIncrementalBackup() throws Exception
     }
 
 
-    private DbRepresentation createInitialDataSet2( File path )
+    @Test
+    public void shouldNotServeTransactionsWithInvalidHighIds() throws Exception
+    {
+        /*
+         * This is in effect a high level test for an edge case that happens when a relationship group is
+         * created and deleted in the same tx. This can end up causing an IllegalArgumentException because
+         * the HighIdApplier used when applying incremental updates (batch transactions in general) will postpone
+         * processing of added/altered record ids but deleted ids will be processed on application. This can result
+         * in a deleted record causing an IllegalArgumentException even though it is not the highest id in the tx.
+         *
+         * The way we try to trigger this is:
+         * 0. In one tx, create a node with 49 relationships, belonging to two types.
+         * 1. In another tx, create another relationship on that node (making it dense) and then delete all
+         *    relationships of one type. This results in the tx state having a relationship group record that was
+         *    created in this tx and also set to not in use.
+         * 2. Receipt of this tx will have the offending rel group command apply its id before the groups that are
+         *    altered. This will try to update the high id with a value larger than what has been seen previously and
+         *    fail the update.
+         * The situation is resolved by a check added in TransactionRecordState which skips the creation of such
+         * commands.
+         * Note that this problem can also happen in HA slaves.
+         */
+        DbRepresentation initialDataSetRepresentation = createInitialDataSet( serverPath );
+        server = startServer( serverPath, "127.0.0.1:6362" );
+
+        // START SNIPPET: onlineBackup
+        OnlineBackup backup = OnlineBackup.from( "127.0.0.1" );
+
+        backup.full( backupPath.getPath() );
+
+        // END SNIPPET: onlineBackup
+        assertEquals( initialDataSetRepresentation, DbRepresentation.of( backupPath ) );
+        shutdownServer( server );
+
+        DbRepresentation furtherRepresentation = createTransactiongWithWeirdRelationshipGroupRecord( serverPath );
+        server = startServer( serverPath, null );
+        // START SNIPPET: onlineBackup
+        backup.incremental( backupPath.getPath() );
+        // END SNIPPET: onlineBackup
+        assertEquals( furtherRepresentation, DbRepresentation.of( backupPath ) );
+        shutdownServer( server );
+    }
+
+    private DbRepresentation createInitialDataSet( File path )
     {
         db = startGraphDatabase( path );
         Transaction tx = db.beginTx();
@@ -134,6 +178,40 @@ private DbRepresentation addMoreData2( File path )
         return result;
     }
 
+    private DbRepresentation createTransactiongWithWeirdRelationshipGroupRecord( File path )
+    {
+        db = startGraphDatabase( path );
+        int i = 0;
+        Node node;
+        DynamicRelationshipType typeToDelete = DynamicRelationshipType.withName( "A" );
+        DynamicRelationshipType theOtherType = DynamicRelationshipType.withName( "B" );
+        int defaultDenseNodeThreshold =
+                Integer.parseInt( GraphDatabaseSettings.dense_node_threshold.getDefaultValue() );
+
+        try ( Transaction tx = db.beginTx() )
+        {
+            node = db.createNode();
+            for ( ; i < defaultDenseNodeThreshold - 1; i++ )
+            {
+                node.createRelationshipTo( db.createNode(), theOtherType );
+            }
+            node.createRelationshipTo( db.createNode(), typeToDelete );
+            tx.success();
+        }
+        try( Transaction tx = db.beginTx() )
+        {
+            node.createRelationshipTo( db.createNode(), theOtherType );
+            for ( Relationship relationship : node.getRelationships( Direction.BOTH, typeToDelete ) )
+            {
+                relationship.delete();
+            }
+            tx.success();
+        }
+        DbRepresentation result = DbRepresentation.of( db );
+        db.shutdown();
+        return result;
+    }
+
     private GraphDatabaseService startGraphDatabase( File path )
     {
         return new TestGraphDatabaseFactory().