From b9b302dce322922ca5a3a563427e5e5f81f812c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Finn=C3=A9?= Date: Wed, 28 Feb 2018 17:48:51 +0100 Subject: [PATCH] Fixes as issue with applying index updates to native unique indexes Given a uniqueness contraint on Foo:bar. If a transaction make changes such that in the end, the constraint is not violated but some intermediate individual step violates the constraint the index would still throw an exception. The reason this happens is that native index validates the uniqueness constraint on every insert (since doing so is for free). Although actually doing so when applying online updates has never been done in the lucene indexes because all that is handled in the kernel layer before committing a transaction anyway. Detecting a conflict while applying updates to a unique index means that there's a bug inside the kernel layer and would result in a kernel panic. Therefore we avoid this problem and do what we do for lucene indexes, namely by not checking conclicts when applying online updates. --- .../schema/ConflictDetectingValueMerger.java | 74 +++++++++------ .../NativeSchemaNumberIndexPopulator.java | 2 +- .../NativeSchemaNumberIndexUpdater.java | 20 ++--- .../ConflictDetectingValueMergerTest.java | 25 ++++-- .../NativeSchemaNumberIndexProviderTest.java | 30 +------ ...dentalUniquenessConstraintViolationIT.java | 90 +++++++++++++++++++ 6 files changed, 162 insertions(+), 79 deletions(-) create mode 100644 community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/index/AccidentalUniquenessConstraintViolationIT.java diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java index 7754f7f48290..466c5a2dbb95 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMerger.java @@ -20,54 +20,72 @@ package org.neo4j.kernel.impl.index.schema; import org.neo4j.index.internal.gbptree.ValueMerger; +import org.neo4j.index.internal.gbptree.ValueMergers; import org.neo4j.index.internal.gbptree.Writer; +import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException; +import org.neo4j.values.storable.Value; +import org.neo4j.values.storable.ValueTuple; /** * {@link ValueMerger} which will merely detect conflict, not change any value if conflict, i.e. if the * key already exists. After this merge has been used in a call to {@link Writer#merge(Object, Object, ValueMerger)} - * the {@link #wasConflict()} accessor can be called to check whether or not that call conflicted with - * an existing key. A call to {@link #wasConflict()} will also clear the conflict flag. + * {@link #checkConflict(Value[])} can be called to check whether or not that call conflicted with + * an existing key. A call to {@link #checkConflict(Value[])} will also clear the conflict flag. * * @param type of values being merged. */ -class ConflictDetectingValueMerger implements ValueMerger +abstract class ConflictDetectingValueMerger implements ValueMerger { - private boolean conflict; - private long existingNodeId; - private long addedNodeId; + /** + * @throw IndexEntryConflictException if merge conflicted with an existing key. This call also clears the conflict flag. + */ + abstract void checkConflict( Value[] values ) throws IndexEntryConflictException; - @Override - public VALUE merge( KEY existingKey, KEY newKey, VALUE existingValue, VALUE newValue ) + private static ConflictDetectingValueMerger DONT_CHECK = new ConflictDetectingValueMerger() { - if ( existingKey.entityId != newKey.entityId ) + @Override + public Object merge( Object existingKey, Object newKey, Object existingValue, Object newValue ) { - conflict = true; - existingNodeId = existingKey.entityId; - addedNodeId = newKey.entityId; + return null; } - return null; - } - /** - * @return whether or not merge conflicted with an existing key. This call also clears the conflict flag. - */ - boolean wasConflict() - { - boolean result = conflict; - if ( conflict ) + @Override + void checkConflict( Value[] values ) { - conflict = false; + // do nothing } - return result; - } + }; - long existingNodeId() + static ConflictDetectingValueMerger dontCheck() { - return existingNodeId; + return DONT_CHECK; } - long addedNodeId() + static class Check extends ConflictDetectingValueMerger { - return addedNodeId; + private boolean conflict; + private long existingNodeId; + private long addedNodeId; + + @Override + public VALUE merge( KEY existingKey, KEY newKey, VALUE existingValue, VALUE newValue ) + { + if ( existingKey.entityId != newKey.entityId ) + { + conflict = true; + existingNodeId = existingKey.entityId; + addedNodeId = newKey.entityId; + } + return null; + } + + void checkConflict( Value[] values ) throws IndexEntryConflictException + { + if ( conflict ) + { + conflict = false; + throw new IndexEntryConflictException( existingNodeId, addedNodeId, ValueTuple.of( values ) ); + } + } } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java index a901609ad361..11c8a6173de8 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexPopulator.java @@ -74,7 +74,7 @@ public abstract class NativeSchemaNumberIndexPopulator(); + this.conflictDetectingValueMerger = new ConflictDetectingValueMerger.Check<>(); } @Override diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java index 15eb2e1d1121..047fff56d318 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexUpdater.java @@ -27,12 +27,14 @@ import org.neo4j.kernel.api.index.IndexUpdater; import org.neo4j.values.storable.ValueTuple; +import static org.neo4j.kernel.impl.index.schema.ConflictDetectingValueMerger.dontCheck; + class NativeSchemaNumberIndexUpdater implements IndexUpdater { private final KEY treeKey; private final VALUE treeValue; - private final ConflictDetectingValueMerger conflictDetectingValueMerger; + private final ConflictDetectingValueMerger conflictDetectingValueMerger = dontCheck(); private Writer writer; private boolean closed = true; @@ -42,7 +44,6 @@ class NativeSchemaNumberIndexUpdater(); } NativeSchemaNumberIndexUpdater initialize( Writer writer, boolean manageClosingOfWriter ) @@ -124,7 +125,7 @@ private static vo treeKey.from( update.getEntityId(), update.values() ); treeValue.from( update.values() ); writer.merge( treeKey, treeValue, conflictDetectingValueMerger ); - assertNoConflict( update, conflictDetectingValueMerger ); + conflictDetectingValueMerger.checkConflict( update.values() ); } static void processAdd( KEY treeKey, VALUE treeValue, @@ -135,17 +136,6 @@ static void proce treeKey.from( update.getEntityId(), update.values() ); treeValue.from( update.values() ); writer.merge( treeKey, treeValue, conflictDetectingValueMerger ); - assertNoConflict( update, conflictDetectingValueMerger ); - } - - private static void assertNoConflict( IndexEntryUpdate update, - ConflictDetectingValueMerger conflictDetectingValueMerger ) throws IndexEntryConflictException - { - if ( conflictDetectingValueMerger.wasConflict() ) - { - long existingNodeId = conflictDetectingValueMerger.existingNodeId(); - long addedNodeId = conflictDetectingValueMerger.addedNodeId(); - throw new IndexEntryConflictException( existingNodeId, addedNodeId, ValueTuple.of( update.values() ) ); - } + conflictDetectingValueMerger.checkConflict( update.values() ); } } diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java index fad3089a2805..9d3da5ef1a48 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/ConflictDetectingValueMergerTest.java @@ -21,23 +21,28 @@ import org.junit.Test; +import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException; import org.neo4j.values.storable.Value; import org.neo4j.values.storable.Values; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.neo4j.helpers.ArrayUtil.array; +import static org.neo4j.values.storable.Values.stringValue; public class ConflictDetectingValueMergerTest { - private final ConflictDetectingValueMerger detector = new ConflictDetectingValueMerger<>(); + private final ConflictDetectingValueMerger detector = new ConflictDetectingValueMerger.Check<>(); @Test public void shouldReportConflictOnSameValueAndDifferentEntityIds() throws Exception { // given - Value value = Values.of( 123); + Value value = Values.of( 123 ); long entityId1 = 10; long entityId2 = 20; @@ -50,9 +55,17 @@ public void shouldReportConflictOnSameValueAndDifferentEntityIds() throws Except // then assertNull( merged ); - assertTrue( detector.wasConflict() ); - assertEquals( entityId1, detector.existingNodeId() ); - assertEquals( entityId2, detector.addedNodeId() ); + try + { + detector.checkConflict( array( value ) ); + fail( "Should've detected conflict" ); + } + catch ( IndexEntryConflictException e ) + { + assertEquals( entityId1, e.getExistingNodeId() ); + assertEquals( entityId2, e.getAddedNodeId() ); + assertEquals( value, e.getSinglePropertyValue() ); + } } @Test @@ -71,7 +84,7 @@ public void shouldNotReportConflictOnSameValueSameEntityId() throws Exception // then assertNull( merged ); - assertFalse( detector.wasConflict() ); + detector.checkConflict( array() ); // <-- should not throw conflict exception } private static SchemaNumberKey key( long entityId, Value... value ) diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexProviderTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexProviderTest.java index ea5101175e0a..a1aa6c165845 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexProviderTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/index/schema/NativeSchemaNumberIndexProviderTest.java @@ -29,7 +29,6 @@ import org.neo4j.io.fs.FileSystemAbstraction; import org.neo4j.io.pagecache.PageCache; -import org.neo4j.kernel.api.exceptions.index.IndexEntryConflictException; import org.neo4j.kernel.api.index.IndexAccessor; import org.neo4j.kernel.api.index.IndexEntryUpdate; import org.neo4j.kernel.api.index.IndexPopulator; @@ -124,7 +123,7 @@ public void getPopulatorMustCreateNonUniquePopulatorForTypeGeneral() throws Exce /* getOnlineAccessor */ @Test - public void getOnlineAccessorMustCreateUniqueAccessorForTypeUnique() throws Exception + public void shouldNotCheckConflictsWhenApplyingUpdatesInOnlineAccessor() throws Exception { // given provider = newProvider(); @@ -137,33 +136,6 @@ public void getOnlineAccessorMustCreateUniqueAccessorForTypeUnique() throws Exce Value value = Values.intValue( 1 ); indexUpdater.process( IndexEntryUpdate.add( 1, descriptor.schema(), value ) ); - // then - try - { - indexUpdater.process( IndexEntryUpdate.add( 2, descriptor.schema(), value ) ); - fail( "Should have failed" ); - } - catch ( IndexEntryConflictException e ) - { - // good - } - } - } - - @Test - public void getOnlineAccessorMustCreateNonUniqueAccessorForTypeGeneral() throws Exception - { - // given - provider = newProvider(); - - // when - IndexDescriptor descriptor = descriptor(); - try ( IndexAccessor accessor = provider.getOnlineAccessor( indexId, descriptor, samplingConfig() ); - IndexUpdater indexUpdater = accessor.newUpdater( IndexUpdateMode.ONLINE ) ) - { - Value value = Values.intValue( 1 ); - indexUpdater.process( IndexEntryUpdate.add( 1, descriptor.schema(), value ) ); - // then // ... expect no failure on duplicate value indexUpdater.process( IndexEntryUpdate.add( 2, descriptor.schema(), value ) ); diff --git a/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/index/AccidentalUniquenessConstraintViolationIT.java b/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/index/AccidentalUniquenessConstraintViolationIT.java new file mode 100644 index 000000000000..d6953a6f2189 --- /dev/null +++ b/community/lucene-index/src/test/java/org/neo4j/kernel/api/impl/index/AccidentalUniquenessConstraintViolationIT.java @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2002-2018 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.kernel.api.impl.index; + +import org.junit.Rule; +import org.junit.Test; + +import java.util.Map; + +import org.neo4j.graphdb.Label; +import org.neo4j.graphdb.Node; +import org.neo4j.graphdb.NotFoundException; +import org.neo4j.graphdb.Transaction; +import org.neo4j.test.rule.DatabaseRule; +import org.neo4j.test.rule.ImpermanentDatabaseRule; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +public class AccidentalUniquenessConstraintViolationIT +{ + private static final Label Foo = Label.label( "Foo" ); + private static final String BAR = "bar"; + + @Rule + public final DatabaseRule db = new ImpermanentDatabaseRule(); + + @Test + public void shouldApplyChangesWithIntermediateConstraintViolations() throws Exception + { + // given + try ( Transaction tx = db.beginTx() ) + { + db.schema().constraintFor( Foo ).assertPropertyIsUnique( BAR ).create(); + tx.success(); + } + Node fourtyTwo; + Node fourtyOne; + try ( Transaction tx = db.beginTx() ) + { + fourtyTwo = db.createNode( Foo ); + fourtyTwo.setProperty( BAR, 42 ); + fourtyOne = db.createNode( Foo ); + fourtyOne.setProperty( BAR, 41 ); + tx.success(); + } + + // when + try ( Transaction tx = db.beginTx() ) + { + Map props = fourtyOne.getAllProperties(); + fourtyOne.delete(); + fourtyTwo.setProperty( BAR, props.get( BAR ) ); + tx.success(); + } + + // then + try ( Transaction tx = db.beginTx() ) + { + assertEquals( 41, fourtyTwo.getProperty( BAR ) ); + try + { + fourtyOne.getProperty( BAR ); + fail( "Should be deleted" ); + } + catch ( NotFoundException e ) + { + // good + } + tx.success(); + } + } +}