-
Notifications
You must be signed in to change notification settings - Fork 92
Index Analysis connects to the database it analyses on Azure #2409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -960,11 +960,51 @@ private async void RunIndexAnalysis_Click(object sender, RoutedEventArgs e) | |
|
|
||
| try | ||
| { | ||
| var utilityConnectionString = _credentialResolver.GetUtilityConnectionString(server); | ||
| var databaseNameEarly = IndexAnalysisDatabaseInput.Text?.Trim(); | ||
| var allDatabasesEarly = IndexAnalysisAllDatabases.IsChecked == true; | ||
|
|
||
| /* #2407: Azure SQL Database has no cross-database execution, so the Utility DB idea — install | ||
| sp_IndexCleanup once and point it at any database on the server — cannot work there. The proc | ||
| runs INSIDE whichever database the connection opened, and @database_name asks it to read | ||
| another one, which Azure refuses. Reported as "set Utility DB to db1, analysing db1 works, | ||
| analysing db2 says no valid database" — the proc's own message, which reads like the database | ||
| is missing rather than unreachable. | ||
|
|
||
| So on Azure the connection targets the database being ANALYSED, not the utility database: the | ||
| proc has to be installed in each database anyway (which is what the reporter found by | ||
| experiment), and pointing at the target is the only shape that can work. */ | ||
| var properties = _dataService == null | ||
| ? null | ||
| : await _dataService.GetLatestServerPropertiesAsync(GetSelectedServerId()); | ||
| var isAzureSqlDb = properties?.EngineEdition == 5; | ||
|
Comment on lines
+977
to
+979
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Azure detection here reads
Consider using |
||
|
|
||
| if (isAzureSqlDb && allDatabasesEarly) | ||
| { | ||
| /* Enumerating every database from one connection is the same cross-database read, so All | ||
| Databases cannot work on Azure either — and failing per-database would half-fill the grid | ||
| with whichever database the connection happened to open. */ | ||
| IndexAnalysisStatusText.Text = | ||
| "Azure SQL Database cannot analyse across databases — clear \u201CAll Databases\u201D and name one, " | ||
| + "with sp_IndexCleanup installed in it."; | ||
| return; | ||
| } | ||
|
Comment on lines
+981
to
+990
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this early-return path only sets |
||
|
|
||
| var utilityConnectionString = isAzureSqlDb && !string.IsNullOrWhiteSpace(databaseNameEarly) | ||
| ? _credentialResolver.GetConnectionStringForDatabase(server, databaseNameEarly!) | ||
| : _credentialResolver.GetUtilityConnectionString(server); | ||
|
Comment on lines
+992
to
+994
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When Azure is correctly detected but the database field is left blank and "All Databases" is unchecked (a reachable, unvalidated combination — nothing requires filling in a name or checking the box), this falls through to That contradicts the updated tooltip in |
||
|
|
||
| var exists = await LocalDataService.CheckSpIndexCleanupExistsAsync(utilityConnectionString); | ||
| if (!exists) | ||
| { | ||
| /* On Azure the proc must live in the target database, so name it — "not installed" against a | ||
| server with 50 databases is not actionable without saying which one was checked. */ | ||
| if (isAzureSqlDb && !string.IsNullOrWhiteSpace(databaseNameEarly)) | ||
| { | ||
| IndexAnalysisStatusText.Text = | ||
| $"sp_IndexCleanup is not installed in [{databaseNameEarly}]. Azure SQL Database cannot run it " | ||
| + "from another database, so it must be installed in each database you analyse."; | ||
| } | ||
|
|
||
| IndexAnalysisNotInstalledMessage.Visibility = Visibility.Visible; | ||
| IndexAnalysisNoDataMessage.Visibility = Visibility.Collapsed; | ||
| _indexSummaryFilterMgr!.UpdateData(new List<IndexCleanupSummaryRow>()); | ||
|
|
@@ -977,8 +1017,8 @@ private async void RunIndexAnalysis_Click(object sender, RoutedEventArgs e) | |
| RunIndexAnalysisButton.IsEnabled = false; | ||
| IndexAnalysisStatusText.Text = "Running analysis..."; | ||
|
|
||
| var databaseName = IndexAnalysisDatabaseInput.Text?.Trim(); | ||
| var getAllDatabases = IndexAnalysisAllDatabases.IsChecked == true; | ||
| var databaseName = databaseNameEarly; | ||
| var getAllDatabases = allDatabasesEarly; | ||
|
|
||
| var (details, summaries) = await LocalDataService.RunIndexAnalysisAsync( | ||
| utilityConnectionString, | ||
|
|
||
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.
Azure detection can silently fail open, reproducing the exact bug this PR fixes.
isAzureSqlDbis derived from_dataService.GetLatestServerPropertiesAsync(...), which reads theserver_propertiestable. That collector is registered withFrequencyMinutes = 0("on-load only",Lite/Services/ScheduleManager.cs) and runs ~29th of ~35 collectors, strictly sequentially, insideRunAllCollectorsForServerAsync— whichMainWindow.xaml.csConnectToServerkicks off after it has already added the server tab toServerTabControl.Itemsand made it the selected item.RunIndexAnalysisButtonhas no gating on initial load completing (it's only disabled while an analysis is in flight), so a user can open the FinOps tab and click "Run Analysis" beforeserver_propertieshas ever been collected for that server — e.g. right afterAddServer(), which doesn't trigger any collection at all.In that window
propertiesisnull, soisAzureSqlDbisfalse, and the code takes the pre-fix branch:_credentialResolver.GetUtilityConnectionString(server). On Azure SQL Database with a Utility DB configured, that's precisely the "Utility DB set to db1, analysing db2 reports no valid database" bug from #2407 — it just resurfaces on a timing window instead of unconditionally._serverManager.GetConnectionStatus(server.Id).SqlEngineEdition == 5(already used the same way elsewhere, e.g.MainWindow.AlertEngine.cs:84/230,RemoteCollectorService.cs:634) is populated synchronously by the permission-free connectivity-check query and is available far earlier/more reliably —_serverManageris already a field on this class and is even null-checked at the top of this same method. That looks like the more robust signal to key this fix off of.