Repository navigation
Add Find-DbaDbMissingIndex - #10779
deepeshd87 wants to merge 6 commits into
Conversation
andreasjordan
left a comment
There was a problem hiding this comment.
Thanks for picking this up so quickly, @deepeshd87. The command covers nearly every point from the discussion in #10770, and the AND 1 = (SELECT 1) fixture is exactly what makes the test deterministic. A few things need to change before this can go in.
Must fix
1. CreateStatement is invalid T-SQL for the most common kind of suggestion.
.Query() returns a NULL column as [DBNull]::Value, and PowerShell treats that as $true. So if ($row.InequalityColumns) never skips an empty column. With the test fixture (equality [CustomerId], no inequality columns, include [Amount]), the command returns:
CREATE NONCLUSTERED INDEX [IX_Orders_CustomerId] ON [dbo].[Orders] ([CustomerId], ) INCLUDE ([Amount]);Without included columns, the statement ends in INCLUDE ();. I checked this on Windows PowerShell 5.1 and on PowerShell 7. Testing all three columns with -isnot [System.DBNull] fixes it. The same bug also breaks overlap detection for suggestions that have only inequality columns, because the key text then starts with a comma.
2. Tests. The test file looks like it was based on a very old template. Please use a current test file as the template, for example tests/Find-DbaDbQueryStoreRegression.Tests.ps1 from your last PR. Please also assert the complete CreateStatement of the fixture, not only that it contains CREATE NONCLUSTERED INDEX. That would have caught point 1.
3. The caller's connection is left in the last database.
$db.Query() and the enumeration of $db.Tables move the server's shared connection into that database and never move it back. When a caller passes a server object or pipes in Get-DbaDatabase output, their next unqualified query runs in the wrong database. Please use the pattern from Find-DbaDbDisabledIndex (see #10555): read $server.ConnectionContext.CurrentDatabase before the work starts, and call Restore-DatabaseContext in a finally block.
Wrong or misleading output
4. Overlap detection misses overlaps. It compares the leading key column of each existing index only with the first column listed in equality_columns. That list is not in key order, so a suggestion on [A], [B] is not flagged against an existing index on (B, A). Please compare the leading key column with every equality column, or with the first inequality column when there are no equality columns. If you do this in the main query by joining sys.indexes and sys.index_columns (key_ordinal = 1), the per-table SMO calls go away too. Right now the command loads the whole Tables collection, plus the indexes and columns of every table with a suggestion.
5. Duplicate rows on SQL Server 2019 and later. The qt CTE (common table expression) picks one query per row with TOP (1) ... ORDER BY last_user_seek DESC, which has no tiebreaker, and then groups the result. When two queries of one group tie on last_user_seek (for example, both are NULL because the queries only scanned), the group can come back as two rows. The suggestion is then returned twice, with different QueryHash values. ROW_NUMBER() OVER (PARTITION BY group_handle ORDER BY last_user_seek DESC, query_hash) with rn = 1 is deterministic and needs only one pass.
6. Suggested index names collide. The name is built from the table and the key columns only. The DMV (dynamic management view) often has several suggestions for one table with the same keys and different INCLUDE lists, and they all get the same name. Running the statements one after another then fails from the second one on. Adding index_handle to the name would keep it unique.
7. Names are not quoted safely. Schema, table and index names are wrapped in brackets without escaping ], so a table named Order]Lines produces broken T-SQL. QUOTENAME() in the query, or .Replace("]", "]]"), fixes that. Also, OBJECT_NAME() and OBJECT_SCHEMA_NAME() return NULL when the login can't see the object's metadata, which currently produces ON [].[].
8. The 600-group check runs once per database. The comment says once per instance, but the check sits inside the database loop. That costs one extra query per database and, at the limit, writes the same warning once for every database. Remembering which instances were already checked fixes both.
Smaller points
9. The default -MinimumSeek 1 filters on user_seeks alone, but ImpactScore counts seeks plus scans. So suggestions that come only from scans are never returned by default. Filtering on user_seeks + user_scans would match the score better; otherwise please say in the help that these suggestions are left out.
10. System databases are always excluded, without a message. -Database msdb (msdb often has real suggestions), or piping in Get-DbaDatabase -Database msdb, returns nothing, and the user reads that as "no suggestions". Please either include a system database when it is named explicitly, or at least write a warning.
Could you also fix the PR title and add Closes #10770 to the description?
This text was created by Claude and reviewed by Andreas Jordan.
potatoqualitee
left a comment
There was a problem hiding this comment.
Reviewed at 043fbb6. Requesting changes for four material issues detailed inline:
- Normal manifest imports do not export the new command.
- SQL NULL values produce malformed CREATE INDEX statements for common suggestions.
- Fresh SMO table/index enumeration can leave a caller-owned connection in the wrong database.
- The new integration fixture references an instance configuration that the test harness does not define, and the exact-parameter test reads a discovery-only variable at runtime.
I checked the complete patch, surrounding module exports, query wrappers and cleanup helpers, test configuration, existing discussions, and current CI. The DBNull behavior was independently traced through the PowerShell DataRow adapter and Boolean conversion implementation. The connection-context finding specifically concerns SMO collection enumeration: Database.Query() already restores context at this head.
There are no executed check results for this head; the PR-triggered workflows await approval. I did not rerun PowerShell/SQL Server tests locally. The review is based on the concrete source-backed failure paths below.
| @@ -0,0 +1,332 @@ | |||
| function Find-DbaDbMissingIndex { | |||
There was a problem hiding this comment.
[P1] Export the new public command through the normal module entry point
This head adds only the function and its test. Find-DbaDbMissingIndex is absent from dbatools.psd1's explicit FunctionsToExport list and the command lists in dbatools.psm1. In a fresh session, importing this checkout through dbatools.psd1 therefore does not expose the function, so Get-Command Find-DbaDbMissingIndex fails. Importing dbatools.psm1 directly can mask this problem during development.
Please add the command to FunctionsToExport and the appropriate psm1 platform command list, then verify that it is available after a normal manifest import, including the supported legacy export path.
| if ($row.EqualityColumns) { $keyCols += $row.EqualityColumns } | ||
| if ($row.InequalityColumns) { $keyCols += $row.InequalityColumns } | ||
| $keyColText = ($keyCols -join ', ') |
There was a problem hiding this comment.
[P1] Exclude DBNull before building the key and INCLUDE clauses
Database.Query() returns DataTable rows without normalizing SQL NULLs, and PowerShell considers DBNull.Value true. Both of these conditions therefore append an empty element when the corresponding DMV field is SQL NULL. With the fixture's equality [CustomerId], NULL inequality and include [Amount], the generated statement is:
CREATE NONCLUSTERED INDEX [IX_Orders_CustomerId] ON [dbo].[Orders] ([CustomerId], ) INCLUDE ([Amount]);The same truth test at line 274 produces INCLUDE () when included_columns is NULL. Inequality-only suggestions also begin with an empty key, breaking their first-key overlap lookup. These are routine DMV results, so the advertised generated SQL is unusable for common suggestions.
Please normalize or explicitly exclude DBNull in all three column lists before building SQL. Add regression assertions for complete valid statements with equality-only keys, inequality-only keys and no INCLUDE columns; the existing substring assertion accepts the malformed statement.
| $firstKey = (($keyColText -split ',')[0] -replace '[\[\]]', '').Trim() | ||
| if (-not $existingIndexCache.ContainsKey($row.ObjectId)) { | ||
| try { | ||
| $smoTable = $db.Tables | Where-Object { $_.ID -eq $row.ObjectId } |
There was a problem hiding this comment.
[P2] Restore the caller's database after fresh SMO collection enumeration
On a caller-owned non-pooled connection, populating a fresh database-level SMO collection moves the shared connection into that database. This path reads $db.Tables and later $smoTable.Indexes/IndexedColumns without a surrounding context-restoration finally. A concrete failing path is a fresh server connected to master, a different user database with a qualifying suggestion, and Find-DbaDbMissingIndex called with that server or its database object. After overlap inspection, SELECT DB_NAME() on the original connection can return the inspected database, so subsequent unqualified SQL runs against the wrong database.
Database.Query() already restores its context in xml/dbatools.Types.ps1xml; the remaining leak is the collection enumeration here. Please capture each caller connection's CurrentDatabase before work can move it and invoke Restore-DatabaseContext in a finally, as the sibling index command does. Validate with a non-pooled connection and fresh collections, since pooling and SMO caching can hide the defect.
| Describe "Find-DbaDbMissingIndex" -Tag "IntegrationTests" { | ||
| BeforeAll { | ||
| $db = "dbatoolsci_missingindex_$(Get-Random)" | ||
| $server = Connect-DbaInstance -SqlInstance $TestConfig.instance2 |
There was a problem hiding this comment.
[P2] Connect the fixture to a test instance that the harness provides
Get-TestConfig does not define instance2. The setup here, cleanup at line 58, and command invocation at line 64 consequently receive a null target instead of the provisioned SQL instance. Scenario routing in tests/pester.groups.ps1 also recognizes InstanceSingle/InstanceMulti/etc., not this obsolete property, so this file does not select the intended single-instance lane. The new fixture cannot provide the claimed integration coverage under the repository's current CI configuration.
Use $TestConfig.InstanceSingle consistently and run the fixture in that scenario. Also repair the ordinary parameter-comparison test at line 29: $knownParameters is populated only in BeforeDiscovery, whose variables are not available in that runtime It. Define that expected list in a runtime setup/body or pass it as test data. These are execution failures, independent of test formatting.
|
Correction to point 3 of my review: This text was created by Claude and reviewed by Andreas Jordan. |
|
Thanks for the detailed review. The DBNull, context-restoration, and test-config/BeforeDiscovery issues were already addressed in a revision after the first submission - overlap detection now runs in the query rather than SMO, so the $db.Tables enumeration behind the context leak is gone, and Restore-DatabaseContext covers the $db.Query() path in a finally. I’ve now also added the command to FunctionsToExport in dbatools.psd1 and the psm1 command list (P1). Tests use $TestConfig.InstanceSingle and assert the full CREATE statement. |
Type of Change
Implements #10770.
Purpose
dbatools has Find-DbaDbDuplicateIndex, Find-DbaDbUnusedIndex, and
Find-DbaDbDisabledIndex, but no missing-index counterpart. This adds a read-only
Find-DbaDbMissingIndex that surfaces suggestions from the sys.dm_db_missing_index_*
DMVs, ranked by impact, with a generated CREATE INDEX statement for review.
Approach (per the design discussed in #10770)
three inputs are returned as their own properties so results can be re-ranked.
(not replacing it) for QueryHash / LastSqlHandle.
collection limit; help notes the counter reset conditions.
requirement, that suggestions are candidates for human review, and links
Invoke-DbaDiagnosticQuery.
Testing
Verified against a seeded fixture on SQL Server 2022: the DMV query, per-database
scoping, the ImpactScore and its inputs, the generated CREATE statement, the SMO
overlap detection, and the 2019+ query-text join (QueryHash/LastSqlHandle, no
double-counting) all return correctly. I wasn't able to run the full in-module Pester
suite locally, so I'm relying on CI for the end-to-end run.
(Second contribution — previously added Find-DbaDbQueryStoreRegression in 2.9.0.)