Skip to content

Conversation

@Arkatufus
Copy link
Contributor

@Arkatufus Arkatufus commented May 21, 2025

Note

These changes are safe because we're using transactions, the old JVM implementation queries were designed that way because they have to account for possible non-transaction delete failures.
We need transaction guarantee because we have to write to multiple tables to support backward compatibility and tag table indexing.

Changes

Optimize event journal delete code

  • Shorten the codes living inside the transaction block
  • Optimize SQL query

Copy link
Contributor Author

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review

_useCloneDataConnection = config.UseCloneConnection;

if (_opts.RetryPolicyOptions.RetryPolicy is null && _opts.ConnectionOptions.ProviderName!.ToLowerInvariant().StartsWith("sqlserver"))
if (_opts.RetryPolicyOptions.RetryPolicy is null && _opts.RetryPolicyOptions.Factory is null && _opts.ConnectionOptions.ProviderName!.ToLowerInvariant().StartsWith("sqlserver"))
Copy link
Contributor Author

@Arkatufus Arkatufus May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add missing retry policy factory check. The Factory (if not null) is used internally by Linq2Db to populate DataConnection.RetryPolicy if the DataOption.RetryPolicyOptions.RetryPolicy is null.

Comment on lines 118 to 121
await using (var connection = ConnectionFactory.GetConnection())
{
maxMarkedDeletion = await MaxMarkedForDeletionMaxPersistenceIdQuery(connection, persistenceId, maxSequenceNr).FirstOrDefaultAsync(ShutdownToken);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retrieve the highest affected sequence number that is affected by the delete operation. No need to use transaction, this is strictly a read operation.

Comment on lines +123 to +124
if (maxMarkedDeletion is 0)
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early bail out condition, just return if there's nothing to delete.

r =>
r.PersistenceId == persistenceId &&
r.SequenceNumber <= maxSequenceNr)
r.SequenceNumber == maxMarkedDeletion)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of marking all of the records less than the requested sequence number as deleted, we mark the highest affected instead. Update 1 record instead of many.

Comment on lines +140 to +145
await journalTable
.Where(
r =>
r.PersistenceId == persistenceId &&
r.SequenceNumber < maxMarkedDeletion)
.DeleteAsync(token);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify the deletion query, just delete anything that is less than the highest affected row.

=> connection
.GetTable<JournalRow>()
.Where(r => r.PersistenceId == persistenceId && r.Deleted)
.Where(r => r.PersistenceId == persistenceId && r.SequenceNumber <= maxSequenceNr)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimization, instead of checking against persistence id and deleted field, check against persistence id and sequence number instead, as they are indexed.

Copy link
Member

@to11mtm to11mtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me

Arkatufus added 2 commits May 22, 2025 00:16
# Conflicts:
#	src/Akka.Persistence.Sql/Journal/Dao/BaseByteArrayJournalDao.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants