Fix installer dropping database on every upgrade#540
Merged
Conversation
00_uninstall.sql (added in #431 for standalone uninstall) was placed in install/ and picked up by the file glob, making it the first script executed on every install — including upgrades. This silently dropped the PerformanceMonitor database before recreating it empty. Four fixes: 1. Exclude 00_* from the install file list (both CLI and GUI), matching the existing 97_/99_ exclusion pattern. 2. Abort installation when any upgrade script fails instead of falling through to the full install path over a partially-upgraded database. New CLI exit code 8 (UpgradesFailed). 3. Version detection fallback: when the database exists but installation_history has no SUCCESS rows (prior GUI bug), return "1.0.0" so all idempotent upgrades are attempted rather than treating it as a fresh install. 4. Increase upgrade script timeout from 5 minutes to 1 hour for data migrations on large tables (compress_query_stats on 240GB+ DBs). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Should we check if a database backup has occurred recently for upgrades, and warn the user with a "do you want to proceed?" message? |
Owner
Author
|
@HannahVernon I've thought about things like this, including adding a backup option to the installer for the upgrade process, but there's a lot of moving parts and potential permissions issues, and of course confusion if AGs are in use and backups are being taken elsewhere. It's not impossible, but it is not something I'd feel very confident executing on. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #538. Fixes #539.
00_uninstall.sqlexcluded from install file list — it was being picked up by theinstall/*.sqlglob and executed as the first step of every install, silently dropping the database. Now filtered alongside97_/99_.8(UpgradesFailed).installation_historyexists but has noSUCCESSrows (prior GUI bug), returns"1.0.0"so all idempotent upgrades are attempted rather than treating it as a fresh install.compress_query_statsdata migration timed out on 240GB+ databases at the old 300sCommandTimeout.Root cause
PR #431 added
install/00_uninstall.sqlas a standalone script for SSMS users. Both installers build their file list by globbinginstall/*.sqlsorted alphabetically — so00_uninstall.sqlbecame the first file executed on every install, including upgrades. Pre-release testing on small databases didn't catch it because there was no data to miss.Test plan
--reinstall): verify00_uninstall.sqldoes NOT appear in the execution log01_install_database.sql00_uninstallinstallation_history: verify "1.0.0" fallback, upgrades attempted--uninstallflag still works (uses its own code path, not the file list)🤖 Generated with Claude Code