Skip to content

Marten natural-key batch enlistment is dead code, and the SQLite deduplication reaper is unbounded #4567

Description

@jeremydmiller

Two small things found while implementing #4505, both in the deduplication / batching neighbourhood.

1. LoadAggregateFrame's natural-key batch enlistment is unreachable

src/Persistence/Wolverine.Marten/Codegen/LoadAggregateFrame.cs:

public void WriteCodeToEnlistInBatchQuery(GeneratedMethod method, ISourceWriter writer)
{
    if (_att.IsNaturalKey)
    {
        writer.WriteLine($"var {_batchQueryItem!.Usage} = {NaturalKeyFetchForWriting(_batchQuery!.Usage)};");
        return;
    }
    ...

WriteCodeToEnlistInBatchQuery is only ever called from MartenBatchFrame.GenerateCode, over frames that
MartenBatchingPolicy enlisted. That policy explicitly refuses natural-key loads:

// src/Persistence/Wolverine.Marten/Codegen/MartenQueryingFrame.cs
private static bool IsBatchable(IBatchableFrame frame)
{
    // Natural key aggregate loads cannot be batched because IBatchedQuery
    // does not have a FetchForWriting<T, TNaturalKey> overload
    if (frame is LoadAggregateFrame laf && laf.IsNaturalKey) return false;

So the branch can never run — and if it ever did it would emit
batchQuery.Events.FetchForWriting<T, TNaturalKey>(...), which is precisely the overload the policy's
comment says does not exist, i.e. it would not compile. It should be deleted, or replaced with a
throw new InvalidOperationException that says the policy is supposed to have excluded this frame, so a
future change to IsBatchable fails loudly instead of emitting uncompilable code.

Spotted originally as an aside in #4505; recording it here so it does not disappear with that issue.

2. The SQLite deduplication reaper is unbounded

RdbmsDeduplicationStore.DeleteExpiredAsync asks the provider for a bounded delete and falls back to one
unbounded statement when it gets null:

var batched = _batchedDeleteExpiredSql(_batchSize);

if (batched.IsEmpty())
{
    // This provider cannot bound the delete. Still correct, just one statement.
    await using var single = _dataSource.CreateCommand(_deleteExpiredSql).With("now", utcNow);
    return await single.ExecuteNonQueryAsync(cancellation).ConfigureAwait(false);
}

BatchedDeleteExpiredDeduplicationClaimsSql is overridden by PostgresqlMessageStore and
SqlServerMessageStore. SqliteMessageStore does not override it, so SQLite — and therefore every
Fisher-backed application — takes the unbounded path.

The bounding exists for a reason the class doc states plainly: "a busy chain under a 24-hour window
accumulates a day's worth of rows, and reaping them in one unbounded statement is exactly the long-lock
problem that moved the handled-envelope cleanup onto its own timer in the first place (issue #3116)". On
SQLite a long write is worse than elsewhere, because it is a file-level write lock and there is only one
writer.

SQLite can express the bound — delete from t where rowid in (select rowid from t where expires <= @now limit N) — so this looks like a straightforward override plus a test. Worth checking whether
SqliteMessageStore has the same gap for BatchedDeleteExpiredHandledEnvelopesSql.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions