From 55d7b9ed6a8271a8f51cd181f7371345ec281590 Mon Sep 17 00:00:00 2001 From: Mattias Persson Date: Fri, 24 Mar 2017 10:47:21 +0100 Subject: [PATCH] Improved testing and error handling on constraint index creation on orphaned indexes --- .../api/StateHandlingStatementOperations.java | 4 +- .../api/state/ConstraintIndexCreator.java | 21 +-- .../ConstraintIndexCreatorTest.java | 126 ++++++++++++++++-- .../api/constraints/ConstraintRecoveryIT.java | 17 ++- 4 files changed, 144 insertions(+), 24 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java index 1bf20be79cf38..57bb849a7ac04 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/StateHandlingStatementOperations.java @@ -532,8 +532,8 @@ public UniquenessConstraint uniquePropertyConstraintCreate( KernelStatement stat } return constraint; } - catch ( ConstraintVerificationFailedKernelException | DropIndexFailureException | TransactionFailureException - e ) + catch ( ConstraintVerificationFailedKernelException | DropIndexFailureException | TransactionFailureException | + AlreadyConstrainedException e ) { throw new CreateConstraintFailureException( constraint, e ); } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/ConstraintIndexCreator.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/ConstraintIndexCreator.java index 4fd66bcda6b72..f4204168ed2ef 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/ConstraintIndexCreator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/state/ConstraintIndexCreator.java @@ -24,18 +24,20 @@ import org.neo4j.kernel.api.KernelAPI; import org.neo4j.kernel.api.KernelTransaction; -import org.neo4j.kernel.api.ReadOperations; import org.neo4j.kernel.api.Statement; +import org.neo4j.kernel.api.StatementTokenNameLookup; import org.neo4j.kernel.api.constraints.UniquenessConstraint; import org.neo4j.kernel.api.exceptions.TransactionFailureException; import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException; import org.neo4j.kernel.api.exceptions.index.IndexNotFoundKernelException; import org.neo4j.kernel.api.exceptions.index.IndexPopulationFailedKernelException; +import org.neo4j.kernel.api.exceptions.schema.AlreadyConstrainedException; import org.neo4j.kernel.api.exceptions.schema.ConstraintVerificationFailedKernelException; import org.neo4j.kernel.api.exceptions.schema.CreateConstraintFailureException; import org.neo4j.kernel.api.exceptions.schema.DropIndexFailureException; import org.neo4j.kernel.api.exceptions.schema.SchemaRuleNotFoundException; import org.neo4j.kernel.api.exceptions.schema.UniquenessConstraintVerificationFailedKernelException; +import org.neo4j.kernel.api.exceptions.schema.SchemaKernelException.OperationContext; import org.neo4j.kernel.api.index.IndexDescriptor; import org.neo4j.kernel.api.index.PropertyAccessor; import org.neo4j.kernel.impl.api.KernelStatement; @@ -89,9 +91,9 @@ public ConstraintIndexCreator( Supplier kernelSupplier, IndexingServi public long createUniquenessConstraintIndex( KernelStatement state, SchemaReadOperations schema, int labelId, int propertyKeyId ) throws ConstraintVerificationFailedKernelException, TransactionFailureException, - CreateConstraintFailureException, DropIndexFailureException + CreateConstraintFailureException, DropIndexFailureException, AlreadyConstrainedException { - IndexDescriptor descriptor = getOrCreateConstraintIndex( state, labelId, propertyKeyId ); + IndexDescriptor descriptor = getOrCreateConstraintIndex( state, schema, labelId, propertyKeyId ); UniquenessConstraint constraint = new UniquenessConstraint( labelId, propertyKeyId ); boolean success = false; @@ -211,11 +213,10 @@ private void awaitIndexPopulation( UniquenessConstraint constraint, long indexId } } - public IndexDescriptor getOrCreateConstraintIndex( KernelStatement state, - int labelId, int propertyKeyId ) + public IndexDescriptor getOrCreateConstraintIndex( KernelStatement state, SchemaReadOperations schema, + int labelId, int propertyKeyId ) throws AlreadyConstrainedException { - ReadOperations readOperations = state.readOperations(); - for ( IndexDescriptor descriptor : loop( readOperations.uniqueIndexesGetForLabel( labelId ) ) ) + for ( IndexDescriptor descriptor : loop( schema.uniqueIndexesGetForLabel( state, labelId ) ) ) { if ( descriptor.getPropertyKeyId() == propertyKeyId ) { @@ -224,10 +225,14 @@ public IndexDescriptor getOrCreateConstraintIndex( KernelStatement state, // constraint creation, due to crash or similar, hence the missing owner. try { - if ( readOperations.indexGetOwningUniquenessConstraintId( descriptor ) == null ) + if ( schema.indexGetOwningUniquenessConstraintId( state, descriptor ) == null ) { return descriptor; } + throw new AlreadyConstrainedException( + new UniquenessConstraint( descriptor.getLabelId(), descriptor.getPropertyKeyId() ), + OperationContext.CONSTRAINT_CREATION, + new StatementTokenNameLookup( state.readOperations() ) ); } catch ( SchemaRuleNotFoundException e ) { diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintIndexCreatorTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintIndexCreatorTest.java index 2821bf5ba22b1..a21c08c75d87a 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintIndexCreatorTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintIndexCreatorTest.java @@ -24,6 +24,7 @@ import java.util.ArrayList; import java.util.List; +import org.neo4j.helpers.collection.Iterators; import org.neo4j.kernel.api.KernelAPI; import org.neo4j.kernel.api.KernelTransaction; import org.neo4j.kernel.api.Statement; @@ -32,6 +33,7 @@ import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.api.exceptions.TransactionFailureException; import org.neo4j.kernel.api.exceptions.index.IndexPopulationFailedKernelException; +import org.neo4j.kernel.api.exceptions.schema.AlreadyConstrainedException; import org.neo4j.kernel.api.exceptions.schema.ConstraintVerificationFailedKernelException; import org.neo4j.kernel.api.index.IndexDescriptor; import org.neo4j.kernel.api.index.PreexistingIndexEntryConflictException; @@ -56,6 +58,8 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; + +import static org.neo4j.helpers.collection.Iterators.iterator; import static org.neo4j.kernel.impl.api.StatementOperationsTestHelper.mockedParts; import static org.neo4j.kernel.impl.api.StatementOperationsTestHelper.mockedState; import static org.neo4j.kernel.impl.store.SchemaStorage.IndexRuleKind.CONSTRAINT; @@ -69,7 +73,8 @@ public void shouldCreateIndexInAnotherTransaction() throws Exception StatementOperationParts constraintCreationContext = mockedParts(); StatementOperationParts indexCreationContext = mockedParts(); - IndexDescriptor descriptor = new IndexDescriptor( 123, 456 ); + int labelId = 123; + IndexDescriptor descriptor = new IndexDescriptor( labelId, 456 ); KernelStatement state = mockedState(); IndexingService indexingService = mock( IndexingService.class ); @@ -80,7 +85,8 @@ public void shouldCreateIndexInAnotherTransaction() throws Exception IndexProxy indexProxy = mock( IndexProxy.class ); when( indexingService.getIndexProxy( 2468L ) ).thenReturn( indexProxy ); PropertyAccessor propertyAccessor = mock( PropertyAccessor.class ); - + when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) ) + .thenReturn( Iterators.emptyIterator() ); ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); // when @@ -92,6 +98,7 @@ public void shouldCreateIndexInAnotherTransaction() throws Exception verify( kernel.statements.get( 0 ).txState() ).constraintIndexRuleDoAdd( descriptor ); verifyNoMoreInteractions( indexCreationContext.schemaWriteOperations() ); verify( constraintCreationContext.schemaReadOperations() ).indexGetCommittedId( state, descriptor, CONSTRAINT ); + verify( constraintCreationContext.schemaReadOperations() ).uniqueIndexesGetForLabel( state, labelId ); verifyNoMoreInteractions( constraintCreationContext.schemaReadOperations() ); verify( indexProxy ).awaitStoreScanCompleted(); } @@ -103,7 +110,8 @@ public void shouldDropIndexIfPopulationFails() throws Exception StatementOperationParts constraintCreationContext = mockedParts(); KernelStatement state = mockedState(); - IndexDescriptor descriptor = new IndexDescriptor( 123, 456 ); + int labelId = 123; + IndexDescriptor descriptor = new IndexDescriptor( labelId, 456 ); IndexingService indexingService = mock( IndexingService.class ); StubKernel kernel = new StubKernel(); @@ -116,13 +124,14 @@ public void shouldDropIndexIfPopulationFails() throws Exception doThrow( new IndexPopulationFailedKernelException( descriptor, "some index", cause) ) .when(indexProxy).awaitStoreScanCompleted(); PropertyAccessor propertyAccessor = mock( PropertyAccessor.class ); - + when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) ) + .thenReturn( Iterators.emptyIterator() ); ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); // when try { - creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), 123, 456 ); + creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), labelId, 456 ); fail( "expected exception" ); } @@ -134,12 +143,13 @@ public void shouldDropIndexIfPopulationFails() throws Exception } assertEquals( 2, kernel.statements.size() ); TransactionState tx1 = kernel.statements.get( 0 ).txState(); - verify( tx1 ).constraintIndexRuleDoAdd( new IndexDescriptor( 123, 456 ) ); + verify( tx1 ).constraintIndexRuleDoAdd( new IndexDescriptor( labelId, 456 ) ); verifyNoMoreInteractions( tx1 ); verify( constraintCreationContext.schemaReadOperations() ).indexGetCommittedId( state, descriptor, CONSTRAINT ); + verify( constraintCreationContext.schemaReadOperations() ).uniqueIndexesGetForLabel( state, labelId ); verifyNoMoreInteractions( constraintCreationContext.schemaReadOperations() ); TransactionState tx2 = kernel.statements.get( 1 ).txState(); - verify( tx2 ).constraintIndexDoDrop( new IndexDescriptor( 123, 456 ) ); + verify( tx2 ).constraintIndexDoDrop( new IndexDescriptor( labelId, 456 ) ); verifyNoMoreInteractions( tx2 ); } @@ -171,7 +181,8 @@ public void shouldReleaseSchemaLockWhileAwaitingIndexPopulation() throws Excepti StubKernel kernel = new StubKernel(); IndexingService indexingService = mock( IndexingService.class ); StatementOperationParts constraintCreationContext = mockedParts(); - IndexDescriptor descriptor = new IndexDescriptor( 123, 456 ); + int labelId = 123; + IndexDescriptor descriptor = new IndexDescriptor( labelId, 456 ); PropertyAccessor propertyAccessor = mock( PropertyAccessor.class ); KernelStatement state = mockedState(); @@ -180,17 +191,114 @@ public void shouldReleaseSchemaLockWhileAwaitingIndexPopulation() throws Excepti .thenReturn( 2468L ); IndexProxy indexProxy = mock( IndexProxy.class ); when( indexingService.getIndexProxy( anyLong() ) ).thenReturn( indexProxy ); + when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) ) + .thenReturn( Iterators.emptyIterator() ); ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, true ); // when - creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), 123, 456 ); + creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), labelId, 456 ); // then verify( state.locks().pessimistic() ).releaseExclusive( ResourceTypes.SCHEMA, ResourceTypes.schemaResource() ); verify( state.locks().pessimistic() ).acquireExclusive( ResourceTypes.SCHEMA, ResourceTypes.schemaResource() ); } + @Test + public void shouldReuseExistingOrphanedConstraintIndex() throws Exception + { + // given + StatementOperationParts constraintCreationContext = mockedParts(); + StatementOperationParts indexCreationContext = mockedParts(); + + int labelId = 123; + int propertyKeyId = 456; + IndexDescriptor descriptor = new IndexDescriptor( labelId, propertyKeyId ); + KernelStatement state = mockedState(); + + IndexingService indexingService = mock( IndexingService.class ); + StubKernel kernel = new StubKernel(); + + long orphanedConstraintIndexId = 111; + when( constraintCreationContext.schemaReadOperations().indexGetCommittedId( state, descriptor, CONSTRAINT ) ) + .thenReturn( orphanedConstraintIndexId ); + IndexProxy indexProxy = mock( IndexProxy.class ); + when( indexingService.getIndexProxy( orphanedConstraintIndexId ) ).thenReturn( indexProxy ); + PropertyAccessor propertyAccessor = mock( PropertyAccessor.class ); + when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) ) + .thenReturn( iterator( descriptor ) ); + when( constraintCreationContext.schemaReadOperations().indexGetOwningUniquenessConstraintId( + state, descriptor ) ).thenReturn( null ); // which means it has no owner + ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); + + // when + long indexId = creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), 123, propertyKeyId ); + + // then + assertEquals( orphanedConstraintIndexId, indexId ); + assertEquals( "There should have been no need to acquire a statement to create the constraint index", 0, + kernel.statements.size() ); + verifyNoMoreInteractions( indexCreationContext.schemaWriteOperations() ); + verify( constraintCreationContext.schemaReadOperations() ).indexGetCommittedId( state, descriptor, CONSTRAINT ); + verify( constraintCreationContext.schemaReadOperations() ).uniqueIndexesGetForLabel( state, labelId ); + verify( constraintCreationContext.schemaReadOperations() ) + .indexGetOwningUniquenessConstraintId( state, descriptor ); + verifyNoMoreInteractions( constraintCreationContext.schemaReadOperations() ); + verify( indexProxy ).awaitStoreScanCompleted(); + } + + @Test + public void shouldFailOnExistingOwnedConstraintIndex() throws Exception + { + // given + StatementOperationParts constraintCreationContext = mockedParts(); + StatementOperationParts indexCreationContext = mockedParts(); + + int labelId = 123; + int propertyKeyId = 456; + IndexDescriptor descriptor = new IndexDescriptor( labelId, propertyKeyId ); + KernelStatement state = mockedState(); + + IndexingService indexingService = mock( IndexingService.class ); + StubKernel kernel = new StubKernel(); + + long constraintIndexId = 111; + long constraintIndexOwnerId = 222; + when( constraintCreationContext.schemaReadOperations().indexGetCommittedId( state, descriptor, CONSTRAINT ) ) + .thenReturn( constraintIndexId ); + IndexProxy indexProxy = mock( IndexProxy.class ); + when( indexingService.getIndexProxy( constraintIndexId ) ).thenReturn( indexProxy ); + PropertyAccessor propertyAccessor = mock( PropertyAccessor.class ); + when( constraintCreationContext.schemaReadOperations().uniqueIndexesGetForLabel( state, labelId ) ) + .thenReturn( iterator( descriptor ) ); + when( constraintCreationContext.schemaReadOperations().indexGetOwningUniquenessConstraintId( + state, descriptor ) ).thenReturn( constraintIndexOwnerId ); // which means there's an owner + when( state.readOperations().labelGetName( labelId ) ).thenReturn( "MyLabel" ); + when( state.readOperations().propertyKeyGetName( propertyKeyId ) ).thenReturn( "MyKey" ); + ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); + + // when + try + { + creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), 123, + propertyKeyId ); + fail( "Should've failed" ); + } + catch ( AlreadyConstrainedException e ) + { + // THEN good + } + + // then + assertEquals( "There should have been no need to acquire a statement to create the constraint index", 0, + kernel.statements.size() ); + verifyNoMoreInteractions( indexCreationContext.schemaWriteOperations() ); + verify( constraintCreationContext.schemaReadOperations() ).uniqueIndexesGetForLabel( state, labelId ); + verify( constraintCreationContext.schemaReadOperations() ) + .indexGetOwningUniquenessConstraintId( state, descriptor ); + verifyNoMoreInteractions( constraintCreationContext.schemaReadOperations() ); + } + private class StubKernel implements KernelAPI { private final List statements = new ArrayList<>(); diff --git a/community/lucene-index/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintRecoveryIT.java b/community/lucene-index/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintRecoveryIT.java index bad5294f403e4..a077b1b326a0a 100644 --- a/community/lucene-index/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintRecoveryIT.java +++ b/community/lucene-index/src/test/java/org/neo4j/kernel/impl/api/constraints/ConstraintRecoveryIT.java @@ -32,6 +32,7 @@ import org.neo4j.graphdb.Node; import org.neo4j.graphdb.Transaction; import org.neo4j.graphdb.mockfs.EphemeralFileSystemAbstraction; +import org.neo4j.graphdb.schema.IndexDefinition; import org.neo4j.helpers.collection.Iterables; import org.neo4j.kernel.impl.api.index.IndexingService; import org.neo4j.kernel.impl.storageengine.impl.recordstorage.RecordStorageEngine; @@ -44,16 +45,20 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.neo4j.helpers.collection.Iterables.single; + public class ConstraintRecoveryIT { + private static final String KEY = "prop"; + private static final Label LABEL = Label.label( "label1" ); + @Rule public EphemeralFileSystemRule fileSystemRule = new EphemeralFileSystemRule(); - private static final Label LABEL = Label.label( "label1" ); private GraphDatabaseAPI db; @Test - public void shouldNotHaveAnIndexIfUniqueConstraintCreationOnRecoveryFails() throws IOException + public void shouldHaveAvailableOrphanedConstraintIndexIfUniqueConstraintCreationFails() throws IOException { // given final EphemeralFileSystemAbstraction fs = fileSystemRule.get(); @@ -87,7 +92,7 @@ public void indexPopulationScanComplete() for ( int i = 0; i < 2; i++ ) { Node node1 = db.createNode( LABEL ); - node1.setProperty( "prop", true ); + node1.setProperty( KEY, true ); } tx.success(); @@ -95,7 +100,7 @@ public void indexPopulationScanComplete() try ( Transaction tx = db.beginTx() ) { - db.schema().constraintFor( LABEL ).assertPropertyIsUnique( "prop" ).create(); + db.schema().constraintFor( LABEL ).assertPropertyIsUnique( KEY ).create(); fail("Should have failed with ConstraintViolationException"); tx.success(); } @@ -128,7 +133,9 @@ public void indexPopulationScanComplete() try(Transaction tx = db.beginTx()) { - assertEquals(0, Iterables.count(Iterables.asList( db.schema().getIndexes() ))); + IndexDefinition orphanedConstraintIndex = single( db.schema().getIndexes() ); + assertEquals( LABEL.name(), orphanedConstraintIndex.getLabel().name() ); + assertEquals( KEY, single( orphanedConstraintIndex.getPropertyKeys() ) ); } db.shutdown();