From 29fe48db8de7becd3777fb56ef386ce00dc6f4b7 Mon Sep 17 00:00:00 2001 From: MishaDemianenko Date: Mon, 5 Jun 2017 22:09:06 +0200 Subject: [PATCH] Split global schema lock Split global schema lock into independent label and relationship type locks that will be used depending from type of index, constraint etc. Completely remove SCHEMA resource from type of resources that can be locked. Slaves in ha now will take labels and rel type locks on master exactly like for nodes and relationship locks. This commit performs split in kernel/ha code. Cypher code not updated as part of this commit. --- .../factory/GraphDatabaseSettings.java | 5 - .../org/neo4j/kernel/NeoStoreDataSource.java | 7 +- .../api/schema/LabelSchemaDescriptor.java | 14 ++ .../schema/RelationTypeSchemaDescriptor.java | 14 ++ .../kernel/api/schema/SchemaDescriptor.java | 20 +++ .../ConstraintEnforcingEntityOperations.java | 3 + .../impl/api/KernelSchemaStateStore.java | 4 + .../impl/api/LockingStatementOperations.java | 148 ++++++++---------- .../kernel/impl/api/SchemaStateConcern.java | 6 - .../api/operations/SchemaStateOperations.java | 5 - .../api/state/ConstraintIndexCreator.java | 65 +++----- .../kernel/impl/locking/ResourceTypes.java | 15 +- .../api/LockingStatementOperationsTest.java | 112 +++++-------- .../ConstraintIndexCreatorTest.java | 21 ++- .../ActiveLocksListingCompatibility.java | 10 +- .../CommunityLockAcquisitionTimeoutIT.java | 24 ++- .../impl/locking/DeferringLockClientTest.java | 10 +- .../kernel/impl/locking/LockUnitTest.java | 4 +- .../ha/MasterTransactionCommitProcess.java | 103 +----------- .../modeswitch/CommitProcessSwitcher.java | 12 +- .../kernel/ha/com/master/MasterImpl.java | 24 +-- .../factory/HighlyAvailableEditionModule.java | 3 +- .../kernel/ha/lock/SlaveLocksClient.java | 43 ++--- .../ha/PropertyConstraintsStressIT.java | 18 +-- .../MasterImplConversationStopFuzzIT.java | 4 +- .../kernel/ha/com/master/MasterImplTest.java | 3 - .../lock/forseti/ForsetiLockManager.java | 8 - .../neo4j/management/TestLockManagerBean.java | 11 +- .../EnterpriseLockAcquisitionTimeoutIT.java | 8 + .../StoreUpgradeIntegrationTest.java | 12 +- 30 files changed, 273 insertions(+), 463 deletions(-) diff --git a/community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java b/community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java index 18d9f2eb09f24..7e107890fcc8f 100644 --- a/community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java +++ b/community/kernel/src/main/java/org/neo4j/graphdb/factory/GraphDatabaseSettings.java @@ -586,11 +586,6 @@ public enum LabelIndex public static final Setting procedure_unrestricted = setting( "dbms.security.procedures.unrestricted", Settings.STRING, "" ); - @Description( "Whether or not to release the exclusive schema lock is while building uniqueness constraints index" ) - @Internal - public static final Setting release_schema_lock_while_building_constraint = setting( - "unsupported.dbms.schema.release_lock_while_building_constraint", BOOLEAN, FALSE ); - @Description( "A list of procedures (comma separated) that are to be loaded. " + "The list may contain both fully-qualified procedure names, and partial names with the wildcard '*'. " + "If this setting is left empty no procedures will be loaded." ) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java b/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java index 01f319c14011a..c6fb589674932 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/NeoStoreDataSource.java @@ -725,11 +725,8 @@ private NeoStoreKernelModule buildKernel( TransactionAppender appender, */ Supplier kernelProvider = () -> kernelModule.kernelAPI(); - boolean releaseSchemaLockWhenBuildingConstratinIndexes = - config.get( GraphDatabaseSettings.release_schema_lock_while_building_constraint ); - ConstraintIndexCreator constraintIndexCreator = - new ConstraintIndexCreator( kernelProvider, indexingService, propertyAccessor, - releaseSchemaLockWhenBuildingConstratinIndexes ); + ConstraintIndexCreator constraintIndexCreator = new ConstraintIndexCreator( kernelProvider, indexingService, + propertyAccessor ); LegacyIndexStore legacyIndexStore = new LegacyIndexStore( config, indexConfigStore, kernelProvider, legacyIndexProviderLookup ); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/schema/LabelSchemaDescriptor.java b/community/kernel/src/main/java/org/neo4j/kernel/api/schema/LabelSchemaDescriptor.java index 557987251dd88..7b7dd0c0733e5 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/schema/LabelSchemaDescriptor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/schema/LabelSchemaDescriptor.java @@ -22,6 +22,8 @@ import java.util.Arrays; import org.neo4j.kernel.api.TokenNameLookup; +import org.neo4j.kernel.impl.locking.ResourceTypes; +import org.neo4j.storageengine.api.lock.ResourceType; public class LabelSchemaDescriptor implements SchemaDescriptor, LabelSchemaSupplier { @@ -58,6 +60,18 @@ public int getLabelId() return labelId; } + @Override + public int keyId() + { + return getLabelId(); + } + + @Override + public ResourceType keyType() + { + return ResourceTypes.LABEL; + } + @Override public int[] getPropertyIds() { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/schema/RelationTypeSchemaDescriptor.java b/community/kernel/src/main/java/org/neo4j/kernel/api/schema/RelationTypeSchemaDescriptor.java index f703c9ea59c11..232d051434e11 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/schema/RelationTypeSchemaDescriptor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/schema/RelationTypeSchemaDescriptor.java @@ -22,6 +22,8 @@ import java.util.Arrays; import org.neo4j.kernel.api.TokenNameLookup; +import org.neo4j.kernel.impl.locking.ResourceTypes; +import org.neo4j.storageengine.api.lock.ResourceType; public class RelationTypeSchemaDescriptor implements SchemaDescriptor { @@ -64,6 +66,18 @@ public int[] getPropertyIds() return propertyIds; } + @Override + public int keyId() + { + return getRelTypeId(); + } + + @Override + public ResourceType keyType() + { + return ResourceTypes.RELATIONSHIP_TYPE; + } + public int getPropertyId() { if ( propertyIds.length != 1 ) diff --git a/community/kernel/src/main/java/org/neo4j/kernel/api/schema/SchemaDescriptor.java b/community/kernel/src/main/java/org/neo4j/kernel/api/schema/SchemaDescriptor.java index be020b0a69063..a4a04bfa4ee71 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/api/schema/SchemaDescriptor.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/api/schema/SchemaDescriptor.java @@ -22,6 +22,8 @@ import java.util.function.Predicate; import org.neo4j.kernel.api.TokenNameLookup; +import org.neo4j.kernel.impl.locking.ResourceTypes; +import org.neo4j.storageengine.api.lock.ResourceType; /** * Internal representation of one schema unit, for example a label-property pair. @@ -71,6 +73,24 @@ public interface SchemaDescriptor */ int[] getPropertyIds(); + /** + * Id of underlying schema descriptor key. + * Key is part of schema unit that determines which resources with specified properties are applicable. + * So far {@link ResourceTypes#LABEL label} and {@link ResourceTypes#RELATIONSHIP_TYPE relationship type} + * are supported as keys types. + * @return id of underlying key + */ + int keyId(); + + /** + * Type of underlying schema descriptor key. + * Key is part of schema unit that determines which resources with specified properties are applicable. + * So far {@link ResourceTypes#LABEL label} and {@link ResourceTypes#RELATIONSHIP_TYPE relationship type} + * are supported as keys types. + * @return type of underlying key + */ + ResourceType keyType(); + /** * Create a predicate that checks whether a schema descriptor Supplier supplies the given schema descriptor. * @param descriptor The schema descriptor to check equality with. diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/ConstraintEnforcingEntityOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/ConstraintEnforcingEntityOperations.java index fed6b8696d315..4df895e55fec7 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/ConstraintEnforcingEntityOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/ConstraintEnforcingEntityOperations.java @@ -70,6 +70,7 @@ import org.neo4j.kernel.impl.constraints.ConstraintSemantics; import org.neo4j.kernel.impl.locking.LockTracer; import org.neo4j.kernel.impl.locking.Locks; +import org.neo4j.kernel.impl.locking.ResourceTypes; import org.neo4j.storageengine.api.Direction; import org.neo4j.storageengine.api.NodeItem; import org.neo4j.storageengine.api.PropertyItem; @@ -153,6 +154,8 @@ public Value nodeSetProperty( KernelStatement state, long nodeId, int propertyKe nodeSchemaMatcher.onMatchingSchema( state, uniquenessConstraints, node, propertyKeyId, ( constraint, propertyIds ) -> { + state.locks().optimistic().acquireShared( state.lockTracer(), ResourceTypes.LABEL, + constraint.schema().getLabelId() ); if ( propertyIds.contains( propertyKeyId ) ) { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelSchemaStateStore.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelSchemaStateStore.java index 12c9c81cecd97..fc3c636ecbb08 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelSchemaStateStore.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/KernelSchemaStateStore.java @@ -47,6 +47,7 @@ public KernelSchemaStateStore( LogProvider logProvider ) } @SuppressWarnings( "unchecked" ) + @Override public V get( K key ) { lock.readLock().lock(); @@ -93,6 +94,7 @@ public V getOrCreate( K key, Function creator ) } } + @Override public void replace( Map replacement ) { lock.writeLock().lock(); @@ -106,6 +108,7 @@ public void replace( Map replacement ) } } + @Override public void apply( Map updates ) { lock.writeLock().lock(); @@ -119,6 +122,7 @@ public void apply( Map updates ) } } + @Override public void clear() { lock.writeLock().lock(); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java index 7e7855e89abf8..49fe91d433e09 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/LockingStatementOperations.java @@ -24,6 +24,7 @@ import java.util.Iterator; import java.util.function.Function; +import org.neo4j.helpers.collection.Iterators; import org.neo4j.kernel.api.Statement; import org.neo4j.kernel.api.exceptions.EntityNotFoundException; import org.neo4j.kernel.api.exceptions.InvalidTransactionTypeKernelException; @@ -61,8 +62,6 @@ import static java.lang.Math.max; import static java.lang.Math.min; -import static org.neo4j.kernel.impl.locking.ResourceTypes.schemaResource; -import static org.neo4j.unsafe.impl.internal.dragons.FeatureToggles.flag; public class LockingStatementOperations implements EntityWriteOperations, @@ -71,9 +70,6 @@ public class LockingStatementOperations implements SchemaStateOperations, LockOperations { - private static final boolean SCHEMA_WRITES_DISABLE = - flag( LockingStatementOperations.class, "schemaWritesDisable", false ); - private final EntityReadOperations entityReadDelegate; private final EntityWriteOperations entityWriteDelegate; private final SchemaReadOperations schemaReadDelegate; @@ -98,18 +94,7 @@ public LockingStatementOperations( public boolean nodeAddLabel( KernelStatement state, long nodeId, int labelId ) throws ConstraintValidationException, EntityNotFoundException { - // TODO (BBC, 22/11/13): - // In order to enforce constraints we need to check whether this change violates constraints; we therefore need - // the schema lock to ensure that our view of constraints is consistent. - // - // We would like this locking to be done naturally when ConstraintEnforcingEntityOperations calls - // SchemaReadOperations#constraintsGetForLabel, but the SchemaReadOperations object that - // ConstraintEnforcingEntityOperations has a reference to does not lock because of the way the cake is - // constructed. - // - // It would be cleaner if the schema and data cakes were separated so that the SchemaReadOperations object used - // by ConstraintEnforcingEntityOperations included the full cake, with locking included. - acquireSharedSchemaLock( state ); + sharedLabelLock( state, labelId ); acquireExclusiveNodeLock( state, nodeId ); state.assertOpen(); @@ -129,7 +114,7 @@ public boolean nodeRemoveLabel( KernelStatement state, long nodeId, int labelId public IndexDescriptor indexCreate( KernelStatement state, LabelSchemaDescriptor descriptor ) throws AlreadyIndexedException, AlreadyConstrainedException, RepeatedPropertyInCompositeSchemaException { - acquireExclusiveSchemaLock( state ); + exclusiveLabelLock( state, descriptor.getLabelId() ); state.assertOpen(); return schemaWriteDelegate.indexCreate( state, descriptor ); } @@ -137,7 +122,7 @@ public IndexDescriptor indexCreate( KernelStatement state, LabelSchemaDescriptor @Override public void indexDrop( KernelStatement state, IndexDescriptor descriptor ) throws DropIndexFailureException { - acquireExclusiveSchemaLock( state ); + exclusiveLabelLock( state, descriptor.schema().getLabelId() ); state.assertOpen(); schemaWriteDelegate.indexDrop( state, descriptor ); } @@ -145,7 +130,7 @@ public void indexDrop( KernelStatement state, IndexDescriptor descriptor ) throw @Override public void uniqueIndexDrop( KernelStatement state, IndexDescriptor descriptor ) throws DropIndexFailureException { - acquireExclusiveSchemaLock( state ); + exclusiveLabelLock( state, descriptor.schema().getLabelId() ); state.assertOpen(); schemaWriteDelegate.uniqueIndexDrop( state, descriptor ); } @@ -153,23 +138,13 @@ public void uniqueIndexDrop( KernelStatement state, IndexDescriptor descriptor ) @Override public V schemaStateGetOrCreate( KernelStatement state, K key, Function creator ) { - acquireSharedSchemaLock( state ); state.assertOpen(); return schemaStateDelegate.schemaStateGetOrCreate( state, key, creator ); } - @Override - public boolean schemaStateContains( KernelStatement state, K key ) - { - acquireSharedSchemaLock( state ); - state.assertOpen(); - return schemaStateDelegate.schemaStateContains( state, key ); - } - @Override public void schemaStateFlush( KernelStatement state ) { - acquireSharedSchemaLock( state ); state.assertOpen(); schemaStateDelegate.schemaStateFlush( state ); } @@ -177,7 +152,7 @@ public void schemaStateFlush( KernelStatement state ) @Override public Iterator indexesGetForLabel( KernelStatement state, int labelId ) { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, labelId ); state.assertOpen(); return schemaReadDelegate.indexesGetForLabel( state, labelId ); } @@ -185,7 +160,7 @@ public Iterator indexesGetForLabel( KernelStatement state, int @Override public IndexDescriptor indexGetForSchema( KernelStatement state, LabelSchemaDescriptor descriptor ) { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, descriptor.getLabelId() ); state.assertOpen(); return schemaReadDelegate.indexGetForSchema( state, descriptor ); } @@ -193,16 +168,19 @@ public IndexDescriptor indexGetForSchema( KernelStatement state, LabelSchemaDesc @Override public Iterator indexesGetAll( KernelStatement state ) { - acquireSharedSchemaLock( state ); state.assertOpen(); - return schemaReadDelegate.indexesGetAll( state ); + return Iterators.map( indexDescriptor -> + { + sharedLabelLock( state, indexDescriptor.schema().getLabelId() ); + return indexDescriptor; + }, schemaReadDelegate.indexesGetAll( state ) ); } @Override public InternalIndexState indexGetState( KernelStatement state, IndexDescriptor descriptor ) throws IndexNotFoundKernelException { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, descriptor.schema().getLabelId() ); state.assertOpen(); return schemaReadDelegate.indexGetState( state, descriptor ); } @@ -211,7 +189,7 @@ public InternalIndexState indexGetState( KernelStatement state, IndexDescriptor public PopulationProgress indexGetPopulationProgress( KernelStatement state, IndexDescriptor descriptor ) throws IndexNotFoundKernelException { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, descriptor.schema().getLabelId() ); state.assertOpen(); return schemaReadDelegate.indexGetPopulationProgress( state, descriptor ); } @@ -219,7 +197,7 @@ public PopulationProgress indexGetPopulationProgress( KernelStatement state, Ind @Override public long indexSize( KernelStatement state, IndexDescriptor descriptor ) throws IndexNotFoundKernelException { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, descriptor.schema().getLabelId() ); state.assertOpen(); return schemaReadDelegate.indexSize( state, descriptor ); } @@ -228,7 +206,7 @@ public long indexSize( KernelStatement state, IndexDescriptor descriptor ) throw public double indexUniqueValuesPercentage( KernelStatement state, IndexDescriptor descriptor ) throws IndexNotFoundKernelException { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, descriptor.schema().getLabelId() ); state.assertOpen(); return schemaReadDelegate.indexUniqueValuesPercentage( state, descriptor ); } @@ -236,7 +214,7 @@ public double indexUniqueValuesPercentage( KernelStatement state, @Override public Long indexGetOwningUniquenessConstraintId( KernelStatement state, IndexDescriptor index ) { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, index.schema().getLabelId() ); state.assertOpen(); return schemaReadDelegate.indexGetOwningUniquenessConstraintId( state, index ); } @@ -245,7 +223,7 @@ public Long indexGetOwningUniquenessConstraintId( KernelStatement state, IndexDe public long indexGetCommittedId( KernelStatement state, IndexDescriptor index ) throws SchemaRuleNotFoundException { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, index.schema().getLabelId() ); state.assertOpen(); return schemaReadDelegate.indexGetCommittedId( state, index ); } @@ -297,7 +275,7 @@ public long relationshipCreate( KernelStatement state, long endNodeId ) throws EntityNotFoundException { - acquireSharedSchemaLock( state ); + sharedRelationshipTypeLock( state, relationshipTypeId ); lockRelationshipNodes( state, startNodeId, endNodeId ); return entityWriteDelegate.relationshipCreate( state, relationshipTypeId, startNodeId, endNodeId ); } @@ -324,21 +302,21 @@ private void lockRelationshipNodes( KernelStatement state, long startNodeId, lon } @Override - public NodeKeyConstraintDescriptor nodeKeyConstraintCreate( KernelStatement state,LabelSchemaDescriptor descriptor ) + public NodeKeyConstraintDescriptor nodeKeyConstraintCreate( KernelStatement state, LabelSchemaDescriptor descriptor ) throws CreateConstraintFailureException, AlreadyConstrainedException, AlreadyIndexedException, RepeatedPropertyInCompositeSchemaException { - acquireExclusiveSchemaLock( state ); + exclusiveLabelLock( state, descriptor.getLabelId() ); state.assertOpen(); return schemaWriteDelegate.nodeKeyConstraintCreate( state, descriptor ); } @Override - public UniquenessConstraintDescriptor uniquePropertyConstraintCreate( KernelStatement state,LabelSchemaDescriptor descriptor ) + public UniquenessConstraintDescriptor uniquePropertyConstraintCreate( KernelStatement state, LabelSchemaDescriptor descriptor ) throws CreateConstraintFailureException, AlreadyConstrainedException, AlreadyIndexedException, RepeatedPropertyInCompositeSchemaException { - acquireExclusiveSchemaLock( state ); + exclusiveLabelLock( state, descriptor.getLabelId() ); state.assertOpen(); return schemaWriteDelegate.uniquePropertyConstraintCreate( state, descriptor ); } @@ -348,7 +326,7 @@ public NodeExistenceConstraintDescriptor nodePropertyExistenceConstraintCreate( LabelSchemaDescriptor descriptor ) throws AlreadyConstrainedException, CreateConstraintFailureException, RepeatedPropertyInCompositeSchemaException { - acquireExclusiveSchemaLock( state ); + exclusiveLabelLock( state, descriptor.getLabelId() ); state.assertOpen(); return schemaWriteDelegate.nodePropertyExistenceConstraintCreate( state, descriptor ); } @@ -359,7 +337,7 @@ public RelExistenceConstraintDescriptor relationshipPropertyExistenceConstraintC throws AlreadyConstrainedException, CreateConstraintFailureException, RepeatedPropertyInCompositeSchemaException { - acquireExclusiveSchemaLock( state ); + exclusiveRelationshipTypeLock( state, descriptor.getRelTypeId() ); state.assertOpen(); return schemaWriteDelegate.relationshipPropertyExistenceConstraintCreate( state, descriptor ); } @@ -367,7 +345,7 @@ public RelExistenceConstraintDescriptor relationshipPropertyExistenceConstraintC @Override public Iterator constraintsGetForSchema( KernelStatement state, SchemaDescriptor descriptor ) { - acquireSharedSchemaLock( state ); + sharedOptimisticLock( state, descriptor.keyType(), descriptor.keyId() ); state.assertOpen(); return schemaReadDelegate.constraintsGetForSchema( state, descriptor ); } @@ -375,7 +353,8 @@ public Iterator constraintsGetForSchema( KernelStatement s @Override public boolean constraintExists( KernelStatement state, ConstraintDescriptor descriptor ) { - acquireSharedSchemaLock( state ); + SchemaDescriptor schema = descriptor.schema(); + sharedOptimisticLock( state, schema.keyType(), schema.keyId() ); state.assertOpen(); return schemaReadDelegate.constraintExists( state, descriptor ); } @@ -383,7 +362,7 @@ public boolean constraintExists( KernelStatement state, ConstraintDescriptor des @Override public Iterator constraintsGetForLabel( KernelStatement state, int labelId ) { - acquireSharedSchemaLock( state ); + sharedLabelLock( state, labelId ); state.assertOpen(); return schemaReadDelegate.constraintsGetForLabel( state, labelId ); } @@ -392,7 +371,7 @@ public Iterator constraintsGetForLabel( KernelStatement st public Iterator constraintsGetForRelationshipType( KernelStatement state, int typeId ) { - acquireSharedSchemaLock( state ); + sharedRelationshipTypeLock( state, typeId ); state.assertOpen(); return schemaReadDelegate.constraintsGetForRelationshipType( state, typeId ); } @@ -400,16 +379,21 @@ public Iterator constraintsGetForRelationshipType( KernelS @Override public Iterator constraintsGetAll( KernelStatement state ) { - acquireSharedSchemaLock( state ); state.assertOpen(); - return schemaReadDelegate.constraintsGetAll( state ); + return Iterators.map( constraintDescriptor -> + { + SchemaDescriptor schema = constraintDescriptor.schema(); + acquireShared( state, schema.keyType(), schema.keyId() ); + return constraintDescriptor; + }, schemaReadDelegate.constraintsGetAll( state ) ); } @Override public void constraintDrop( KernelStatement state, ConstraintDescriptor constraint ) throws DropConstraintFailureException { - acquireExclusiveSchemaLock( state ); + SchemaDescriptor schema = constraint.schema(); + exclusiveOptimisticLock( state, schema.keyType(), schema.keyId() ); state.assertOpen(); schemaWriteDelegate.constraintDrop( state, constraint ); } @@ -419,19 +403,6 @@ public Value nodeSetProperty( KernelStatement state, long nodeId, int propertyKe throws ConstraintValidationException, EntityNotFoundException, AutoIndexingKernelException, InvalidTransactionTypeKernelException { - // TODO (BBC, 22/11/13): - // In order to enforce constraints we need to check whether this change violates constraints; we therefore need - // the schema lock to ensure that our view of constraints is consistent. - // - // We would like this locking to be done naturally when ConstraintEnforcingEntityOperations calls - // SchemaReadOperations#constraintsGetForLabel, but the SchemaReadOperations object that - // ConstraintEnforcingEntityOperations has a reference to does not lock because of the way the cake is - // constructed. - // - // It would be cleaner if the schema and data cakes were separated so that the SchemaReadOperations object used - // by ConstraintEnforcingEntityOperations included the full cake, with locking included. - acquireSharedSchemaLock( state ); - acquireExclusiveNodeLock( state, nodeId ); state.assertOpen(); return entityWriteDelegate.nodeSetProperty( state, nodeId, propertyKeyId, value ); @@ -508,7 +479,6 @@ public void releaseShared( KernelStatement state, ResourceType type, long resour state.assertOpen(); } - // === TODO Below is unnecessary delegate methods @Override public String indexGetFailure( Statement state, IndexDescriptor descriptor ) throws IndexNotFoundKernelException @@ -520,7 +490,7 @@ private void acquireExclusiveNodeLock( KernelStatement state, long nodeId ) { if ( !state.hasTxStateWithChanges() || !state.txState().nodeIsAddedInThisTx( nodeId ) ) { - state.locks().optimistic().acquireExclusive( state.lockTracer(), ResourceTypes.NODE, nodeId ); + exclusiveOptimisticLock( state, ResourceTypes.NODE, nodeId ); } } @@ -528,27 +498,37 @@ private void acquireExclusiveRelationshipLock( KernelStatement state, long relat { if ( !state.hasTxStateWithChanges() || !state.txState().relationshipIsAddedInThisTx( relationshipId ) ) { - state.locks().optimistic().acquireExclusive( state.lockTracer(), ResourceTypes.RELATIONSHIP, relationshipId ); + exclusiveOptimisticLock( state, ResourceTypes.RELATIONSHIP, relationshipId ); } } - private void acquireSharedSchemaLock( KernelStatement state ) + private void exclusiveLabelLock( KernelStatement state, long labelId ) { - if ( !SCHEMA_WRITES_DISABLE ) - { - state.locks().optimistic().acquireShared( state.lockTracer(), ResourceTypes.SCHEMA, schemaResource() ); - } + exclusiveOptimisticLock( state, ResourceTypes.LABEL, labelId ); } - private void acquireExclusiveSchemaLock( KernelStatement state ) + private void sharedLabelLock( KernelStatement state, long labelId ) { - if ( SCHEMA_WRITES_DISABLE ) - { - throw new IllegalStateException( "Schema modifications have been disabled via feature toggle" ); - } - else - { - state.locks().optimistic().acquireExclusive( state.lockTracer(), ResourceTypes.SCHEMA, schemaResource() ); - } + sharedOptimisticLock( state, ResourceTypes.LABEL, labelId ); + } + + private void exclusiveRelationshipTypeLock( KernelStatement state, long typeId ) + { + exclusiveOptimisticLock( state, ResourceTypes.RELATIONSHIP_TYPE, typeId ); + } + + private void sharedRelationshipTypeLock( KernelStatement state, long typeId ) + { + sharedOptimisticLock( state, ResourceTypes.RELATIONSHIP_TYPE, typeId ); + } + + private void sharedOptimisticLock( KernelStatement statement, ResourceType resource, long resourceId ) + { + statement.locks().optimistic().acquireShared( statement.lockTracer(), resource, resourceId ); + } + + private void exclusiveOptimisticLock( KernelStatement statement, ResourceType resource, long resourceId ) + { + statement.locks().optimistic().acquireExclusive( statement.lockTracer(), resource, resourceId ); } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/SchemaStateConcern.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/SchemaStateConcern.java index 841d2383cadf1..c1f8ce30ff2f4 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/SchemaStateConcern.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/SchemaStateConcern.java @@ -38,12 +38,6 @@ public V schemaStateGetOrCreate( KernelStatement state, K key, Function boolean schemaStateContains( KernelStatement state, K key ) - { - return schemaState.get( key ) != null; - } - @Override public void schemaStateFlush( KernelStatement state ) { diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/operations/SchemaStateOperations.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/operations/SchemaStateOperations.java index b6e9d0a0a64ae..ae7edd5d35faa 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/api/operations/SchemaStateOperations.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/api/operations/SchemaStateOperations.java @@ -38,11 +38,6 @@ public interface SchemaStateOperations */ V schemaStateGetOrCreate( KernelStatement state, K key, Function creator ); - /** - * Check if some key is in the schema state. - */ - boolean schemaStateContains( KernelStatement state, K key ); - /** * Flush the schema state. */ 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 2259dfcd3476a..53edc40a01a38 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 @@ -30,14 +30,14 @@ 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.AlreadyIndexedException; import org.neo4j.kernel.api.exceptions.schema.CreateConstraintFailureException; import org.neo4j.kernel.api.exceptions.schema.DropIndexFailureException; import org.neo4j.kernel.api.exceptions.schema.SchemaKernelException; +import org.neo4j.kernel.api.exceptions.schema.SchemaKernelException.OperationContext; import org.neo4j.kernel.api.exceptions.schema.SchemaRuleNotFoundException; import org.neo4j.kernel.api.exceptions.schema.UniquePropertyValueValidationException; -import org.neo4j.kernel.api.exceptions.schema.AlreadyConstrainedException; -import org.neo4j.kernel.api.exceptions.schema.SchemaKernelException.OperationContext; import org.neo4j.kernel.api.index.PropertyAccessor; import org.neo4j.kernel.api.schema.LabelSchemaDescriptor; import org.neo4j.kernel.api.schema.constaints.ConstraintDescriptorFactory; @@ -53,38 +53,35 @@ import static org.neo4j.kernel.api.exceptions.schema.ConstraintValidationException.Phase.VERIFICATION; import static org.neo4j.kernel.api.exceptions.schema.SchemaKernelException.OperationContext.CONSTRAINT_CREATION; import static org.neo4j.kernel.api.security.SecurityContext.AUTH_DISABLED; -import static org.neo4j.kernel.impl.locking.ResourceTypes.SCHEMA; -import static org.neo4j.kernel.impl.locking.ResourceTypes.schemaResource; +import static org.neo4j.kernel.impl.locking.ResourceTypes.LABEL; public class ConstraintIndexCreator { private final IndexingService indexingService; private final Supplier kernelSupplier; private final PropertyAccessor propertyAccessor; - private final boolean releaseSchemaLockWhenCreatingConstraint; public ConstraintIndexCreator( Supplier kernelSupplier, IndexingService indexingService, - PropertyAccessor propertyAccessor, boolean releaseSchemaLockWhenCreatingConstraint ) + PropertyAccessor propertyAccessor ) { this.kernelSupplier = kernelSupplier; this.indexingService = indexingService; this.propertyAccessor = propertyAccessor; - this.releaseSchemaLockWhenCreatingConstraint = releaseSchemaLockWhenCreatingConstraint; } /** - * You MUST hold a schema write lock before you call this method. - * However the schema write lock is temporarily released while populating the index backing the constraint. + * You MUST hold a label write lock before you call this method. + * However the label write lock is temporarily released while populating the index backing the constraint. * It goes a little like this: *
    *
  1. Prerequisite: Getting here means that there's an open schema transaction which has acquired the - * SCHEMA WRITE lock.
    2. + * LABEL WRITE lock. *
  2. Index schema rule which is backing the constraint is created in a nested mini-transaction * which doesn't acquire any locking, merely adds tx state and commits so that the index rule is applied * to the store, which triggers the index population
    3. - *
  3. Release the SCHEMA WRITE lock
    4. + *
  4. Release the LABEL WRITE lock
    5. *
  5. Await index population to complete
    6. - *
  6. Acquire the SCHEMA WRITE lock (effectively blocking concurrent transactions changing + *
  7. Acquire the LABEL WRITE lock (effectively blocking concurrent transactions changing * data related to this constraint, and it so happens, most other transactions as well) and verify * the uniqueness of the built index
    8. *
  8. Leave this method, knowing that the uniqueness constraint rule will be added to tx state @@ -112,26 +109,26 @@ public long createUniquenessConstraintIndex( } boolean success = false; - boolean reacquiredSchemaLock = false; + boolean reacquiredLabelLock = false; Client locks = state.locks().pessimistic(); try { long indexId = schemaOps.indexGetCommittedId( state, index ); - // Release the SCHEMA WRITE lock during index population. + // Release the LABEL WRITE lock during index population. // At this point the integrity of the constraint to be created was checked // while holding the lock and the index rule backing the soon-to-be-created constraint // has been created. Now it's just the population left, which can take a long time - releaseSchemaLock( locks ); + releaseLabelLock( locks, descriptor.getLabelId() ); awaitConstrainIndexPopulation( constraint, indexId ); // Index population was successful, but at this point we don't know if the uniqueness constraint holds. - // Acquire SCHEMA WRITE lock and verify the constraints here in this user transaction + // Acquire LABEL WRITE lock and verify the constraints here in this user transaction // and if everything checks out then it will be held until after the constraint has been // created and activated. - acquireSchemaLock( state, locks ); - reacquiredSchemaLock = true; + acquireLabelLock( state, locks, descriptor.getLabelId() ); + reacquiredLabelLock = true; indexingService.getIndexProxy( indexId ).verifyDeferredConstraints( propertyAccessor ); success = true; return indexId; @@ -153,29 +150,23 @@ public long createUniquenessConstraintIndex( { if ( !success ) { - if ( !reacquiredSchemaLock ) + if ( !reacquiredLabelLock ) { - acquireSchemaLock( state, locks ); + acquireLabelLock( state, locks, descriptor.getLabelId() ); } dropUniquenessConstraintIndex( index ); } } } - private void acquireSchemaLock( KernelStatement state, Client locks ) + private void acquireLabelLock( KernelStatement state, Client locks, int labelId ) { - if ( releaseSchemaLockWhenCreatingConstraint ) - { - locks.acquireExclusive( state.lockTracer(), SCHEMA, schemaResource() ); - } + locks.acquireExclusive( state.lockTracer(), LABEL, labelId ); } - private void releaseSchemaLock( Client locks ) + private void releaseLabelLock( Client locks, int labelId ) { - if ( releaseSchemaLockWhenCreatingConstraint ) - { - locks.releaseExclusive( SCHEMA, schemaResource() ); - } + locks.releaseExclusive( LABEL, labelId ); } /** @@ -188,13 +179,6 @@ public void dropUniquenessConstraintIndex( IndexDescriptor descriptor ) kernelSupplier.get().newTransaction( KernelTransaction.Type.implicit, AUTH_DISABLED ); Statement statement = transaction.acquireStatement() ) { - // NOTE: This creates the index (obviously) but it DOES NOT grab a schema - // write lock. It is assumed that the transaction that invoked this "inner" transaction - // holds a schema write lock, and that it will wait for this inner transaction to do its - // work. - // TODO (Ben+Jake): The Transactor is really part of the kernel internals, so it needs access to the - // internal implementation of Statement. However it is currently used by the external - // RemoveOrphanConstraintIndexesOnStartup job. This needs revisiting. ((KernelStatement) statement).txState().indexDoDrop( descriptor ); transaction.success(); } @@ -260,13 +244,6 @@ public IndexDescriptor createConstraintIndex( final LabelSchemaDescriptor schema kernelSupplier.get().newTransaction( KernelTransaction.Type.implicit, AUTH_DISABLED ); Statement statement = transaction.acquireStatement() ) { - // NOTE: This creates the index (obviously) but it DOES NOT grab a schema - // write lock. It is assumed that the transaction that invoked this "inner" transaction - // holds a schema write lock, and that it will wait for this inner transaction to do its - // work. - // TODO (Ben+Jake): The Transactor is really part of the kernel internals, so it needs access to the - // internal implementation of Statement. However it is currently used by the external - // RemoveOrphanConstraintIndexesOnStartup job. This needs revisiting. IndexDescriptor index = IndexDescriptorFactory.uniqueForSchema( schema ); ((KernelStatement) statement).txState().indexRuleDoAdd( index ); transaction.success(); diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/locking/ResourceTypes.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/locking/ResourceTypes.java index 00958263c9354..84f757f66047e 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/locking/ResourceTypes.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/locking/ResourceTypes.java @@ -37,9 +37,10 @@ public enum ResourceTypes implements ResourceType NODE( 0, LockWaitStrategies.INCREMENTAL_BACKOFF ), RELATIONSHIP( 1, LockWaitStrategies.INCREMENTAL_BACKOFF ), GRAPH_PROPS( 2, LockWaitStrategies.INCREMENTAL_BACKOFF ), - SCHEMA( 3, LockWaitStrategies.INCREMENTAL_BACKOFF ), - INDEX_ENTRY( 4, LockWaitStrategies.INCREMENTAL_BACKOFF ), - LEGACY_INDEX( 5, LockWaitStrategies.INCREMENTAL_BACKOFF ); + INDEX_ENTRY( 3, LockWaitStrategies.INCREMENTAL_BACKOFF ), + LEGACY_INDEX( 4, LockWaitStrategies.INCREMENTAL_BACKOFF ), + LABEL( 5, LockWaitStrategies.INCREMENTAL_BACKOFF ), + RELATIONSHIP_TYPE( 6, LockWaitStrategies.INCREMENTAL_BACKOFF ); private static final Map idToType = new HashMap<>(); static @@ -77,9 +78,6 @@ public static long legacyIndexResourceId( String name, String key ) return (long)name.hashCode() << 32 | key.hashCode(); } - /** - * This is the schema index entry hashing method used since 2.2.0 and onwards. - */ public static long indexEntryResourceId( long labelId, IndexQuery.ExactPredicate... predicates ) { return indexEntryResourceId( labelId, predicates, 0 ); @@ -126,11 +124,6 @@ public static long graphPropertyResource() return 0L; } - public static long schemaResource() - { - return 0L; - } - public static ResourceType fromId( int typeId ) { return idToType.get( typeId ); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java index 98b4c3711b8a7..d5df0a62aba1f 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/api/LockingStatementOperationsTest.java @@ -24,8 +24,8 @@ import java.util.Iterator; import java.util.Optional; -import java.util.function.Function; +import org.neo4j.helpers.collection.Iterators; import org.neo4j.io.pagecache.tracing.cursor.PageCursorTracer; import org.neo4j.kernel.api.exceptions.EntityNotFoundException; import org.neo4j.kernel.api.exceptions.InvalidTransactionTypeKernelException; @@ -34,7 +34,7 @@ import org.neo4j.kernel.api.schema.LabelSchemaDescriptor; import org.neo4j.kernel.api.schema.SchemaDescriptorFactory; import org.neo4j.kernel.api.schema.constaints.ConstraintDescriptor; -import org.neo4j.kernel.api.schema.constaints.ConstraintDescriptorFactory; +import org.neo4j.kernel.api.schema.constaints.RelExistenceConstraintDescriptor; import org.neo4j.kernel.api.schema.constaints.UniquenessConstraintDescriptor; import org.neo4j.kernel.api.schema.index.IndexDescriptor; import org.neo4j.kernel.api.schema.index.IndexDescriptorFactory; @@ -72,8 +72,10 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.when; import static org.neo4j.helpers.collection.Iterators.asList; +import static org.neo4j.kernel.api.schema.constaints.ConstraintDescriptorFactory.existsForRelType; +import static org.neo4j.kernel.api.schema.constaints.ConstraintDescriptorFactory.uniqueForLabel; +import static org.neo4j.kernel.api.schema.constaints.ConstraintDescriptorFactory.uniqueForSchema; import static org.neo4j.kernel.impl.api.TwoPhaseNodeForRelationshipLockingTest.returnRelationships; -import static org.neo4j.kernel.impl.locking.ResourceTypes.schemaResource; public class LockingStatementOperationsTest { @@ -135,11 +137,12 @@ public void shouldNotAcquireEntityWriteLockBeforeAddingLabelToJustCreatedNode() public void shouldAcquireSchemaReadLockBeforeAddingLabelToNode() throws Exception { // when - lockingOps.nodeAddLabel( state, 123, 456 ); + int labelId = 456; + lockingOps.nodeAddLabel( state, 123, labelId ); // then - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); - order.verify( entityWriteOps ).nodeAddLabel( state, 123, 456 ); + order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.LABEL, labelId ); + order.verify( entityWriteOps ).nodeAddLabel( state, 123, labelId ); } @Test @@ -173,21 +176,6 @@ public void shouldNotAcquireEntityWriteLockBeforeSettingPropertyOnJustCreatedNod order.verify( entityWriteOps ).nodeSetProperty( state, 123, propertyKeyId, value ); } - @Test - public void shouldAcquireSchemaReadLockBeforeSettingPropertyOnNode() throws Exception - { - // given - int propertyKeyId = 8; - Value value = Values.of( 9 ); - - // when - lockingOps.nodeSetProperty( state, 123, propertyKeyId, value ); - - // then - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); - order.verify( entityWriteOps ).nodeSetProperty( state, 123, propertyKeyId, value ); - } - @Test public void shouldAcquireEntityWriteLockBeforeDeletingNode() throws EntityNotFoundException, AutoIndexingKernelException, InvalidTransactionTypeKernelException @@ -225,7 +213,7 @@ public void shouldAcquireSchemaWriteLockBeforeAddingIndexRule() throws Exception // then assertSame( index, result ); - order.verify( locks ).acquireExclusive( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); + order.verify( locks ).acquireExclusive( LockTracer.NONE, ResourceTypes.LABEL, descriptor.getLabelId() ); order.verify( schemaWriteOps ).indexCreate( state, descriptor ); } @@ -239,31 +227,34 @@ public void shouldAcquireSchemaWriteLockBeforeRemovingIndexRule() throws Excepti lockingOps.indexDrop( state, index ); // then - order.verify( locks ).acquireExclusive( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); + order.verify( locks ).acquireExclusive( LockTracer.NONE, ResourceTypes.LABEL, index.schema().getLabelId() ); order.verify( schemaWriteOps ).indexDrop( state, index ); } @Test - public void shouldAcquireSchemaReadLockBeforeGettingIndexRules() throws Exception + public void acquireReadLockBeforeGettingIndexRules() throws Exception { // given - Iterator rules = emptyIterator(); + int labelId = 1; + IndexDescriptor labelDescriptor = IndexDescriptorFactory.forLabel( labelId, 2, 3 ); + + Iterator rules = Iterators.iterator( labelDescriptor ); when( schemaReadOps.indexesGetAll( state ) ).thenReturn( rules ); // when Iterator result = lockingOps.indexesGetAll( state ); + Iterators.count( result ); // then - assertSame( rules, result ); - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); order.verify( schemaReadOps ).indexesGetAll( state ); + order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.LABEL, labelId ); } @Test public void shouldAcquireSchemaWriteLockBeforeCreatingUniquenessConstraint() throws Exception { // given - UniquenessConstraintDescriptor constraint = ConstraintDescriptorFactory.uniqueForSchema( descriptor ); + UniquenessConstraintDescriptor constraint = uniqueForSchema( descriptor ); when( schemaWriteOps.uniquePropertyConstraintCreate( state, descriptor ) ).thenReturn( constraint ); // when @@ -271,7 +262,7 @@ public void shouldAcquireSchemaWriteLockBeforeCreatingUniquenessConstraint() thr // then assertSame( constraint, result ); - order.verify( locks ).acquireExclusive( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); + order.verify( locks ).acquireExclusive( LockTracer.NONE, ResourceTypes.LABEL, descriptor.getLabelId() ); order.verify( schemaWriteOps ).uniquePropertyConstraintCreate( state, descriptor ); } @@ -279,13 +270,13 @@ public void shouldAcquireSchemaWriteLockBeforeCreatingUniquenessConstraint() thr public void shouldAcquireSchemaWriteLockBeforeDroppingConstraint() throws Exception { // given - UniquenessConstraintDescriptor constraint = ConstraintDescriptorFactory.uniqueForSchema( descriptor ); + UniquenessConstraintDescriptor constraint = uniqueForSchema( descriptor ); // when lockingOps.constraintDrop( state, constraint ); // then - order.verify( locks ).acquireExclusive( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); + order.verify( locks ).acquireExclusive( LockTracer.NONE, ResourceTypes.LABEL, descriptor.getLabelId() ); order.verify( schemaWriteOps ).constraintDrop( state, constraint ); } @@ -300,7 +291,7 @@ public void shouldAcquireSchemaReadLockBeforeGettingConstraintsByLabelAndPropert // then assertThat( asList( result ), empty() ); - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); + order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.LABEL, descriptor.getLabelId() ); order.verify( schemaReadOps ).constraintsGetForSchema( state, descriptor ); } @@ -308,66 +299,39 @@ public void shouldAcquireSchemaReadLockBeforeGettingConstraintsByLabelAndPropert public void shouldAcquireSchemaReadLockBeforeGettingConstraintsByLabel() throws Exception { // given - when( schemaReadOps.constraintsGetForLabel( state, 123 ) ).thenReturn( emptyIterator() ); + int labelId = 123; + when( schemaReadOps.constraintsGetForLabel( state, labelId ) ).thenReturn( emptyIterator() ); // when - Iterator result = lockingOps.constraintsGetForLabel( state, 123 ); + Iterator result = lockingOps.constraintsGetForLabel( state, labelId ); // then assertThat( asList( result ), empty() ); - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); - order.verify( schemaReadOps ).constraintsGetForLabel( state, 123 ); + order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.LABEL, labelId ); + order.verify( schemaReadOps ).constraintsGetForLabel( state, labelId ); } @Test public void shouldAcquireSchemaReadLockBeforeGettingAllConstraints() throws Exception { // given - when( schemaReadOps.constraintsGetAll( state ) ).thenReturn( emptyIterator() ); + int labelId = 1; + int relTypeId = 2; + UniquenessConstraintDescriptor uniquenessConstraint = uniqueForLabel( labelId, 2, 3, 3 ); + RelExistenceConstraintDescriptor existenceConstraint = existsForRelType( relTypeId, 3, 4, 5 ); + + when( schemaReadOps.constraintsGetAll( state ) ) + .thenReturn( Iterators.iterator( uniquenessConstraint, existenceConstraint ) ); // when Iterator result = lockingOps.constraintsGetAll( state ); + Iterators.count( result ); // then assertThat( asList( result ), empty() ); - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); order.verify( schemaReadOps ).constraintsGetAll( state ); - } - - @Test - public void shouldAcquireSchemaReadLockBeforeUpdatingSchemaState() throws Exception - { - // given - Function creator = from -> null; - - // when - lockingOps.schemaStateGetOrCreate( state, null, creator ); - - // then - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); - order.verify( schemaStateOps ).schemaStateGetOrCreate( state, null, creator ); - } - - @Test - public void shouldAcquireSchemaReadLockBeforeCheckingSchemaState() throws Exception - { - // when - lockingOps.schemaStateContains( state, null ); - - // then - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); - order.verify( schemaStateOps ).schemaStateContains( state, null ); - } - - @Test - public void shouldAcquireSchemaReadLockBeforeFlushingSchemaState() throws Exception - { - // when - lockingOps.schemaStateFlush( state ); - - // then - order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, schemaResource() ); - order.verify( schemaStateOps ).schemaStateFlush( state ); + order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.LABEL, labelId ); + order.verify( locks ).acquireShared( LockTracer.NONE, ResourceTypes.RELATIONSHIP_TYPE, relTypeId ); } @Test 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 21d0d1323ae1f..eb9145c29c0fe 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 @@ -34,8 +34,8 @@ import org.neo4j.kernel.api.exceptions.TransactionFailureException; import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException; import org.neo4j.kernel.api.exceptions.index.IndexPopulationFailedKernelException; -import org.neo4j.kernel.api.exceptions.schema.UniquePropertyValueValidationException; import org.neo4j.kernel.api.exceptions.schema.AlreadyConstrainedException; +import org.neo4j.kernel.api.exceptions.schema.UniquePropertyValueValidationException; import org.neo4j.kernel.api.index.PropertyAccessor; import org.neo4j.kernel.api.proc.CallableProcedure; import org.neo4j.kernel.api.proc.CallableUserAggregationFunction; @@ -64,7 +64,6 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; - import static org.neo4j.kernel.impl.api.StatementOperationsTestHelper.mockedParts; import static org.neo4j.kernel.impl.api.StatementOperationsTestHelper.mockedState; @@ -95,7 +94,7 @@ public void shouldCreateIndexInAnotherTransaction() throws Exception PropertyAccessor propertyAccessor = mock( PropertyAccessor.class ); when( constraintCreationContext.schemaReadOperations().indexGetForSchema( state, descriptor ) ) .thenReturn( null ); - ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); + ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor ); // when long indexId = creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), descriptor ); @@ -131,7 +130,7 @@ public void shouldDropIndexIfPopulationFails() throws Exception PropertyAccessor propertyAccessor = mock( PropertyAccessor.class ); when( constraintCreationContext.schemaReadOperations().indexGetForSchema( state, descriptor ) ) .thenReturn( null ); - ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); + ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor ); // when try @@ -168,7 +167,7 @@ public void shouldDropIndexInAnotherTransaction() throws Exception PropertyAccessor propertyAccessor = mock( PropertyAccessor.class ); - ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); + ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor ); // when creator.dropUniquenessConstraintIndex( index ); @@ -180,7 +179,7 @@ public void shouldDropIndexInAnotherTransaction() throws Exception } @Test - public void shouldReleaseSchemaLockWhileAwaitingIndexPopulation() throws Exception + public void shouldReleaseLabelLockWhileAwaitingIndexPopulation() throws Exception { // given StubKernel kernel = new StubKernel(); @@ -198,17 +197,17 @@ public void shouldReleaseSchemaLockWhileAwaitingIndexPopulation() throws Excepti when( constraintCreationContext.schemaReadOperations().indexGetForSchema( state, descriptor ) ) .thenReturn( null ); - ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, true ); + ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor ); // when creator.createUniquenessConstraintIndex( state, constraintCreationContext.schemaReadOperations(), descriptor ); // then verify( state.locks().pessimistic() ) - .releaseExclusive( ResourceTypes.SCHEMA, ResourceTypes.schemaResource() ); + .releaseExclusive( ResourceTypes.LABEL, descriptor.getLabelId() ); verify( state.locks().pessimistic() ) - .acquireExclusive( state.lockTracer(), ResourceTypes.SCHEMA, ResourceTypes.schemaResource() ); + .acquireExclusive( state.lockTracer(), ResourceTypes.LABEL, descriptor.getLabelId() ); } @Test @@ -233,7 +232,7 @@ public void shouldReuseExistingOrphanedConstraintIndex() throws Exception .thenReturn( index ); when( constraintCreationContext.schemaReadOperations().indexGetOwningUniquenessConstraintId( state, index ) ).thenReturn( null ); // which means it has no owner - ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); + ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor ); // when long indexId = creator.createUniquenessConstraintIndex( state, @@ -277,7 +276,7 @@ public void shouldFailOnExistingOwnedConstraintIndex() throws Exception state, index ) ).thenReturn( constraintIndexOwnerId ); // which means there's an owner when( state.readOperations().labelGetName( LABEL_ID ) ).thenReturn( "MyLabel" ); when( state.readOperations().propertyKeyGetName( PROPERTY_KEY_ID ) ).thenReturn( "MyKey" ); - ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor, false ); + ConstraintIndexCreator creator = new ConstraintIndexCreator( () -> kernel, indexingService, propertyAccessor ); // when try diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/ActiveLocksListingCompatibility.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/ActiveLocksListingCompatibility.java index bd2cdf04af2ae..d04f4b6f2fd20 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/ActiveLocksListingCompatibility.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/locking/ActiveLocksListingCompatibility.java @@ -19,20 +19,20 @@ */ package org.neo4j.kernel.impl.locking; -import java.util.HashSet; -import java.util.stream.Stream; - import org.junit.Ignore; import org.junit.Test; +import java.util.HashSet; +import java.util.stream.Stream; + import static java.util.Arrays.asList; import static java.util.stream.Collectors.toSet; import static org.junit.Assert.assertEquals; import static org.neo4j.kernel.impl.locking.ActiveLock.exclusiveLock; import static org.neo4j.kernel.impl.locking.ActiveLock.sharedLock; +import static org.neo4j.kernel.impl.locking.ResourceTypes.LABEL; import static org.neo4j.kernel.impl.locking.ResourceTypes.NODE; import static org.neo4j.kernel.impl.locking.ResourceTypes.RELATIONSHIP; -import static org.neo4j.kernel.impl.locking.ResourceTypes.SCHEMA; @Ignore( "Not a test. This is a compatibility suite, run from LockingCompatibilityTestSuite." ) public class ActiveLocksListingCompatibility extends LockingCompatibilityTestSuite.Compatibility @@ -68,7 +68,7 @@ public void shouldListLocksHeldByTheCurrentClient() throws Exception public void shouldCountNumberOfActiveLocks() throws Exception { // given - clientA.acquireShared( LockTracer.NONE, SCHEMA, 0 ); + clientA.acquireShared( LockTracer.NONE, LABEL, 0 ); clientA.acquireShared( LockTracer.NONE, RELATIONSHIP, 17 ); clientA.acquireShared( LockTracer.NONE, NODE, 12 ); diff --git a/community/neo4j/src/test/java/org/neo4j/locking/CommunityLockAcquisitionTimeoutIT.java b/community/neo4j/src/test/java/org/neo4j/locking/CommunityLockAcquisitionTimeoutIT.java index c254cd137898c..bf641b987bbf0 100644 --- a/community/neo4j/src/test/java/org/neo4j/locking/CommunityLockAcquisitionTimeoutIT.java +++ b/community/neo4j/src/test/java/org/neo4j/locking/CommunityLockAcquisitionTimeoutIT.java @@ -28,12 +28,12 @@ import org.junit.rules.ExpectedException; import java.io.File; -import java.time.Clock; import java.util.Map; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.function.Predicate; +import org.neo4j.graphdb.DependencyResolver; import org.neo4j.graphdb.GraphDatabaseService; import org.neo4j.graphdb.Label; import org.neo4j.graphdb.Node; @@ -50,7 +50,12 @@ import org.neo4j.kernel.impl.factory.GraphDatabaseFacadeFactory; import org.neo4j.kernel.impl.factory.PlatformModule; import org.neo4j.kernel.impl.locking.LockAcquisitionTimeoutException; +import org.neo4j.kernel.impl.locking.LockTracer; +import org.neo4j.kernel.impl.locking.Locks; +import org.neo4j.kernel.impl.locking.ResourceTypes; import org.neo4j.kernel.impl.locking.community.CommunityLockClient; +import org.neo4j.kernel.impl.locking.community.CommunityLockManger; +import org.neo4j.kernel.internal.GraphDatabaseAPI; import org.neo4j.test.OtherThreadExecutor; import org.neo4j.test.TestGraphDatabaseFactory; import org.neo4j.test.mockito.matcher.RootCauseMatcher; @@ -104,7 +109,7 @@ public void tearDown() clockExecutor.close(); } - @Test( timeout = 5000 ) + @Test( timeout = TEST_TIMEOUT ) public void timeoutOnAcquiringExclusiveLock() throws Exception { expectedException.expect( new RootCauseMatcher<>( LockAcquisitionTimeoutException.class, @@ -148,11 +153,12 @@ public void timeoutOnAcquiringSharedLock() throws Exception "The transaction has been terminated. " + "Retry your operation in a new transaction, and you should see a successful result. " + "Unable to acquire lock within configured timeout. " + - "Unable to acquire lock for resource: SCHEMA with id: 0 within 2000 millis." ) ); + "Unable to acquire lock for resource: LABEL with id: 1 within 2000 millis." ) ); try ( Transaction ignored = database.beginTx() ) { - database.schema().indexFor( marker ).on( TEST_PROPERTY_NAME ).create(); + Locks lockManger = getLockManager(); + lockManger.newClient().acquireExclusive( LockTracer.NONE, ResourceTypes.LABEL, 1 ); Future propertySetFuture = secondTransactionExecutor.executeDontWait( state -> { @@ -178,6 +184,16 @@ public void timeoutOnAcquiringSharedLock() throws Exception } } + protected Locks getLockManager() + { + return getDependencyResolver().resolveDependency( CommunityLockManger.class ); + } + + protected DependencyResolver getDependencyResolver() + { + return ((GraphDatabaseAPI) database).getDependencyResolver(); + } + protected Predicate exclusiveLockWaitingPredicate() { return waitDetails -> waitDetails.isAt( CommunityLockClient.class, "acquireExclusive" ); diff --git a/enterprise/deferred-locks/src/test/java/org/neo4j/kernel/impl/locking/DeferringLockClientTest.java b/enterprise/deferred-locks/src/test/java/org/neo4j/kernel/impl/locking/DeferringLockClientTest.java index 20e0c29a28dc1..5ae3aad66c2cb 100644 --- a/enterprise/deferred-locks/src/test/java/org/neo4j/kernel/impl/locking/DeferringLockClientTest.java +++ b/enterprise/deferred-locks/src/test/java/org/neo4j/kernel/impl/locking/DeferringLockClientTest.java @@ -19,6 +19,9 @@ */ package org.neo4j.kernel.impl.locking; +import org.junit.Rule; +import org.junit.Test; + import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -26,9 +29,6 @@ import java.util.Set; import java.util.stream.Stream; -import org.junit.Rule; -import org.junit.Test; - import org.neo4j.kernel.lifecycle.LifecycleAdapter; import org.neo4j.storageengine.api.lock.AcquireLockTimeoutException; import org.neo4j.storageengine.api.lock.ResourceType; @@ -343,7 +343,7 @@ public void exclusiveLocksAcquiredFirst() client.acquireExclusive( LockTracer.NONE, ResourceTypes.NODE, 3 ); client.acquireExclusive( LockTracer.NONE, ResourceTypes.RELATIONSHIP, 1 ); client.acquireShared( LockTracer.NONE, ResourceTypes.RELATIONSHIP, 2 ); - client.acquireShared( LockTracer.NONE, ResourceTypes.SCHEMA, 1 ); + client.acquireShared( LockTracer.NONE, ResourceTypes.LABEL, 1 ); client.acquireExclusive( LockTracer.NONE, ResourceTypes.NODE, 42 ); // WHEN @@ -357,7 +357,7 @@ public void exclusiveLocksAcquiredFirst() new LockUnit( ResourceTypes.RELATIONSHIP, 1, true ), new LockUnit( ResourceTypes.NODE, 1, false ), new LockUnit( ResourceTypes.RELATIONSHIP, 2, false ), - new LockUnit( ResourceTypes.SCHEMA, 1, false ) ) + new LockUnit( ResourceTypes.LABEL, 1, false ) ) ); actualClient.assertRegisteredLocks( expectedLocks ); diff --git a/enterprise/deferred-locks/src/test/java/org/neo4j/kernel/impl/locking/LockUnitTest.java b/enterprise/deferred-locks/src/test/java/org/neo4j/kernel/impl/locking/LockUnitTest.java index 65dec2049ae14..ecf75acfc4e2a 100644 --- a/enterprise/deferred-locks/src/test/java/org/neo4j/kernel/impl/locking/LockUnitTest.java +++ b/enterprise/deferred-locks/src/test/java/org/neo4j/kernel/impl/locking/LockUnitTest.java @@ -36,7 +36,7 @@ public void exclusiveLocksAppearFirst() LockUnit unit2 = new LockUnit( ResourceTypes.NODE, 2, false ); LockUnit unit3 = new LockUnit( ResourceTypes.RELATIONSHIP, 1, false ); LockUnit unit4 = new LockUnit( ResourceTypes.RELATIONSHIP, 2, true ); - LockUnit unit5 = new LockUnit( ResourceTypes.SCHEMA, 1, false ); + LockUnit unit5 = new LockUnit( ResourceTypes.RELATIONSHIP_TYPE, 1, false ); List list = asList( unit1, unit2, unit3, unit4, unit5 ); Collections.sort( list ); @@ -50,7 +50,7 @@ public void exclusiveOrderedByResourceTypes() LockUnit unit1 = new LockUnit( ResourceTypes.NODE, 1, true ); LockUnit unit2 = new LockUnit( ResourceTypes.RELATIONSHIP, 1, true ); LockUnit unit3 = new LockUnit( ResourceTypes.NODE, 2, true ); - LockUnit unit4 = new LockUnit( ResourceTypes.SCHEMA, 1, true ); + LockUnit unit4 = new LockUnit( ResourceTypes.RELATIONSHIP_TYPE, 1, true ); LockUnit unit5 = new LockUnit( ResourceTypes.RELATIONSHIP, 2, true ); List list = asList( unit1, unit2, unit3, unit4, unit5 ); diff --git a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/MasterTransactionCommitProcess.java b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/MasterTransactionCommitProcess.java index c9f7c236c6e94..f024366c7e8ea 100644 --- a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/MasterTransactionCommitProcess.java +++ b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/MasterTransactionCommitProcess.java @@ -19,46 +19,25 @@ */ package org.neo4j.kernel.ha; -import java.io.IOException; - -import org.neo4j.helpers.collection.Visitor; import org.neo4j.kernel.api.exceptions.TransactionFailureException; import org.neo4j.kernel.ha.transaction.TransactionPropagator; import org.neo4j.kernel.impl.api.TransactionCommitProcess; import org.neo4j.kernel.impl.api.TransactionToApply; -import org.neo4j.kernel.impl.locking.LockTracer; -import org.neo4j.kernel.impl.locking.Locks; -import org.neo4j.kernel.impl.transaction.command.Command.NodeCommand; -import org.neo4j.kernel.impl.transaction.command.Command.PropertyCommand; import org.neo4j.kernel.impl.transaction.state.IntegrityValidator; import org.neo4j.kernel.impl.transaction.tracing.CommitEvent; -import org.neo4j.storageengine.api.StorageCommand; import org.neo4j.storageengine.api.TransactionApplicationMode; -import static org.neo4j.kernel.impl.api.index.NodePropertyCommandsExtractor.mayResultInIndexUpdates; -import static org.neo4j.kernel.impl.locking.ResourceTypes.SCHEMA; -import static org.neo4j.kernel.impl.locking.ResourceTypes.schemaResource; - /** * Commit process on the master side in HA, where transactions either comes in from slaves committing, * or gets created and committed directly on the master. */ public class MasterTransactionCommitProcess implements TransactionCommitProcess { - /** - * Detector of transactions coming in from slaves which should acquire the shared schema lock before - * being applied (and validated for application). - */ - private static final Visitor REQUIRES_SHARED_SCHEMA_LOCK = command -> - command instanceof NodeCommand && mayResultInIndexUpdates( (NodeCommand) command ) || - command instanceof PropertyCommand && mayResultInIndexUpdates( (PropertyCommand) command ); private final TransactionCommitProcess inner; private final TransactionPropagator txPropagator; private final IntegrityValidator validator; private final Monitor monitor; - private final Locks locks; - private final boolean reacquireSharedSchemaLockOnIncomingTransactions; public interface Monitor { @@ -66,26 +45,20 @@ public interface Monitor } public MasterTransactionCommitProcess( TransactionCommitProcess commitProcess, TransactionPropagator txPropagator, - IntegrityValidator validator, Monitor monitor, Locks locks, - boolean reacquireSharedSchemaLockOnIncomingTransactions ) + IntegrityValidator validator, Monitor monitor ) { this.inner = commitProcess; this.txPropagator = txPropagator; this.validator = validator; this.monitor = monitor; - this.locks = locks; - this.reacquireSharedSchemaLockOnIncomingTransactions = reacquireSharedSchemaLockOnIncomingTransactions; } @Override public long commit( TransactionToApply batch, CommitEvent commitEvent, TransactionApplicationMode mode ) throws TransactionFailureException { - long result; - try ( Locks.Client locks = validate( batch ) ) - { - result = inner.commit( batch, commitEvent, mode ); - } + validate( batch ); + long result = inner.commit( batch, commitEvent, mode ); // Assuming all the transactions come from the same author int missedReplicas = txPropagator.committed( result, batch.transactionRepresentation().getAuthorId() ); @@ -98,74 +71,14 @@ public long commit( TransactionToApply batch, CommitEvent commitEvent, Transacti return result; } - private Locks.Client validate( TransactionToApply batch ) throws TransactionFailureException + private void validate( TransactionToApply batch ) throws TransactionFailureException { - Locks.Client locks = null; - boolean success = false; - try - { - while ( batch != null ) - { - if ( reacquireSharedSchemaLockOnIncomingTransactions ) - { - locks = acquireSharedSchemaLockIfTransactionResultsInIndexUpdates( batch, locks ); - } - validator.validateTransactionStartKnowledge( - batch.transactionRepresentation().getLatestCommittedTxWhenStarted() ); - batch = batch.next(); - } - success = true; - return locks; - } - finally + while ( batch != null ) { - if ( !success && locks != null ) - { - // There was an exception which prevents us from returning the Locks.Client to the caller - // which ultimately should have been responsible for closing it, but now we can't so - // we need to close it ourselves in here before letting the exception propagate further. - locks.close(); - locks = null; - } + validator.validateTransactionStartKnowledge( + batch.transactionRepresentation().getLatestCommittedTxWhenStarted() ); + batch = batch.next(); } } - /** - * Looks at the transaction coming from slave and decide whether or not the shared schema lock - * should be acquired before letting it apply. - *

    - * In HA the shared schema lock isn't acquired on the master. This has been fine due to other - * factors and guards being in place. However this was introduced when releasing the exclusive schema lock - * during index population when creating a uniqueness constraint. This added locking guards for race - * between constraint creating transaction (on master) and concurrent slave transactions which may - * result in index updates for that constraint, and potentially break it. - * - * @param batch {@link TransactionToApply} to apply, only HEAD since linked list looping is done outside. - * @param locks potentially existing locks client, otherwise this method will create and return - * if there's a need to acquire a lock. - * @return either, if {@code locks} is non-null then the same instance, or if {@code locks} is null - * and some locking is required then a new locks instance. - * @throws TransactionFailureException on failure to read transaction. - */ - private Locks.Client acquireSharedSchemaLockIfTransactionResultsInIndexUpdates( TransactionToApply batch, - Locks.Client locks ) throws TransactionFailureException - { - try - { - if ( batch.accept( REQUIRES_SHARED_SCHEMA_LOCK ) ) - { - if ( locks == null ) - { - locks = this.locks.newClient(); - } - locks.acquireShared( LockTracer.NONE, SCHEMA, schemaResource() ); - } - return locks; - } - catch ( IOException e ) - { - throw new TransactionFailureException( - "Weird error when trying to figure out whether or not to acquire shared schema lock", e ); - } - } } diff --git a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/cluster/modeswitch/CommitProcessSwitcher.java b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/cluster/modeswitch/CommitProcessSwitcher.java index 03a784ae94583..a0a00351cfcd5 100644 --- a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/cluster/modeswitch/CommitProcessSwitcher.java +++ b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/cluster/modeswitch/CommitProcessSwitcher.java @@ -28,7 +28,6 @@ import org.neo4j.kernel.ha.transaction.TransactionPropagator; import org.neo4j.kernel.impl.api.TransactionCommitProcess; import org.neo4j.kernel.impl.api.TransactionRepresentationCommitProcess; -import org.neo4j.kernel.impl.locking.Locks; import org.neo4j.kernel.impl.transaction.log.TransactionAppender; import org.neo4j.kernel.impl.transaction.state.IntegrityValidator; import org.neo4j.kernel.monitoring.Monitors; @@ -41,22 +40,16 @@ public class CommitProcessSwitcher extends AbstractComponentSwitcher delegate, RequestContextFactory requestContextFactory, - Locks locks, - Monitors monitors, DependencyResolver dependencyResolver, - boolean reacquireSharedSchemaLockOnIncomingTransactions ) + Monitors monitors, DependencyResolver dependencyResolver ) { super( delegate ); this.txPropagator = txPropagator; this.master = master; this.requestContextFactory = requestContextFactory; - this.locks = locks; this.dependencyResolver = dependencyResolver; - this.reacquireSharedSchemaLockOnIncomingTransactions = reacquireSharedSchemaLockOnIncomingTransactions; this.monitor = monitors.newMonitor( MasterTransactionCommitProcess.Monitor.class ); } @@ -74,7 +67,6 @@ protected TransactionCommitProcess getMasterImpl() dependencyResolver.resolveDependency( StorageEngine.class ) ); IntegrityValidator validator = dependencyResolver.resolveDependency( IntegrityValidator.class ); - return new MasterTransactionCommitProcess( commitProcess, txPropagator, validator, monitor, locks, - reacquireSharedSchemaLockOnIncomingTransactions ); + return new MasterTransactionCommitProcess( commitProcess, txPropagator, validator, monitor ); } } diff --git a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/com/master/MasterImpl.java b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/com/master/MasterImpl.java index 9ecf1b8b1018c..a9d77f8f2c6c8 100644 --- a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/com/master/MasterImpl.java +++ b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/com/master/MasterImpl.java @@ -40,7 +40,6 @@ import org.neo4j.kernel.ha.lock.LockStatus; import org.neo4j.kernel.impl.locking.LockTracer; import org.neo4j.kernel.impl.locking.Locks; -import org.neo4j.kernel.impl.locking.ResourceTypes; import org.neo4j.kernel.impl.store.StoreId; import org.neo4j.kernel.impl.store.id.IdType; import org.neo4j.kernel.impl.transaction.IllegalResourceException; @@ -174,7 +173,7 @@ public Response commit( RequestContext context, TransactionRepresentation // Client is not holding locks, use a temporary lock client try ( Conversation conversation = conversationManager.acquire() ) { - return commit0( context, preparedTransaction, conversation.getLocks() ); + return commit0( context, preparedTransaction ); } } else @@ -183,10 +182,9 @@ public Response commit( RequestContext context, TransactionRepresentation try { Conversation conversation = conversationManager.acquire( context ); - Locks.Client locks = conversation.getLocks(); try { - return commit0( context, preparedTransaction, locks ); + return commit0( context, preparedTransaction ); } finally { @@ -200,21 +198,11 @@ public Response commit( RequestContext context, TransactionRepresentation } } - private Response commit0( RequestContext context, TransactionRepresentation preparedTransaction, Locks - .Client locks ) throws IOException, org.neo4j.kernel.api.exceptions.TransactionFailureException + private Response commit0( RequestContext context, TransactionRepresentation preparedTransaction ) + throws IOException, org.neo4j.kernel.api.exceptions.TransactionFailureException { - if ( locks.trySharedLock( ResourceTypes.SCHEMA, ResourceTypes.schemaResource() ) ) - { - long txId = spi.applyPreparedTransaction( preparedTransaction ); - return spi.packTransactionObligationResponse( context, txId ); - } - else - { - throw new TransactionFailureException( Status.Schema.SchemaModifiedConcurrently, - "Failed to commit, because another transaction is making " + - "schema changes. Slave commits are disallowed while schema changes are being committed. " + - "Retrying the transaction should yield a successful result." ); - } + long txId = spi.applyPreparedTransaction( preparedTransaction ); + return spi.packTransactionObligationResponse( context, txId ); } @Override diff --git a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/factory/HighlyAvailableEditionModule.java b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/factory/HighlyAvailableEditionModule.java index 07c490493154b..f6c57bb0b4677 100644 --- a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/factory/HighlyAvailableEditionModule.java +++ b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/factory/HighlyAvailableEditionModule.java @@ -639,8 +639,7 @@ private CommitProcessFactory createCommitProcessFactory( Dependencies dependenci TransactionCommitProcess.class ); CommitProcessSwitcher commitProcessSwitcher = new CommitProcessSwitcher( transactionPropagator, - master, commitProcessDelegate, requestContextFactory, lockManager, monitors, dependencies, - config.get( GraphDatabaseSettings.release_schema_lock_while_building_constraint ) ); + master, commitProcessDelegate, requestContextFactory, monitors, dependencies ); componentSwitcherContainer.add( commitProcessSwitcher ); return new HighlyAvailableCommitProcessFactory( commitProcessDelegate ); diff --git a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/lock/SlaveLocksClient.java b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/lock/SlaveLocksClient.java index 56899d3352c33..ddfa2e11bf7eb 100644 --- a/enterprise/ha/src/main/java/org/neo4j/kernel/ha/lock/SlaveLocksClient.java +++ b/enterprise/ha/src/main/java/org/neo4j/kernel/ha/lock/SlaveLocksClient.java @@ -20,9 +20,6 @@ package org.neo4j.kernel.ha.lock; import java.util.Arrays; -import java.util.Map; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Function; import java.util.stream.Stream; import org.neo4j.collection.primitive.PrimitiveLongCollections; @@ -59,10 +56,6 @@ */ class SlaveLocksClient implements Locks.Client { - private static final Function>,Stream> - EXCLUSIVE_ACTIVE_LOCKS = activeLocks( ActiveLock.Factory.EXCLUSIVE_LOCK ); - private static final Function>,Stream> - SHARED_ACTIVE_LOCKS = activeLocks( ActiveLock.Factory.SHARED_LOCK ); private final Master master; private final Locks.Client client; private final Locks localLockManager; @@ -230,16 +223,6 @@ public long activeLockCount() return client.activeLockCount(); } - private static Function>,Stream> activeLocks( - ActiveLock.Factory activeLock ) - { - return entry -> entry.getValue().keySet().stream().map( resourceId -> - { - ResourceType resourceType = entry.getKey(); - return activeLock.create( resourceType, resourceId ); - } ); - } - private void stopLockSessionOnMaster() { try @@ -337,22 +320,20 @@ private void releaseExclusive( ResourceType resourceType, long[] resourceIds, lo private void acquireSharedOnMaster( ResourceType resourceType, long... resourceIds ) { - if ( resourceType == ResourceTypes.NODE - || resourceType == ResourceTypes.RELATIONSHIP - || resourceType == ResourceTypes.GRAPH_PROPS - || resourceType == ResourceTypes.LEGACY_INDEX ) + if ( resourceType == ResourceTypes.INDEX_ENTRY ) { - makeSureTxHasBeenInitialized(); + return; + } + makeSureTxHasBeenInitialized(); - RequestContext requestContext = newRequestContextFor( this ); - try ( Response response = master.acquireSharedLock( requestContext, resourceType, resourceIds ) ) - { - receiveLockResponse( response ); - } - catch ( ComException e ) - { - throw new DistributedLockFailureException( "Cannot get shared lock(s) on master", master, e ); - } + RequestContext requestContext = newRequestContextFor( this ); + try ( Response response = master.acquireSharedLock( requestContext, resourceType, resourceIds ) ) + { + receiveLockResponse( response ); + } + catch ( ComException e ) + { + throw new DistributedLockFailureException( "Cannot get shared lock(s) on master", master, e ); } } diff --git a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/PropertyConstraintsStressIT.java b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/PropertyConstraintsStressIT.java index 69a231aaf3877..fb6788c4a4efd 100644 --- a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/PropertyConstraintsStressIT.java +++ b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/PropertyConstraintsStressIT.java @@ -44,7 +44,6 @@ import org.neo4j.graphdb.Transaction; import org.neo4j.graphdb.TransactionFailureException; import org.neo4j.graphdb.TransientTransactionFailureException; -import org.neo4j.graphdb.factory.GraphDatabaseSettings; import org.neo4j.helpers.Exceptions; import org.neo4j.kernel.api.exceptions.schema.ConstraintValidationException; import org.neo4j.kernel.impl.ha.ClusterManager; @@ -69,12 +68,9 @@ @RunWith( Parameterized.class ) public class PropertyConstraintsStressIT { - @Parameter( 0 ) + @Parameter public ConstraintOperations constraintOps; - @Parameter( 1 ) - public boolean releaseSchemaLockWhileBuildingIndex; - @Rule public final SuppressOutput suppressOutput = SuppressOutput.suppressAll(); @ClassRule @@ -105,14 +101,10 @@ public class PropertyConstraintsStressIT private final AtomicInteger roundNo = new AtomicInteger( 0 ); @Parameterized.Parameters( name = "{0}:{1}" ) - public static Iterable params() + public static Iterable params() { - return Arrays.asList( new Object[][]{ - {UNIQUE_PROPERTY_CONSTRAINT_OPS, false}, - {UNIQUE_PROPERTY_CONSTRAINT_OPS, true}, - {NODE_PROPERTY_EXISTENCE_CONSTRAINT_OPS, false}, - {REL_PROPERTY_EXISTENCE_CONSTRAINT_OPS, false}, - } ); + return Arrays.asList( UNIQUE_PROPERTY_CONSTRAINT_OPS, UNIQUE_PROPERTY_CONSTRAINT_OPS, + NODE_PROPERTY_EXISTENCE_CONSTRAINT_OPS, REL_PROPERTY_EXISTENCE_CONSTRAINT_OPS ); } @Before @@ -120,8 +112,6 @@ public void setup() throws Exception { cluster = clusterRule .withSharedSetting( HaSettings.pull_interval, "0" ) - .withSharedSetting( GraphDatabaseSettings.release_schema_lock_while_building_constraint, - String.valueOf( releaseSchemaLockWhileBuildingIndex ) ) .startCluster(); clearData(); } diff --git a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/com/master/MasterImplConversationStopFuzzIT.java b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/com/master/MasterImplConversationStopFuzzIT.java index 1d4f27ed405a4..8a027a790e5ac 100644 --- a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/com/master/MasterImplConversationStopFuzzIT.java +++ b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/com/master/MasterImplConversationStopFuzzIT.java @@ -55,13 +55,13 @@ import org.neo4j.kernel.impl.store.id.IdType; import org.neo4j.kernel.impl.transaction.TransactionRepresentation; import org.neo4j.kernel.impl.transaction.log.TransactionIdStore; -import org.neo4j.scheduler.JobScheduler; import org.neo4j.kernel.impl.util.Neo4jJobScheduler; import org.neo4j.kernel.impl.util.collection.ConcurrentAccessException; import org.neo4j.kernel.impl.util.collection.TimedRepository; import org.neo4j.kernel.lifecycle.LifeSupport; import org.neo4j.kernel.monitoring.Monitors; import org.neo4j.logging.FormattedLog; +import org.neo4j.scheduler.JobScheduler; import org.neo4j.test.rule.VerboseTimeout; import org.neo4j.time.Clocks; @@ -95,7 +95,7 @@ public class MasterImplConversationStopFuzzIT private final JobScheduler scheduler = life.add( new Neo4jJobScheduler() ); private final Config config = Config.embeddedDefaults( stringMap( server_id.name(), "0", lock_read_timeout.name(), "1" ) ); private final Locks locks = new ForsetiLockManager( Config.defaults(), Clocks.systemClock(), - ResourceTypes.NODE, ResourceTypes.SCHEMA ); + ResourceTypes.NODE, ResourceTypes.LABEL ); private static MasterExecutionStatistic executionStatistic = new MasterExecutionStatistic(); diff --git a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/com/master/MasterImplTest.java b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/com/master/MasterImplTest.java index bc34fc7b70460..be567f7383d97 100644 --- a/enterprise/ha/src/test/java/org/neo4j/kernel/ha/com/master/MasterImplTest.java +++ b/enterprise/ha/src/test/java/org/neo4j/kernel/ha/com/master/MasterImplTest.java @@ -306,12 +306,9 @@ public void shouldAllowCommitIfClientHoldsNoLocks() throws Throwable Config config = config(); DefaultConversationSPI conversationSpi = mockedConversationSpi(); ConversationManager conversationManager = new ConversationManager( conversationSpi, config ); - Client locks = mock( Client.class ); - when(locks.trySharedLock( ResourceTypes.SCHEMA, ResourceTypes.schemaResource() ) ).thenReturn( true ); when( spi.isAccessible() ).thenReturn( true ); when( spi.getTransactionChecksum( anyLong() ) ).thenReturn( 1L ); - when( conversationSpi.acquireClient()).thenReturn( locks ); mockEmptyResponse( spi ); MasterImpl master = new MasterImpl( spi, conversationManager, mock( MasterImpl.Monitor.class ), config ); diff --git a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/enterprise/lock/forseti/ForsetiLockManager.java b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/enterprise/lock/forseti/ForsetiLockManager.java index c00bc5b167e3f..96f7acdc45a73 100644 --- a/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/enterprise/lock/forseti/ForsetiLockManager.java +++ b/enterprise/kernel/src/main/java/org/neo4j/kernel/impl/enterprise/lock/forseti/ForsetiLockManager.java @@ -106,14 +106,6 @@ * traversing the graph like this until we either find ourselves amongst the owners - a deadlock - or we run out of * locks that are being waited upon - no deadlock. *

    - *

    Future work

    - *

    - * We have at least one type of lock (SchemaLock) that can be held concurrently by several hundred transactions. It may - * be worth investigating fat locks, or in any case optimize the current way SharedLock adds and removes clients from - * its holder list. - *

    - * The maps used by Forseti should be replaced by faster concurrent maps, perhaps a striped hopscotch map or something - * similar. */ public class ForsetiLockManager implements Locks { diff --git a/enterprise/management/src/test/java/org/neo4j/management/TestLockManagerBean.java b/enterprise/management/src/test/java/org/neo4j/management/TestLockManagerBean.java index 0a354b15e9b36..e3c2a5c9003ca 100644 --- a/enterprise/management/src/test/java/org/neo4j/management/TestLockManagerBean.java +++ b/enterprise/management/src/test/java/org/neo4j/management/TestLockManagerBean.java @@ -41,7 +41,6 @@ public class TestLockManagerBean @Rule public ImpermanentDatabaseRule dbRule = new ImpermanentDatabaseRule(); - @SuppressWarnings( "deprecation" ) private GraphDatabaseAPI graphDb; @Before @@ -68,7 +67,7 @@ public void modifiedNodeImpliesLock() node.setProperty( "key", "value" ); List locks = lockManager.getLocks(); - assertEquals( "unexpected lock count", 2, locks.size() ); + assertEquals( "unexpected lock count", 1, locks.size() ); LockInfo lock = locks.get( 0 ); assertNotNull( "null lock", lock ); @@ -87,12 +86,4 @@ private Node createNode() } } - private LockInfo getSingleLock() - { - List locks = lockManager.getLocks(); - assertEquals( "unexpected lock count", 1, locks.size() ); - LockInfo lock = locks.get( 0 ); - assertNotNull( "null lock", lock ); - return lock; - } } diff --git a/enterprise/neo4j-enterprise/src/test/java/org/neo4j/lock/EnterpriseLockAcquisitionTimeoutIT.java b/enterprise/neo4j-enterprise/src/test/java/org/neo4j/lock/EnterpriseLockAcquisitionTimeoutIT.java index 60a134a92142c..36e919b5f348b 100644 --- a/enterprise/neo4j-enterprise/src/test/java/org/neo4j/lock/EnterpriseLockAcquisitionTimeoutIT.java +++ b/enterprise/neo4j-enterprise/src/test/java/org/neo4j/lock/EnterpriseLockAcquisitionTimeoutIT.java @@ -22,6 +22,8 @@ import java.util.function.Predicate; import org.neo4j.kernel.impl.enterprise.lock.forseti.ForsetiClient; +import org.neo4j.kernel.impl.enterprise.lock.forseti.ForsetiLockManager; +import org.neo4j.kernel.impl.locking.Locks; import org.neo4j.locking.CommunityLockAcquisitionTimeoutIT; import org.neo4j.test.OtherThreadExecutor; @@ -38,4 +40,10 @@ protected Predicate sharedLockWaitingPredicate( { return waitDetails -> waitDetails.isAt( ForsetiClient.class, "acquireShared" ); } + + @Override + protected Locks getLockManager() + { + return getDependencyResolver().resolveDependency( ForsetiLockManager.class ); + } } diff --git a/integrationtests/src/test/java/org/neo4j/storeupgrade/StoreUpgradeIntegrationTest.java b/integrationtests/src/test/java/org/neo4j/storeupgrade/StoreUpgradeIntegrationTest.java index 66fdc66adb87d..d0b8c078a049a 100644 --- a/integrationtests/src/test/java/org/neo4j/storeupgrade/StoreUpgradeIntegrationTest.java +++ b/integrationtests/src/test/java/org/neo4j/storeupgrade/StoreUpgradeIntegrationTest.java @@ -416,7 +416,7 @@ private static void checkIndexCounts( Store store, GraphDatabaseAPI db ) throws try ( KernelTransaction tx = kernel.newTransaction( KernelTransaction.Type.implicit, AnonymousContext.read() ); Statement statement = tx.acquireStatement() ) { - Iterator indexes = IndexDescriptor.sortByType( getAllIndexes( db ) ); + Iterator indexes = IndexDescriptor.sortByType( getAllIndexes( statement ) ); DoubleLongRegister register = Registers.newDoubleLongRegister(); for ( int i = 0; indexes.hasNext(); i++ ) { @@ -435,15 +435,9 @@ private static void checkIndexCounts( Store store, GraphDatabaseAPI db ) throws } } - private static Iterator getAllIndexes( GraphDatabaseAPI db ) + private static Iterator getAllIndexes( Statement statement ) { - try ( Transaction ignored = db.beginTx() ) - { - ThreadToStatementContextBridge bridge = db.getDependencyResolver() - .resolveDependency( ThreadToStatementContextBridge.class ); - Statement statement = bridge.get(); - return statement.readOperations().indexesGetAll(); - } + return statement.readOperations().indexesGetAll(); } private static void checkLabelCounts( GraphDatabaseAPI db )