Bound the per-database watermark read to the clamp horizon (#2344) #2346
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -430,9 +430,16 @@ store round-trips on cancellationToken made THIS loop — the one the field repo | |
| specifically: a budget expiry abandons the whole pass, so the watermark does not | ||
| advance, the clamp is re-derived next cycle, and the hole is re-recorded (merged wider | ||
| with any already pending) rather than lost. */ | ||
| /* #2344: same bound as the enumerated arm. Safe here for the same reason and | ||
| by a different route — this branch does not clamp itself, but query_store's own | ||
| BuildCutoffParameters does (the #1836 double-clamp the policy documents), so the | ||
| value this read returns is clamped before anything uses it. */ | ||
| var azureReadFloor = string.Equals(definition.Name, QueryStoreCollector.Instance.Name, StringComparison.Ordinal) | ||
| ? WatermarkPolicy.ReadFloor(collectionTime) | ||
| : null; | ||
|
Comment on lines
+433
to
+439
There was a problem hiding this comment. Bounding the read here can silently drop a real backfill hole. If the true stored watermark is older than Worth considering: either read the raw watermark unbounded specifically for this hole-recording path, or explicitly detect "found nothing because it's outside the read floor" vs. "genuinely never collected" so the hole can still be recorded (even if only approximately, e.g. from the read floor itself). |
||
| context.Watermark = await GetLastCollectedTimeForDatabaseAsync( | ||
| server.ServerId, definition.TargetTable, definition.WatermarkColumn!, | ||
| definition.PerDatabaseWatermarkColumn!, databaseName, dbToken, azureReadFloor); | ||
|
|
||
| /* #2111 adaptive shrink, Azure arm — tighten BEFORE BuildQuery: the | ||
| definition's own clamp only floors OLDER watermarks, so a tighter one | ||
|
|
@@ -745,9 +752,17 @@ would otherwise be silently counted as row-streaming time. Measured here so | |
| DrainMsFrom can subtract it; the whole point of the split is that each number | ||
| names one real phase. */ | ||
| var watermarkWatch = Stopwatch.StartNew(); | ||
| /* #2344: bound the read for the ONE collector whose value is clamped right | ||
| below. Name-guarded rather than applied to every enumerating definition, | ||
| for the reason WatermarkPolicy's remarks give: a ring-buffer source whose | ||
| legitimate catch-up spans days must keep reading its whole history, and the | ||
| floor would silently truncate it. The clamp and the bound travel together. */ | ||
| var readFloor = string.Equals(definition.Name, QueryStoreCollector.Instance.Name, StringComparison.Ordinal) | ||
| ? WatermarkPolicy.ReadFloor(collectionTime) | ||
| : null; | ||
|
Comment on lines
+755
to
+762
There was a problem hiding this comment. Same issue as the Azure arm above, on this enumerated path: with the bound in place, |
||
| var raw = await GetLastCollectedTimeForDatabaseAsync( | ||
| server.ServerId, definition.TargetTable, definition.WatermarkColumn!, | ||
| definition.PerDatabaseWatermarkColumn!, item, ct, readFloor); | ||
| var clamped = WatermarkPolicy.ClampCatchup(raw, collectionTime); | ||
| if (raw.HasValue && clamped != raw) | ||
| { | ||
|
|
@@ -2076,17 +2091,35 @@ public async Task<int> WriteBackfillBatchAsync<TRow>( | |
| /// value for ONE database, for definitions with a PerDatabaseWatermarkColumn (Azure SQL DB | ||
| /// per-database XE capture, #1535). Null on first run for that database or on failure — the | ||
| /// caller falls back to the definition's documented window. | ||
| /// | ||
| /// <para><paramref name="collectedSince"/> bounds the read on <c>collection_time</c> — the | ||
| /// PARTITIONING column, so the bound actually prunes chunks (#2344). Null keeps the unbounded | ||
| /// behaviour, which is correct for any reader whose watermark is NOT clamped; pass | ||
| /// <see cref="WatermarkPolicy.ReadFloor"/> only from a caller whose value is, and read that method's | ||
| /// remarks for why the bound provably changes no answer. Unbounded, this is a <c>MAX</c> over a | ||
| /// non-partitioning column with no time predicate — every chunk in retention, per database, per | ||
| /// cycle, at a cost that grows with the store rather than the workload.</para> | ||
| /// </summary> | ||
| public async Task<DateTime?> GetLastCollectedTimeForDatabaseAsync( | ||
| int serverId, string tableName, string columnName, string databaseColumnName, string databaseName, | ||
| CancellationToken cancellationToken, DateTime? collectedSince = null) | ||
| { | ||
| try | ||
| { | ||
| await using var connection = await _postgres.OpenConnectionAsync(cancellationToken); | ||
| var sql = collectedSince is null | ||
| ? $"SELECT MAX({columnName}) FROM {tableName} WHERE server_id = $1 AND {databaseColumnName} = $2" | ||
| : $"SELECT MAX({columnName}) FROM {tableName} WHERE server_id = $1 AND {databaseColumnName} = $2 AND collection_time > $3"; | ||
| using var command = new NpgsqlCommand(sql, connection); | ||
| command.Parameters.AddWithValue(serverId); | ||
| command.Parameters.AddWithValue(databaseName); | ||
| if (collectedSince is DateTime floor) | ||
| { | ||
| /* Naive like every other timestamp bound in this store (#1969): a Utc Kind infers | ||
| timestamptz and Postgres would convert it into the session zone on the way in. */ | ||
| command.Parameters.AddWithValue(DateTime.SpecifyKind(floor, DateTimeKind.Unspecified)); | ||
| } | ||
|
|
||
| var result = await command.ExecuteScalarAsync(cancellationToken); | ||
| if (result is DateTime dt) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removes the
[#2266]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2266link-reference definition, but[#2266]is still referenced three times elsewhere in this file (lines ~30, ~38, ~46 in the new version, in the entries for the scale-test reporting fix, the skipped-maintenance-cycle log line, and the heal-test flakiness fix). With the definition gone, those three occurrences will render as literal[#2266]text instead of links to the issue. Looks like an unintended deletion — please restore the line (or drop it back in alongside the[#2344]addition) rather than removing it.