Version Column causes extra update and if no fields are updated gets updated anyway

Issue #192 resolved
Todd Flora
created an issue

Two Questions / Issues here:

TUpdateExecutor uses two queries to update when version column is defined instead of one

This might not seem like a big deal but when dealing with a large number of records this extra update wastes valuable processing time.

Please consider the following code and comments from Spring.Persistence.SQL.Commands.Update

procedure TUpdateExecutor.Execute(const entity: TObject);
var
  statement: IDBStatement;
  sqlStatement: string;
begin
  Assert(Assigned(entity));

  if EntityData.HasVersionColumn and not TryIncrementVersionFor(entity) then 
    raise EORMOptimisticLockException.Create(entity);

The TryIncrementVersionFor function actually performs an update against the database and attempts to increment the version.

  statement := Connection.CreateStatement;

  fColumns.Clear;
  if fEntityMap.IsMapped(entity) then
  begin
    fColumns := fEntityMap.GetChangedMembers(entity, EntityData);
    if HasChangedVersionColumnOnly then
      Exit;
  end
  else
    fColumns.AddRange(EntityData.Columns);

Next it seems that it checks to see if any other fields have been modified and exits if none have, but since the Version is already incremented it is not rolled back

  fCommand.SetCommandFieldsFromColumns(fColumns);

  if not fCommand.UpdateFields.Any then
    Exit;

  fCommand.Entity := entity;
  sqlStatement := Generator.GenerateUpdate(fCommand);
  if sqlStatement = '' then
    raise EORMCannotGenerateQueryStatement.Create(entity);

  statement.SetSQLCommand(sqlStatement);
  BuildParams(entity);
  statement.SetParams(SQLParameters);
  statement.Execute;
end;

Finally the actual update is performed.

The Optimistic lock functionality could be accomplished with just one statement if the version attribute were checked and incremented in the actual update.

For instance: If the Underlying update query looked as follows:

Update TableName 
Set VersionColumn = VersionColumn + 1, 
      Other Columns,
where VersionColumn = :CurrentVersionNumber,
            Other Where Columns;

Now all that needs to be done is to check that rows affected is equal to zero, and if so throw the Optimistic Lock Exception.

When there is nothing to udpate, instead of exiting without updating the row version is updated anyway

This is not critical but as seen in the center code block code checks to see if only the version column was updated and just exits and does not perform the update if that is the only column updated. This is awesome but because the previous TryIncrementVersionFor already ran it seems that the version value gets updated anyway in the database. It would be really nice if the version value was not updated if no fields changed.

This would also be accomplished if the entire update was done in one statement as described in number 1 above.

Comments (7)

  1. Todd Flora reporter

    One thing I missed on this is to increment the version value on the Entity.

    So the last line in the code snipet below is not in the pas unit, but is necessary.

    procedure TUpdateExecutor.Execute(const entity: TObject);
    var
      statement: IDBStatement;
      sqlStatement: string;
      RowsAffected : Integer;
    begin
      Assert(Assigned(entity));
    
      fColumns.Clear;
      if fEntityMap.IsMapped(entity) then
      begin
        fColumns := fEntityMap.GetChangedMembers(entity, EntityData);
        if (FColumns.Count = 0) then  // No Columns to update
          Exit;
      end
      else
        fColumns.AddRange(EntityData.Columns);
    
      fCommand.VersionColumn := entityData.VersionColumn;
      fCommand.SetCommandFieldsFromColumns(fColumns);
      if not fCommand.UpdateFields.Any then
        Exit;
    
      statement := Connection.CreateStatement;
    
      fCommand.Entity := entity;
      sqlStatement := Generator.GenerateUpdate(fCommand);
      if sqlStatement = '' then
        raise EORMCannotGenerateQueryStatement.Create(entity);
    
      statement.SetSQLCommand(sqlStatement);
      BuildParams(entity);
      statement.SetParams(SQLParameters);
      RowsAffected := statement.Execute;
    
      if EntityData.HasVersionColumn and (RowsAffected = 0) then
        raise EORMOptimisticLockException.Create(entity);
    
      EntityData.VersionColumn.Member.SetValue(entity, EntityData.VersionColumn.Member.GetValue(entity).AsInteger + 1);  <  -- Need to update the Version Column value here.
    end;
    
  2. Todd Flora reporter

    OOPS actually the last line needs to be

     if Assigned(EntityData.VersionColumn)
       then EntityData.VersionColumn.Member.SetValue(entity, EntityData.VersionColumn.Member.GetValue(entity).AsInteger + 1);
    
  3. Log in to comment