diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/RecordAccess.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/RecordAccess.java index d0ebdb1335672..b352a1ad86211 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/RecordAccess.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/RecordAccess.java @@ -80,6 +80,8 @@ public interface RecordProxy RECORD getBefore(); boolean isChanged(); + + boolean isCreated(); } /** diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/TransactionRecordState.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/TransactionRecordState.java index 9681432772978..89e21bc15bab7 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/transaction/state/TransactionRecordState.java +++ b/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 commands ) throws TransactionFa // Collect nodes, relationships, properties List nodeCommands = new ArrayList<>( context.getNodeRecords().changeSize() ); + int skippedCommands = 0; for ( RecordProxy change : context.getNodeRecords().changes() ) { NodeRecord record = change.forReadingLinkage(); @@ -191,6 +193,32 @@ public void extractCommands( Collection commands ) throws TransactionFa List relGroupCommands = new ArrayList<>( context.getRelGroupRecords().changeSize() ); for ( RecordProxy 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 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; } diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/DirectRecordAccess.java b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/DirectRecordAccess.java index 9b927f0bceec9..61b066da33010 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/DirectRecordAccess.java +++ b/community/kernel/src/main/java/org/neo4j/unsafe/batchinsert/DirectRecordAccess.java @@ -118,6 +118,7 @@ private class DirectRecordProxy implements RecordProxy 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 diff --git a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/store/BatchingRecordAccess.java b/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/store/BatchingRecordAccess.java index 7d5a2bbfbf6a4..89ac17b2d1740 100644 --- a/community/kernel/src/main/java/org/neo4j/unsafe/impl/batchimport/store/BatchingRecordAccess.java +++ b/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; + } } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/state/TrackingRecordProxy.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/state/TrackingRecordProxy.java index d9212cab1d281..ae47947535c16 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/state/TrackingRecordProxy.java +++ b/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; + } } \ No newline at end of file diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/state/TransactionRecordStateTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/state/TransactionRecordStateTest.java index 5610c7bbdb3f7..e96769f9bbe49 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/transaction/state/TransactionRecordStateTest.java +++ b/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 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> commands = + new LinkedList<>(); + + RecordAccess.RecordProxy 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 resultingCommands = new HashSet<>(); + recordState.extractCommands( resultingCommands ); + + // Then + assertTrue( resultingCommands.isEmpty() ); + } + @Test public void shouldExtractDeleteCommandsInCorrectOrder() throws Exception { diff --git a/enterprise/backup/src/test/java/org/neo4j/backup/IncrementalBackupTests.java b/enterprise/backup/src/test/java/org/neo4j/backup/IncrementalBackupTests.java index e72a505d4140a..22c8789259821 100644 --- a/enterprise/backup/src/test/java/org/neo4j/backup/IncrementalBackupTests.java +++ b/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().