Dev#792
Conversation
Update sp_IndexCleanup.sql
|
Thanks @mmcdonald-1 — appreciate the contribution. I dug into this with the procedure context and ran a repro on SQL Server 2022. There's one correctness issue that blocks merging as-is, plus a batch of style nits. Blocker: index-level totals are duplicated across per-partition rows
Repro (SQL Server 2022, 4-partition table, all partitions PAGE-compressed)Direct from What the PR's INSERT produces:
Fix options
Style nitsThe repo has a T-SQL style guide at CLAUDE.md — worth a skim. The new code has a handful of departures from it:
Heads-up: there's some style drift elsewhere in the file too, and I plan to do a cleanup pass eventually, so don't sweat it if a couple of these slip through — but the indentation, data-type casing, and Non-blocking
@mmcdonald-1 would you like to take a crack at the correctness fix and style cleanup, or would you rather I take it from here? Happy either way — just let me know. |
|
I'll take a crack at it. I don't have anything with multiple partitions
and didn't even think about it but it makes complete sense. Your style is
different from mine and I thought I had caught all the
differences...obviously not. SQL Prompt by Redgate capitalizes all data
types by default and I don't even think about it. I'll remember to turn it
off (it's got other things as well).
Cheers
…On Tue, May 19, 2026 at 8:04 AM Erik Darling ***@***.***> wrote:
*erikdarlingdata* left a comment (erikdarlingdata/DarlingData#792)
<#792 (comment)>
Thanks @mmcdonald-1 <https://github.com/mmcdonald-1> — appreciate the
contribution. I dug into this with the procedure context and ran a repro on
SQL Server 2022. There's one correctness issue that blocks merging as-is,
plus a batch of style nits.
Blocker: index-level totals are duplicated across per-partition rows
#operational_stats is populated by a GROUP BY at the index level (no
partition_number), so the page_compression_attempt_count /
page_compression_success_count columns it holds are *already summed
across all partitions of an index*. The new INSERT joins per-partition
#partition_stats against that index-level table on index keys only, so
every PAGE-compressed partition row of an index ends up carrying the same
index-wide totals, and the "success rate" is an index-wide weighted average
— not per-partition, despite the column name.
Repro (SQL Server 2022, 4-partition table, all partitions PAGE-compressed)
Direct from sys.dm_db_index_operational_stats (the truth, per partition):
partition attempts successes
1 518 65
2 519 64
3 519 65
4 516 61
What the PR's INSERT produces:
partition attempts successes rate
1 2072 255 12.31
2 2072 255 12.31
3 2072 255 12.31
4 2072 255 12.31
2072 = 518 + 519 + 519 + 516 and 255 = 65 + 64 + 65 + 61 — same
index-level totals repeated on each PAGE partition row.
Fix options
- *Easiest:* aggregate at the index level for reporting. One row per
index, plus a "PAGE partitions / total partitions" column so a
partially-PAGE-compressed index is obvious.
- *Most faithful to the issue:* carry partition-level compression
counts in a dedicated temp table by re-querying
sys.dm_db_index_operational_stats without the index-level SUM / GROUP
BY. Then the per-partition output is real.
Style nits
The repo has a T-SQL style guide at CLAUDE.md
<https://github.com/erikdarlingdata/DarlingData/blob/main/CLAUDE.md> —
worth a skim. The new code has a handful of departures from it:
- *Tabs in the new INSERT/SELECT block* — guide is 4-space
indentation, no tabs.
- *Uppercase / abbreviated data types* (BIGINT, INT, NUMERIC(6,2)) —
should be lowercase and unabbreviated (bigint, integer, numeric(6,2)).
- *Leading commas* in the SELECT list — guide is trailing commas.
- *Bracket-quoted regular column names* ([database_name], [object_id])
— inconsistent with the surrounding file.
- *FROM #success_rate sr* — missing AS on the alias.
- *INNER JOIN* — the rest of the file uses plain JOIN.
- *ORDER BY success_rate_pct* — unqualified; should be
sr.success_rate_pct.
- *SELECT ... AS success_rate* in the ELSE branch — guide uses alias =
expression rather than expression AS alias.
- *Missing WITH (TABLOCK)* on the new INSERT into a temp table.
- *Missing semicolons* after CREATE TABLE #success_rate, the
OPTION(RECOMPILE) block, and the END blocks.
- *IF EXISTS … BEGIN* indentation — BEGIN should align with the IF.
Heads-up: there's some style drift elsewhere in the file too, and I plan
to do a cleanup pass eventually, so don't sweat it if a couple of these
slip through — but the indentation, data-type casing, and WITH (TABLOCK)
are the ones I'd want to see addressed in this PR.
Non-blocking
- The empty-result placeholder (SELECT 'NO INDEXES WITH PAGE
COMPRESSION FOUND' AS success_rate) returns a different schema than
the populated case, which is awkward for any caller keying off column
names. Either match the columns (NULLed out) or drop the result set
entirely when empty.
- success_rate = 'SuccessRate' as a literal header column is a new
pattern for this proc — consider naming it result_type for consistency
with the other emitted result sets.
- New result set isn't wired into @debug messaging (RAISERROR(...,
'Generating ...', 0, 0) WITH NOWAIT;) or @help.
------------------------------
@mmcdonald-1 <https://github.com/mmcdonald-1> would you like to take a
crack at the correctness fix and style cleanup, or would you rather I take
it from here? Happy either way — just let me know.
—
Reply to this email directly, view it on GitHub
<#792 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANLUHMFGQ3OACMVGIFUVLOT43RETJAVCNFSM6AAAAACZED47BGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DIOBXGUZDEOBXGA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Pull request [FEATURE] #781