sp_QuickieStore: added the 'log' and 'total log' sort orders - #849
Conversation
erikdarlingdata
left a comment
There was a problem hiding this comment.
Thanks for this! Coverage is complete across all nine sort-order CASE sites, docs are updated in all three places, and the README Examples link fix is a nice drive-by. The avg log/average log prefix forms also work automatically, and there's no collision with @wait_filter = 'log' — I checked.
Two real problems need fixing before merge, and both are invisible to CI:
-
Regression mode reports the log change 8,192x too large, on the default path. The
@regression_comparator = 'absolute'display conversion (andabsoluteis the default) uses the 8KB-pages-to-MB formula forlog/total log, butavg_log_bytes_usedis in bytes, not pages. Details inline. -
SQL Server 2016 gets a hard error instead of the graceful fallback.
avg_log_bytes_usedonly exists insys.query_store_runtime_statson 2017+/Azure (@new = 1).tempdbis protected twice — the up-front version check that reverts to cpu with a warning, and inlineCASE WHEN @new = 1fallbacks —loghas neither. CI can't catch this because SQL Server on Linux starts at 2017. Details inline.
One style nit inline as well (missing N prefixes).
Non-blocking FYI: the @find_parameter_sensitive rank-metric whitelist (~line 7244) doesn't include 'log', so PS mode silently ranks by cpu. That's the documented fallback ("anything else = cpu"), fine to leave, but easy to add if you want it.
On your "I still cannot shake the feeling that we used to have this feature" — git log -S "'total log'" comes up empty for the file's whole history. The columns have been in the output since early on; the sort orders never existed. You were half-remembering the columns.
7e9fabd to
671bd7b
Compare
|
Addressed comments. Will re-test when I get a chance, but these small changes shouldn't break anything. |
erikdarlingdata
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — the 2016 protection is now done properly and thoroughly: the up-front revert covers log/total log (and pulling total tempdb into that same gate is a nice consistency drive-by — it previously relied on the inline fallbacks alone, with no warning), and every inline arm carries the CASE WHEN @new = 1 fallback mirroring tempdb. The N-prefixes landed too.
Two things still block, and the second is the same one as last round:
1. Syntax error — the proc won't create. In the ORDER BY expression block, the 'total log' arm is missing its THEN:
WHEN 'total log' THEN CASE WHEN @new = 1 N'qsrs.avg_log_bytes_used * qsrs.count_executions' ELSE ...
^^ missing THENCompare the 'total tempdb' line directly below it. CI will catch this one when it runs (the proc fails to CREATE), but naming it saves you the round trip.
2. The absolute-regression conversion is still wrong — now 8× instead of 8,192×. Both 'log' and 'total log' arms now read:
N'(hashes_with_changes.change_since_regression_time_period * 8.) / 1048576.'You corrected the divisor (KB→bytes) but kept the * 8. — that multiplier converts 8 KB pages to KB, and avg_log_bytes_used is already in bytes. Bytes → MB is simply:
N'hashes_with_changes.change_since_regression_time_period / 1048576.'With the current expression, @regression_comparator = 'absolute' (the default) reports every log change 8× too large. CI cannot catch this one — it runs on 2017+ where the query executes happily and produces confidently wrong numbers, which is why I'd ask for one actual run of regression mode with @sort_order = 'log' against a known workload before the next push, rather than "shouldn't break anything" — that instinct is exactly what both of these slipped past.
Everything else from last round is resolved. Fix the THEN and drop the * 8. and this merges.
671bd7b to
e95a753
Compare
|
Addressed comments. Will re-test when I get a chance. |
|
Re-tested. I think this all works. |
Added the 'log' and 'total log' sort orders to
sp_QuickieStore. Closes #819.sp_QuickieStorehas a lot of features now, so I don't know if I've tested everywhere that I need to. Hopefully the screenshots are convincing enough? I've never taken the time to understand the new Pareto stuff but my impression is that you need to opt sort orders into that before you can break anything.I still cannot shake the feeling that we used to have this feature.
Normal usage:
Regression mode: