[erikdarlingdata]

Claude posting for Erik Darling.

Closes #907

sp_IndexCleanup now judges unused indexes on each database's own usage window instead of the server uptime alone. All four items under "Additional Context" in the issue are done as written.

What changed (sp_IndexCleanup/sp_IndexCleanup.sql)

1. Two new columns, usage_since and usage_since_reason, on the DATABASE row of the summary. usage_since is the latest of the server start, the database's last restore (MAX(restore_date) in msdb.dbo.restorehistory for that destination_database_name) and sys.databases.create_date. The reason is Server start, Database restore or Database create. The ANALYZED OBJECT DETAILS and TABLE rows output NULL for both. They sit at the end of the visible columns, so existing column positions do not move.
2. The Unused Index (WARNING: ...) label follows the database window (< 14 days). When the window is the server start the label is byte-identical to today. When a restore or create set the window, it reads Unused Index (WARNING: Database usage window < 14 days - usage data may be incomplete).
3. The skip is per database. With @dedupe_only = 0, a database whose window is 7 days or less skips Rule 1, and its DATABASE row says: Database restore. Unused Index rule skipped for this database: only N days of usage data (7 days or less). Other databases in the run are judged normally. A server up 7 days or less still trips the existing global switch and message, and an explicit @dedupe_only = 1 still skips Rule 1 everywhere. Those two cases add no per-database note.
4. AG primary with a readable secondary: the DATABASE row's reason gets . Availability group primary with readable secondaries: reads on the secondaries are not counted. Detection follows the existing AG pattern (behind @is_hadr_enabled, dynamic SQL, TRY/CATCH). A failure adds no note.

The server uptime stays in the header row and the ANALYZED OBJECT DETAILS row. The per-day rate columns on TABLE rows now divide by the database window (see the last section).

Engine gates: the restore lookup is skipped on Azure SQL Database (no msdb.dbo.restorehistory) and runs as dynamic SQL in TRY/CATCH, so a login that cannot read it falls back to create_date without failing the run.

Measured

• Does RESTORE change create_date? It depends on the form. RESTORE under a new name sets the new database's create_date to the restore moment (73 ms after the restorehistory row). RESTORE ... WITH REPLACE over an existing database keeps the original create_date, so only restorehistory shows that case (the one the issue describes). Because of the first form, a create_date within 60 seconds of the winning time is credited to the restore when there is one, so a new-name restore reads Database restore, not Database create.
• Low-privilege login: a scratch login (db_owner in a scratch database, VIEW SERVER STATE) can run the proc. With DENY SELECT ON msdb.dbo.restorehistory the debug output shows Restore history lookup failed: The SELECT permission was denied ..., the run completes, and the DATABASE row reads Database create. Login, user, database and history rows were dropped.
• Item 4 (AG): not run. No lab server has an AG. I ran only the dynamic SQL against SQL2022 with a stand-in #databases to check that it parses and executes (0 rows); the primary and readable-secondary logic is code-read only.

Tests (SQL2022)

Harness	Result
run_tests.py	58 passed
fixture_cases_test.py	30 passed
rule_coverage_test.py	134 passed (was 111)
no_access_test.py	4 passed

The proc also installs cleanly on SQL2016 (syntax check only).

New Group A-Window in rule_coverage_test.py (22 assertions, runs on any uptime): it installs a scratch copy of the proc as sp_IndexCleanup_zz_window (precedent: 3a69179) with the server start pinned 12 days back and each scratch database's create_date moved back. It then builds five ic_win_* databases, backs one up to the default backup directory, and does a real RESTORE under a new name and a RESTORE WITH REPLACE over an existing one. It asserts, per database: the reason (server start, create, restore, restore), the skip note, no Unused rows for the restored, replaced and fresh databases, the original label for the server-start window, the new label for a 10-day database, an Unused row still present for the old database in the same run, NULL usage_since outside the DATABASE rows, the server uptime still in the header row, and @dedupe_only = 1 giving no per-database note. Every scratch database, history row, backup file and the scratch proc are dropped afterwards (checked on SQL2022).

Group A's existing branch now keys on Crap's own usage window, not only server uptime: a long-up server with a newly created Crap skips Rule 1 for it, so the guard assertions apply. On the lab nothing changes. tests/README.md counts are now 58/30/134/4 (131 for rule_coverage_test.py on a server up more than 7 days) and it has a short paragraph on the new group.

Not run: CI containers (Linux). The harness uses the default backup directory with either path separator and wraps the backup file delete in TRY/CATCH.

Follow-up 2b39213: SERVERPROPERTY('InstanceDefaultBackupPath') is NULL before SQL Server 2019, so the backup path was NULL on SQL Server 2017. The harness now falls back to InstanceDefaultDataPath and drops its trailing separator. rule_coverage_test.py on SQL2017: 133 passed, no ic_win_* databases left.

For the README at release

Lines ~93-94 of sp_IndexCleanup/README.md describe the uptime guard. Suggested replacement text:

Index usage statistics start over when SQL Server restarts, and also when a database is restored or created. The summary's DATABASE row shows usage_since (the latest of the server start, the database's last restore in msdb.dbo.restorehistory, and its create_date) and usage_since_reason (which of those it was). If a database's usage window is 7 days or less, Rule 1 (unused indexes) is skipped for that database only, and its DATABASE row says so. A server started 7 days ago or less still switches to @dedupe_only for every database. Unused Index rows carry a warning while the window is under 14 days. On an availability group primary with readable secondaries, the DATABASE row notes that reads on the secondaries are not counted.

Per-day rates use the database window (9a2ec08)

daily_write_ops_saved, daily_lock_waits_saved and daily_latch_waits_saved on TABLE rows now divide by the table's database usage window (#databases.usage_window_days), not server uptime. Usage and operational counters all restart when a database is restored or created, so all three use the same window. The TABLE branch LEFT JOINs #databases by name (its primary key), with @server_uptime_days as the fallback and the existing NULLIF(..., 0) kept. When the window is the server start it is the same DATEDIFF as uptime, so the result is unchanged. SUMMARY and DATABASE rows still show N/A, and the header text still uses server uptime.

Test: Group A-Window now gives the fixture 120 updates touching both indexes and asserts that ic_win_mid (created 10 days ago, server start pinned 12) shows the 10-day answer (12, where a 12-day divisor gives 10).

Results at 9a2ec08: SQL2022 run_tests.py 58, fixture_cases_test.py 30, rule_coverage_test.py 134, no_access_test.py 4, all passed. SQL2017 rule_coverage_test.py 134 passed. No ic_win_* databases left on either server. tests/README.md counts updated (134, and 131 on a server up more than 7 days). Only rule_coverage_test.py gained an assertion, so the other three counts are unchanged.

Considered, not added

• The restore lookup does not filter by restore_type: any restore into the name (full, differential, log) counts, since each one brings the database online and resets its usage stats.
• A name collision across collations between msdb and the user database is not handled (names differing only by case on a case-sensitive msdb). No concrete trigger.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UT4xePKcEVUmyzwGYSEuqU

[claude]

Claude finished @erikdarlingdata's task in 32s —— View job

---

Review of PR #925

I found no blocking problems. I read the diff but did not run any of it, including the tests.

• Scope: only sp_IndexCleanup.sql and the test files changed. Install-All/DarlingData.sql is untouched, and the @version lines are not bumped.
• Self-containment: there are no new objects. The new #database_restores and #ag_primary_readable_databases are temp tables inside the proc.
• Version and engine gating: msdb.dbo.restorehistory is read through dynamic SQL in TRY/CATCH and skipped on Azure SQL DB. The AG lookup sits behind @is_hadr_enabled, also in dynamic SQL with TRY/CATCH. A failure in either one falls back quietly, as the PR describes.
• Window logic: usage_since is the greatest of the restore date, create_date and the server start. A NULL restore date drops out of the VALUES max, and the NULL >= ... comparison falls through to the next WHEN. That gives the intended "Database create" or "Server start" reason. The 60-second credit for a new-name restore is reasonable.
• Per-database skip: it applies only when @dedupe_only = 0, so the global switch and an explicit @dedupe_only = 1 don't get a duplicate per-database note. For a window set by the server start, the label text matches the old one.
• Column placement: the new columns go at the end and are NULL on the ANALYZED OBJECT DETAILS and TABLE rows, so existing column positions don't move.

Minor, optional notes:

1. DATEDIFF(DAY, ...) counts midnight boundaries, so a "7 day" window can be a little under 7 × 24 hours. This is probably consistent with how @uptime_days is computed, so it is probably fine.
2. The restore lookup uses COLLATE DATABASE_DEFAULT, but the AG lookup compares db.database_name = d.name without it. Both sides are normally the server default collation. If they differed, the failure would be swallowed and no note would be added. You already list the collation point under "Considered, not added".
3. The skip/label branch checks usage_since_reason LIKE N'Server start%'. This works, but it ties logic to display text. A small flag column would be sturdier if the reason strings ever change.

The AG path (item 4) is code-read only, as the PR says. The rest looks good to me.
· branch fix/907-usage-window

[claude]

Claude finished @erikdarlingdata's task in 26s —— View job

---

Review of PR #925

The change looks sound. I read the sp_IndexCleanup.sql diff only. I did not run the tests or look at the test harness changes.

What checks out

• Self-contained: no new objects. Only temp tables were added.
• Version and engine gating:
  • The msdb.dbo.restorehistory lookup is skipped on Azure SQL DB.
  • Both new lookups use dynamic SQL inside TRY/CATCH, so a permissions failure or missing object falls back quietly. The error shows only under @debug.
  • sys.fn_hadr_is_primary_replica and secondary_role_allow_connections_desc are available on 2016+ and sit behind @is_hadr_enabled.
• Window logic: the latest of server start, restore and create is a sensible choice. The 60-second credit for a new-name restore matches what you measured. Because the second UPDATE only appends text, the Server start reason is unchanged when no per-database note applies. The LIKE N'Server start%' test therefore still matches, including when the AG note is appended.
• No version bumps. Install-All/DarlingData.sql is untouched.
• Style: consistent with CLAUDE.md. I found no -- comments, no abbreviated types, and aliases are present.

Minor points, none blocking

1. Day count: DATEDIFF(DAY, ...) counts midnight boundaries, not elapsed time. A database restored at 23:59 yesterday shows 1 day, and one restored 7 days and 1 minute ago can show 8. This is probably fine for a "7 days or less" heuristic, and the existing uptime logic may use the same approach. A note in a comment would help.
2. Stale variables: in the Rule 1 SELECT ... FROM #databases WHERE database_id = @current_database_id, a missed row would leave @usage_window_days, @usage_since_server_start and @skip_unused_rule holding the previous database's values. The ISNULL defaults only cover NULL columns. In practice the cursor iterates #databases, so this should not happen. If you want it airtight, reset the three variables just before the SELECT.
3. Per-day rates: these still divide by server uptime, as you already noted. A restored database will show understated per-day numbers next to its usage_since column. That is acceptable for this scope, and a follow-up issue might be worth opening.
4. Collation: the restore lookup uses COLLATE DATABASE_DEFAULT on both sides, but the later JOIN #database_restores AS dr ON dr.database_name = db.database_name compares two temp-table columns, so it needs no collation hint. Your "considered, not added" note on case-only name differences stands.

The README replacement text in the PR description is clear. Apply it at release time.
| Branch: fix/907-usage-window

[claude]

Claude finished @erikdarlingdata's task in 16s —— View job

---

Review of PR #925 (re-run on synchronize)

No new blocking problems. The merge of dev into the branch was the only new commit. I did not re-read the full diff or run any tests this time. I only scanned the added lines of sp_IndexCleanup.sql, and the grep found no -- comments, abbreviated types or EXEC. It did match integer, since my pattern for the abbreviated INT ignored case.

My earlier findings still stand:

• Scope: only the proc and its tests changed. Install-All/DarlingData.sql is untouched and the @version lines are not bumped.
• Self-containment: the new objects are temp tables inside the proc. There are no new database objects.
• Gating: the restore lookup and the AG lookup both use dynamic SQL in TRY/CATCH. The restore lookup is skipped on Azure SQL DB, and the AG lookup sits behind @is_hadr_enabled.

Optional points from the earlier reviews, none blocking:

1. DATEDIFF(DAY, ...) counts midnight boundaries, so the 7 and 14 day thresholds are approximate.
2. If the Rule 1 SELECT against #databases ever returned no row, @usage_window_days, @usage_since_server_start and @skip_unused_rule would keep the previous database's values. Resetting them just before the SELECT would rule that out.
3. The logic tests usage_since_reason LIKE N'Server start%', which ties it to display text. A flag column would be sturdier.
4. The AG path (item 4) is still code-read only.

If you wanted something specific from this @claude mention, tell me what it is.
| Branch: fix/907-usage-window

[claude]

Claude finished @erikdarlingdata's task in 16s —— View job

---

Review of PR #925 (re-run on synchronize)

No blocking problems. The new commit is 9a2ec08, which makes the TABLE per-day rates divide by the database usage window. I read that diff. I did not run any tests.

• Correctness:
  • daily_write_ops_saved, daily_lock_waits_saved and daily_latch_waits_saved now use ISNULL(d.usage_window_days, @server_uptime_days). The NULLIF(..., 0) guard is kept.
  • The LEFT JOIN #databases AS d ON d.database_name = irs.database_name won't multiply rows if the commit message is right that database_name is the primary key of #databases. I didn't re-check the key.
  • Where the window is the server start, the result matches the old output. That's the commit's claim, and I didn't verify it.
  • This addresses the "Considered, not added" item in the PR description about per-day rates, so that item is now out of date. Update the description.
• Scope, self-containment, style: there are no new database objects. Install-All/DarlingData.sql is untouched and the @version lines are not bumped. The new comment is a /* */ block.

Optional points from the earlier reviews still apply:

1. DATEDIFF(DAY, ...) counts midnight boundaries, so the 7 and 14 day thresholds are approximate.
2. Reset @usage_window_days, @usage_since_server_start and @skip_unused_rule just before the Rule 1 SELECT. That rules out stale values from the previous database if the row is ever missing.
3. The logic tests usage_since_reason LIKE N'Server start%'. A flag column would be sturdier than matching display text.
4. The AG path (item 4) is still code-read only.
   | Branch: fix/907-usage-window