Unified add/change property at commit

Transaction state before commit time represents logical changes that
are to be converted to physical changes during commit. This is true
for everything except property add/change. User can only "set"
properties, not differentiate between add or change by intent.
Yet the transaction state would capture and keep state for every
property whether or not it was an added or changed property,
i.e. whether or not the property already existed in the store at
the time of making the change in the transaction (state).
This information would then affect how the property changes would
be converted into physical changes, i.e. the logical transaction state
contained actual pysical state, which is fundamentally wrong.
This happened to work since write locks were acquired before reading
physical state affecting how a property change was represented
in the logical transaction state. With different locking strategies
this problem was exposed and so have to be fixed.

Now property changes will be treated simply as "set" operation,
i.e. "add or change" when converting logical state to physical
at commit time.

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

@@ -87,16 +87,10 @@ public <T extends PrimitiveRecord> void removeProperty( RecordProxy<Long,T,Void>
         propertyDeleter.removeProperty( primitiveProxy, propertyKey, getPropertyRecords() );
     }
 
-    public <P extends PrimitiveRecord> void primitiveChangeProperty( RecordProxy<Long, P, Void> primitive,
+    public <P extends PrimitiveRecord> void primitiveSetProperty( RecordProxy<Long, P, Void> primitive,
             int propertyKey, Object value )
     {
-        propertyCreator.primitiveChangeProperty( primitive, propertyKey, value, getPropertyRecords() );
-    }
-
-    public <P extends PrimitiveRecord> void primitiveAddProperty( RecordProxy<Long, P, Void> primitive,
-            int propertyKey, Object value )
-    {
-        propertyCreator.primitiveAddProperty( primitive, propertyKey, value, getPropertyRecords() );
+        propertyCreator.primitiveSetProperty( primitive, propertyKey, value, getPropertyRecords() );
     }
 
     public void createPropertyKeyToken( String name, int id )

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

@@ -53,130 +53,146 @@ public PropertyCreator( DynamicRecordAllocator stringRecordAllocator, DynamicRec
         this.traverser = traverser;
     }
 
-    public <P extends PrimitiveRecord> void primitiveChangeProperty(
+    public <P extends PrimitiveRecord> void primitiveSetProperty(
             RecordProxy<Long, P, Void> primitiveRecordChange, int propertyKey, Object value,
             RecordAccess<Long, PropertyRecord, PrimitiveRecord> propertyRecords )
     {
-        P primitive = primitiveRecordChange.forReadingLinkage();
-        assert traverser.assertPropertyChain( primitive, propertyRecords );
-        long propertyId = // propertyData.getId();
-                traverser.findPropertyRecordContaining( primitive, propertyKey, propertyRecords, true );
-        PropertyRecord propertyRecord = propertyRecords.getOrLoad( propertyId, primitive ).forChangingData();
-        if ( !propertyRecord.inUse() )
-        {
-            throw new IllegalStateException( "Unable to change property["
-                                             + propertyId
-                                             + "] since it has been deleted." );
-        }
-        PropertyBlock block = propertyRecord.getPropertyBlock( propertyKey );
-        if ( block == null )
-        {
-            throw new IllegalStateException( "Property with index["
-                                             + propertyKey
-                                             + "] is not present in property["
-                                             + propertyId + "]" );
-        }
-        propertyRecord.setChanged( primitive );
-        for ( DynamicRecord record : block.getValueRecords() )
-        {
-            assert record.inUse();
-            record.setInUse( false, block.getType().intValue() );
-            propertyRecord.addDeletedRecord( record );
-        }
-        encodeValue( block, propertyKey, value );
-        if ( propertyRecord.size() > PropertyType.getPayloadSize() )
-        {
-            propertyRecord.removePropertyBlock( propertyKey );
-            /*
-             * The record should never, ever be above max size. Less obviously, it should
-             * never remain empty. If removing a property because it won't fit when changing
-             * it leaves the record empty it means that this block was the last one which
-             * means that it doesn't fit in an empty record. Where i come from, we call this
-             * weird.
-             *
-             assert propertyRecord.size() <= PropertyType.getPayloadSize() : propertyRecord;
-             assert propertyRecord.size() > 0 : propertyRecord;
-             */
-            addPropertyBlockToPrimitive( block, primitiveRecordChange, propertyRecords );
-        }
-        assert traverser.assertPropertyChain( primitive, propertyRecords );
-    }
-
-    public PropertyBlock encodePropertyValue( int propertyKey, Object value )
-    {
-        return encodeValue( new PropertyBlock(), propertyKey, value );
-    }
-
-    public PropertyBlock encodeValue( PropertyBlock block, int propertyKey, Object value )
-    {
-        PropertyStore.encodeValue( block, propertyKey, value, stringRecordAllocator, arrayRecordAllocator );
-        return block;
-    }
-
-    public <P extends PrimitiveRecord> void primitiveAddProperty(
-            RecordProxy<Long, P, Void> primitive, int propertyKey, Object value,
-            RecordAccess<Long, PropertyRecord, PrimitiveRecord> propertyRecords )
-    {
-        P record = primitive.forReadingLinkage();
-        assert traverser.assertPropertyChain( record, propertyRecords );
-        PropertyBlock block = new PropertyBlock();
-        encodeValue( block, propertyKey, value );
-        addPropertyBlockToPrimitive( block, primitive, propertyRecords );
-        assert traverser.assertPropertyChain( record, propertyRecords );
-    }
-
-    private <P extends PrimitiveRecord> void addPropertyBlockToPrimitive(
-            PropertyBlock block, RecordProxy<Long, P, Void> primitiveRecordChange,
-            RecordAccess<Long, PropertyRecord, PrimitiveRecord> propertyRecords )
-    {
+        PropertyBlock block = encodePropertyValue( propertyKey, value );
         P primitive = primitiveRecordChange.forReadingLinkage();
         assert traverser.assertPropertyChain( primitive, propertyRecords );
         int newBlockSizeInBytes = block.getSize();
 
-        // Scan the property record chain for a place to fit this property block
-        PropertyRecord host = null;
+        // Traverse the existing property chain. Tracking two things along the way:
+        // - (a) Free space for this block (candidateHost)
+        // - (b) Existence of a block with the property key
+        // Chain traversal can be aborted only if:
+        // - (1) (b) occurs and new property block fits where the current is
+        // - (2) (a) occurs and (b) has occurred, but new property block didn't fit
+        // - (3) Chain ends
+        RecordProxy<Long, PropertyRecord, PrimitiveRecord> freeHostProxy = null;
+        RecordProxy<Long, PropertyRecord, PrimitiveRecord> existingHostProxy = null;
         long prop = primitive.getNextProp();
-        while ( prop != Record.NO_NEXT_PROPERTY.intValue() )
+        while ( prop != Record.NO_NEXT_PROPERTY.intValue() ) // <-- (3)
         {
-            // We do not store in map - might not have enough space
-            RecordProxy<Long, PropertyRecord, PrimitiveRecord> change =
+            RecordProxy<Long, PropertyRecord, PrimitiveRecord> proxy =
                     propertyRecords.getOrLoad( prop, primitive );
-            PropertyRecord propRecord = change.forReadingLinkage();
+            PropertyRecord propRecord = proxy.forReadingLinkage();
             assert propRecord.inUse() : propRecord;
-            int propSize = propRecord.size();
-            assert propSize > 0 : propRecord;
-            if ( propSize + newBlockSizeInBytes <= PropertyType.getPayloadSize() )
+
+            // (a) search for free space
+            if ( propertyFitsInside( newBlockSizeInBytes, propRecord ) )
+            {
+                freeHostProxy = proxy;
+                if ( existingHostProxy != null )
+                {
+                    // (2)
+                    PropertyRecord freeHost = proxy.forChangingData();
+                    freeHost.addPropertyBlock( block );
+                    freeHost.setChanged( primitive );
+                    assert traverser.assertPropertyChain( primitive, propertyRecords );
+                    return;
+                }
+            }
+
+            // (b) search for existence of property key
+            PropertyBlock existingBlock = propRecord.getPropertyBlock( propertyKey );
+            if ( existingBlock != null )
             {
-                propRecord = change.forChangingData();
-                host = propRecord;
-                host.addPropertyBlock( block );
-                host.setChanged( primitive );
-                break;
+                // We found an existing property and whatever happens we have to remove the existing
+                // block so that we can add the new one, where ever we decide to place it
+                existingHostProxy = proxy;
+                PropertyRecord existingHost = existingHostProxy.forChangingData();
+                removeProperty( primitive, existingHost, existingBlock );
+
+                // Now see if we at this point can add the new block
+                if ( newBlockSizeInBytes <= existingBlock.getSize() || // cheap check
+                     propertyFitsInside( newBlockSizeInBytes, existingHost ) ) // fallback check
+                {
+                    // (1) yes we could add it right into the host of the existing block
+                    existingHost.addPropertyBlock( block );
+                    assert traverser.assertPropertyChain( primitive, propertyRecords );
+                    return;
+                }
+                else if ( freeHostProxy != null )
+                {
+                    // (2) yes we could add it to a previously found host with sufficiently free space in it
+                    PropertyRecord freeHost = freeHostProxy.forChangingData();
+                    freeHost.addPropertyBlock( block );
+                    freeHost.setChanged( primitive );
+                    assert traverser.assertPropertyChain( primitive, propertyRecords );
+                    return;
+                }
+                // else we can't add it at this point
             }
+
+            // Continue down the chain
             prop = propRecord.getNextProp();
         }
 
-        if ( host == null )
+        // At this point we haven't added the property block, although we may have found room for it
+        // along the way. If we didn't then just create a new record, it's fine
+        PropertyRecord freeHost = null;
+        if ( freeHostProxy == null )
         {
-            // There was no room for the propery block in any record in the chain, make new one
-            host = propertyRecords.create( propertyRecordIdGenerator.nextId(), primitive ).forChangingData();
+            // We couldn't find free space along the way, so create a new host record
+            freeHost = propertyRecords.create( propertyRecordIdGenerator.nextId(), primitive ).forChangingData();
+            freeHost.setInUse( true );
             if ( primitive.getNextProp() != Record.NO_NEXT_PROPERTY.intValue() )
             {
+                // This isn't the first property record for the entity, re-shuffle the first one so that
+                // the new one becomes the first
                 PropertyRecord prevProp = propertyRecords.getOrLoad( primitive.getNextProp(),
                         primitive ).forChangingLinkage();
                 assert prevProp.getPrevProp() == Record.NO_PREVIOUS_PROPERTY.intValue();
-                prevProp.setPrevProp( host.getId() );
-                host.setNextProp( prevProp.getId() );
+                prevProp.setPrevProp( freeHost.getId() );
+                freeHost.setNextProp( prevProp.getId() );
                 prevProp.setChanged( primitive );
             }
-            primitiveRecordChange.forChangingLinkage().setNextProp( host.getId() );
-            host.addPropertyBlock( block );
-            host.setInUse( true );
+
+            // By the way, this is the only condition where the primitive record also needs to change
+            primitiveRecordChange.forChangingLinkage().setNextProp( freeHost.getId() );
+        }
+        else
+        {
+            freeHost = freeHostProxy.forChangingData();
         }
-        // Ok, here host does for the job. Use it
+
+        // At this point we know that we have a host record with sufficient space in it for the block
+        // to add, so simply add it
+        freeHost.addPropertyBlock( block );
         assert traverser.assertPropertyChain( primitive, propertyRecords );
     }
 
+    private void removeProperty( PrimitiveRecord primitive, PropertyRecord host, PropertyBlock block )
+    {
+        host.removePropertyBlock( block.getKeyIndexId() );
+        host.setChanged( primitive );
+        for ( DynamicRecord record : block.getValueRecords() )
+        {
+            assert record.inUse();
+            record.setInUse( false, block.getType().intValue() );
+            host.addDeletedRecord( record );
+        }
+    }
+
+    private boolean propertyFitsInside( int newBlockSizeInBytes, PropertyRecord propRecord )
+    {
+        int propSize = propRecord.size();
+        assert propSize >= 0 : propRecord;
+        return propSize + newBlockSizeInBytes <= PropertyType.getPayloadSize();
+    }
+
+    public PropertyBlock encodePropertyValue( int propertyKey, Object value )
+    {
+        return encodeValue( new PropertyBlock(), propertyKey, value );
+    }
+
+    public PropertyBlock encodeValue( PropertyBlock block, int propertyKey, Object value )
+    {
+        PropertyStore.encodeValue( block, propertyKey, value, stringRecordAllocator, arrayRecordAllocator );
+        return block;
+    }
+
     public long createPropertyChain( PrimitiveRecord owner, Iterator<PropertyBlock> properties,
             RecordAccess<Long, PropertyRecord, PrimitiveRecord> propertyRecords )
     {

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

@@ -23,6 +23,7 @@
 import java.util.LinkedList;
 import java.util.List;
 
+import org.neo4j.kernel.impl.store.PropertyType;
 import org.neo4j.kernel.impl.store.record.PrimitiveRecord;
 import org.neo4j.kernel.impl.store.record.PropertyBlock;
 import org.neo4j.kernel.impl.store.record.PropertyRecord;
@@ -81,6 +82,7 @@ public boolean assertPropertyChain( PrimitiveRecord primitive,
             toCheck.add( propRecord );
             assert propRecord.inUse() : primitive + "->"
                                         + Arrays.toString( toCheck.toArray() );
+            assert propRecord.size() <= PropertyType.getPayloadSize() : propRecord + " size " + propRecord.size();
             nextIdToFetch = propRecord.getNextProp();
         }
         if ( toCheck.isEmpty() )

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

@@ -361,7 +361,7 @@ public void nodeRemoveProperty( long nodeId, int propertyKey )
     public DefinedProperty relChangeProperty( long relId, int propertyKey, Object value )
     {
         RecordProxy<Long, RelationshipRecord, Void> rel = context.getRelRecords().getOrLoad( relId, null );
-        context.primitiveChangeProperty( rel, propertyKey, value );
+        context.primitiveSetProperty( rel, propertyKey, value );
         return Property.property( propertyKey, value );
     }
 
@@ -377,7 +377,7 @@ public DefinedProperty relChangeProperty( long relId, int propertyKey, Object va
     public DefinedProperty nodeChangeProperty( long nodeId, int propertyKey, Object value )
     {
         RecordProxy<Long, NodeRecord, Void> node = context.getNodeRecords().getOrLoad( nodeId, null ); //getNodeRecord( nodeId );
-        context.primitiveChangeProperty( node, propertyKey, value );
+        context.primitiveSetProperty( node, propertyKey, value );
         return Property.property( propertyKey, value );
     }
 
@@ -393,7 +393,7 @@ public DefinedProperty nodeChangeProperty( long nodeId, int propertyKey, Object
     public DefinedProperty relAddProperty( long relId, int propertyKey, Object value )
     {
         RecordProxy<Long, RelationshipRecord, Void> rel = context.getRelRecords().getOrLoad( relId, null );
-        context.primitiveAddProperty( rel, propertyKey, value );
+        context.primitiveSetProperty( rel, propertyKey, value );
         return Property.property( propertyKey, value );
     }
 
@@ -408,7 +408,7 @@ public DefinedProperty relAddProperty( long relId, int propertyKey, Object value
     public DefinedProperty nodeAddProperty( long nodeId, int propertyKey, Object value )
     {
         RecordProxy<Long, NodeRecord, Void> node = context.getNodeRecords().getOrLoad( nodeId, null );
-        context.primitiveAddProperty( node, propertyKey, value );
+        context.primitiveSetProperty( node, propertyKey, value );
         return Property.property( propertyKey, value );
     }
 
@@ -538,7 +538,7 @@ public NeoStoreRecord clone(NeoStoreRecord neoStoreRecord ) {
      */
     public DefinedProperty graphAddProperty( int propertyKey, Object value )
     {
-        context.primitiveAddProperty( getOrLoadNeoStoreRecord(), propertyKey, value );
+        context.primitiveSetProperty( getOrLoadNeoStoreRecord(), propertyKey, value );
         return Property.property( propertyKey, value );
     }
 
@@ -552,7 +552,7 @@ public DefinedProperty graphAddProperty( int propertyKey, Object value )
      */
     public DefinedProperty graphChangeProperty( int propertyKey, Object value )
     {
-        context.primitiveChangeProperty( getOrLoadNeoStoreRecord(), propertyKey, value );
+        context.primitiveSetProperty( getOrLoadNeoStoreRecord(), propertyKey, value );
         return Property.property( propertyKey, value );
     }
 

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

@@ -405,24 +405,13 @@ public IndexCreator createDeferredSchemaIndex( Label label )
         return new IndexCreatorImpl( actions, label );
     }
 
-    private void removePropertyIfExist( RecordProxy<Long, ? extends PrimitiveRecord,Void> recordProxy,
-            int propertyKey, RecordAccess<Long,PropertyRecord,PrimitiveRecord> propertyRecords )
-    {
-        if ( propertyTraverser.findPropertyRecordContaining( recordProxy.forReadingData(),
-                propertyKey, propertyRecords, false ) != Record.NO_NEXT_PROPERTY.intValue() )
-        {
-            propertyDeletor.removeProperty( recordProxy, propertyKey, propertyRecords );
-        }
-    }
-
     private void setPrimitiveProperty( RecordProxy<Long,? extends PrimitiveRecord,Void> primitiveRecord,
             String propertyName, Object propertyValue )
     {
         int propertyKey = getOrCreatePropertyKeyId( propertyName );
         RecordAccess<Long,PropertyRecord,PrimitiveRecord> propertyRecords = recordAccess.getPropertyRecords();
 
-        removePropertyIfExist( primitiveRecord, propertyKey, propertyRecords );
-        propertyCreator.primitiveAddProperty( primitiveRecord, propertyKey, propertyValue, propertyRecords );
+        propertyCreator.primitiveSetProperty( primitiveRecord, propertyKey, propertyValue, propertyRecords );
     }
 
     private void validateIndexCanBeCreated( int labelId, int propertyKeyId )