From e04fe3cf803e211bcbb947da4992004b4884f21b Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 14:24:28 +0200 Subject: [PATCH 01/22] Keeps what a table looked like before the migrations A backup table holds the rows and, on both engines, very little else: LIKE copies columns and their types but no indexes, no keys and no sequence, so what an admin has afterwards is not enough to rebuild the table it came from. The migrations table is somewhere a migration can leave what it knew before it acted, grouped by the run that wrote it. The backup step uses it to keep each table's definition. A run that is interrupted and started again is the same run, finds what it already wrote and leaves it alone, so the definition stays the one taken before anything changed. Signed-off-by: albertlast --- Sources/Db/Schema/v3_0/Migrations.php | 110 +++++++++++++ Sources/Maintenance/MigrationData.php | 226 ++++++++++++++++++++++++++ Sources/Maintenance/Tools/Upgrade.php | 56 ++++++- 3 files changed, 391 insertions(+), 1 deletion(-) create mode 100644 Sources/Db/Schema/v3_0/Migrations.php create mode 100644 Sources/Maintenance/MigrationData.php diff --git a/Sources/Db/Schema/v3_0/Migrations.php b/Sources/Db/Schema/v3_0/Migrations.php new file mode 100644 index 0000000000..4f3bdda3f8 --- /dev/null +++ b/Sources/Db/Schema/v3_0/Migrations.php @@ -0,0 +1,110 @@ +name = 'migrations'; + + $this->columns = [ + 'id_entry' => new Column( + name: 'id_entry', + type: 'int', + unsigned: true, + not_null: true, + auto: true, + ), + 'id_run' => new Column( + name: 'id_run', + type: 'varchar', + size: 36, + not_null: true, + default: '', + ), + 'migration' => new Column( + name: 'migration', + type: 'varchar', + size: 255, + not_null: true, + default: '', + ), + 'data_type' => new Column( + name: 'data_type', + type: 'varchar', + size: 30, + not_null: true, + default: '', + ), + 'data_key' => new Column( + name: 'data_key', + type: 'varchar', + size: 255, + not_null: true, + default: '', + ), + 'data' => new Column( + name: 'data', + type: 'mediumtext', + not_null: true, + ), + 'time_added' => new Column( + name: 'time_added', + type: 'bigint', + unsigned: true, + not_null: true, + default: 0, + ), + ]; + + $this->indexes = [ + 'primary' => new DbIndex( + type: 'primary', + columns: [ + [ + 'name' => 'id_entry', + ], + ], + ), + 'idx_run' => new DbIndex( + name: 'idx_run', + columns: [ + [ + 'name' => 'id_run', + ], + [ + 'name' => 'data_type', + ], + ], + ), + ]; + } +} diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php new file mode 100644 index 0000000000..c8ec6188dd --- /dev/null +++ b/Sources/Maintenance/MigrationData.php @@ -0,0 +1,226 @@ +insert( + 'insert', + '{db_prefix}migrations', + [ + 'id_run' => 'string-36', + 'migration' => 'string-255', + 'data_type' => 'string-30', + 'data_key' => 'string-255', + 'data' => 'string', + 'time_added' => 'int', + ], + [ + [$run, $migration, $type, $key, $data, time()], + ], + ['id_entry'], + ); + + return true; + } + + /** + * Reads one entry back. + * + * @param string $run The run that wrote it. + * @param string $type What kind of thing it is. + * @param string $key What it is about. + * @return string|null The thing, or null if this run never recorded it. + */ + public static function get(string $run, string $type, string $key): ?string + { + if (!self::exists()) { + return null; + } + + $request = Db::$db->query( + 'SELECT data + FROM {db_prefix}migrations + WHERE id_run = {string:run} + AND data_type = {string:type} + AND data_key = {string:key} + LIMIT 1', + [ + 'run' => $run, + 'type' => $type, + 'key' => $key, + ], + ); + + $row = Db::$db->fetch_assoc($request); + Db::$db->free_result($request); + + return $row === false || $row === null ? null : (string) $row['data']; + } + + /** + * Reads back everything one run recorded of a kind. + * + * @param string $run The run that wrote them. + * @param string $type What kind of thing they are. + * @return array The things, keyed by what each is about. + */ + public static function all(string $run, string $type): array + { + if (!self::exists()) { + return []; + } + + $entries = []; + + $request = Db::$db->query( + 'SELECT data_key, data + FROM {db_prefix}migrations + WHERE id_run = {string:run} + AND data_type = {string:type} + ORDER BY data_key', + [ + 'run' => $run, + 'type' => $type, + ], + ); + + while ($row = Db::$db->fetch_assoc($request)) { + $entries[$row['data_key']] = (string) $row['data']; + } + + Db::$db->free_result($request); + + return $entries; + } + + /** + * Removes one entry. + * + * @param string $run The run that wrote it. + * @param string $type What kind of thing it is. + * @param string $key What it is about. + */ + public static function forget(string $run, string $type, string $key): void + { + if (!self::exists()) { + return; + } + + Db::$db->query( + 'DELETE FROM {db_prefix}migrations + WHERE id_run = {string:run} + AND data_type = {string:type} + AND data_key = {string:key}', + [ + 'run' => $run, + 'type' => $type, + 'key' => $key, + ], + ); + } + + /** + * Whether there is anywhere to record this. + * + * The table arrives with 3.0, so anything asking before it has been created + * has nowhere to write. A migration that cannot record something carries on + * rather than ending the upgrade, so this is asked rather than left to the + * query to discover. + * + * Only a yes is remembered. A no is the answer until the table is made, and + * the run that makes it is usually the one asking. + * + * @return bool Whether the table is there. + */ + public static function exists(): bool + { + static $exists = false; + + return $exists = $exists || (new Migrations())->exists(true); + } + + /** + * Makes the table if it is not there yet. + * + * The upgrader reaches the backup step before it reaches the migrations + * that build the 3.0 schema, so anything wanting to record what a table + * looked like beforehand has to ask for this first. + * + * @return bool Whether there is a table to write to now. + */ + public static function ensure(): bool + { + if (self::exists()) { + return true; + } + + (new Migrations())->normalize(); + + return self::exists(); + } +} diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 2b3a633535..213c09c5af 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -25,6 +25,7 @@ use SMF\Maintenance\GenericSubStep; use SMF\Maintenance\Maintenance; use SMF\Maintenance\Migration; +use SMF\Maintenance\MigrationData; use SMF\Maintenance\Step; use SMF\Maintenance\Utf8ConverterStep; use SMF\QueryString; @@ -36,6 +37,7 @@ use SMF\User; use SMF\UserDataset; use SMF\Utils; +use SMF\Uuid; /** * Upgrade tool. @@ -333,6 +335,16 @@ class Upgrade extends ToolsBase implements ToolsInterface */ protected string $start_smf_version = ''; + /** + * @var string + * + * Identifies this upgrade, and stays the same when it is started again + * after being interrupted. What a migration records against it therefore + * describes the database as this upgrade found it, not as a later attempt + * found it half changed. + */ + protected string $id_run = ''; + /** * @var null|string * @@ -1380,13 +1392,49 @@ public function backupRecommended(): bool */ public function doBackupTable($table): bool { - return Db::$db->backup_table($table, 'backup_' . $table); + $this->recordDefinition($table); + + return Db::$db->backup_table($table, 'backup_' . $table) !== false; } /****************** * Internal methods ******************/ + /** + * Records what a table looked like before the migrations reach it. + * + * The backup holds the rows. This holds the shape they were in: the SQL + * that would build the table again, with its indexes, its keys and, on + * PostgreSQL, a sequence of its own. Without it a backup table is a set of + * columns and nothing else, which is not enough to put a forum back. + * + * Only the first pass of a run records anything. A run that was + * interrupted and started again reaches this a second time, over a + * database the migrations have already changed, and what it would write + * then is not what the admin wanted a copy of. + * + * @param string $table Name of the table, with the prefix on it. + */ + private function recordDefinition(string $table): void + { + if (!MigrationData::ensure()) { + return; + } + + if (MigrationData::get($this->id_run, MigrationData::TYPE_DEFINITION, $table) !== null) { + return; + } + + MigrationData::save( + $this->id_run, + static::class, + MigrationData::TYPE_DEFINITION, + $table, + Db::$db->table_sql($table), + ); + } + /** * Prepare the configuration to handle support with some older installs. */ @@ -1441,6 +1489,11 @@ private function getProgress(): void $this->user['name'] = (string) ($data['user_name'] ?? ''); $this->user['maint'] = (int) ($data['maint'] ?? Config::$maintenance); $this->start_smf_version = str_replace(' ', '.', strtolower($data['smf_version'] ?? Config::$modSettings['smfVersion'] ?? '0.0.dev.0')); + $this->id_run = (string) ($data['run'] ?? ''); + + if ($this->id_run === '') { + $this->id_run = (string) Uuid::create(); + } } /** @@ -1460,6 +1513,7 @@ private function saveProgress(): bool 'user_name' => $this->user['name'], 'maint' => $this->user['maint'] ?? 0, 'smf_version' => $this->start_smf_version, + 'run' => $this->id_run, ])); } else { $data = ''; From cbba1eb3e3e8c689ad65c28792442738a8372881 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 14:25:42 +0200 Subject: [PATCH 02/22] Copies rows into a backup without copying its source's sequence A default is copied as the expression it is written with, so a column fed by a sequence left backup_smf_calendar_holidays.id_holiday pointing at smf_calendar_holidays_seq. The backup became an object that sequence could not be dropped without, and HolidaysToEvents drops it. Nothing is lost by leaving the defaults out now: the table's definition is recorded before the migrations run, and it holds the defaults along with the indexes, the keys and the sequence that a copy never carried. Signed-off-by: albertlast --- Sources/Db/APIs/PostgreSQL.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Sources/Db/APIs/PostgreSQL.php b/Sources/Db/APIs/PostgreSQL.php index e3c39760ab..75fb7ece35 100644 --- a/Sources/Db/APIs/PostgreSQL.php +++ b/Sources/Db/APIs/PostgreSQL.php @@ -975,14 +975,18 @@ public function backup_table(string $table, string $backup_table): object|bool ); } - /** - * @todo Should we create backups of sequences as well? - */ + // The copy takes the columns and their types. It does not take their + // defaults, because a default is copied as the expression it is written + // with: a column fed by a sequence would arrive pointing at the live + // table's sequence, leaving the backup as an object that sequence + // cannot be dropped without and drawing ids from it if anything were + // inserted here. What each column defaults to is kept by the upgrader + // alongside the rest of the table's definition, which is a fuller + // record than a copy of the defaults would have been. $result = $this->query( 'CREATE TABLE {raw:backup_table} ( LIKE {raw:table} - INCLUDING DEFAULTS )', [ 'backup_table' => $backup_table, From 9e65c10bf089e64993da2cc662ab184d8f0f2e0f Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 14:39:39 +0200 Subject: [PATCH 03/22] Keeps the run's identity where a killed process cannot take it The progress data in Settings.php is written by preExit(), which the command line reaches only once the upgrade has finished -- and a killed process never reaches at all -- so on that path nothing is ever written and every attempt would look like a new run. The settings table is what both ways of running this share and what survives being killed, so the id lives there, and the step that records the new version forgets it again. Signed-off-by: albertlast --- Sources/Maintenance/Tools/Upgrade.php | 53 +++++++++++++++++++++------ 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 213c09c5af..510eab0eb0 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -341,7 +341,8 @@ class Upgrade extends ToolsBase implements ToolsInterface * Identifies this upgrade, and stays the same when it is started again * after being interrupted. What a migration records against it therefore * describes the database as this upgrade found it, not as a later attempt - * found it half changed. + * found it half changed. Read through getRunId(), which knows where it + * lives. */ protected string $id_run = ''; @@ -1220,8 +1221,14 @@ public function finalize(): bool Utils::$context['form_action'] = Config::$boardurl . '/index.php'; - // Update the database with the new SMF version. - $this->updateModSettings(['smfVersion' => SMF_VERSION]); + // Update the database with the new SMF version. Forgetting which run + // this was goes with it: whatever upgrades this forum next is a + // different one, and records what it finds rather than reading the + // notes this one left. + $this->updateModSettings([ + 'smfVersion' => SMF_VERSION, + 'upgrade_run' => '', + ]); // Clean any old cache files away. CacheApi::load(); @@ -1422,12 +1429,14 @@ private function recordDefinition(string $table): void return; } - if (MigrationData::get($this->id_run, MigrationData::TYPE_DEFINITION, $table) !== null) { + $run = $this->getRunId(); + + if (MigrationData::get($run, MigrationData::TYPE_DEFINITION, $table) !== null) { return; } MigrationData::save( - $this->id_run, + $run, static::class, MigrationData::TYPE_DEFINITION, $table, @@ -1435,6 +1444,34 @@ private function recordDefinition(string $table): void ); } + /** + * What identifies this upgrade, making one if there is not one yet. + * + * It is kept in the settings table rather than in the progress data in + * Settings.php, because that is written by preExit(), which the command + * line reaches only once the upgrade has finished and a killed process + * never reaches at all. The database is the one place both ways of running + * this can leave something behind. + * + * @return string The run's id. + */ + private function getRunId(): string + { + if ($this->id_run !== '') { + return $this->id_run; + } + + $this->id_run = (string) (Config::$modSettings['upgrade_run'] ?? ''); + + if ($this->id_run === '') { + $this->id_run = (string) Uuid::create(); + + $this->updateModSettings(['upgrade_run' => $this->id_run]); + } + + return $this->id_run; + } + /** * Prepare the configuration to handle support with some older installs. */ @@ -1489,11 +1526,6 @@ private function getProgress(): void $this->user['name'] = (string) ($data['user_name'] ?? ''); $this->user['maint'] = (int) ($data['maint'] ?? Config::$maintenance); $this->start_smf_version = str_replace(' ', '.', strtolower($data['smf_version'] ?? Config::$modSettings['smfVersion'] ?? '0.0.dev.0')); - $this->id_run = (string) ($data['run'] ?? ''); - - if ($this->id_run === '') { - $this->id_run = (string) Uuid::create(); - } } /** @@ -1513,7 +1545,6 @@ private function saveProgress(): bool 'user_name' => $this->user['name'], 'maint' => $this->user['maint'] ?? 0, 'smf_version' => $this->start_smf_version, - 'run' => $this->id_run, ])); } else { $data = ''; From c65554b818de5fad1eb7af64f9ce6729e6c82b8c Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 14:55:14 +0200 Subject: [PATCH 04/22] Gives each upgrade a row of its own The detail table alone had nowhere to say what a run was, and the id had to be parked in a setting to survive being killed. A run now has a row: where it started from, who started it, when it was last heard from, and where it had got to. A run is open until something says it finished, so a killed process leaves its row behind and the next attempt finds it and carries on as the same run. The position is written as each substep begins, which is a record of where an upgrade stopped rather than a place to resume from: restarting still begins at the top. Signed-off-by: albertlast --- .../{Migrations.php => MigrationData.php} | 4 +- Sources/Db/Schema/v3_0/MigrationRuns.php | 130 ++++++++++++++++ Sources/Maintenance/MigrationData.php | 143 +++++++++++++++++- Sources/Maintenance/Tools/Upgrade.php | 45 ++++-- 4 files changed, 297 insertions(+), 25 deletions(-) rename Sources/Db/Schema/v3_0/{Migrations.php => MigrationData.php} (96%) create mode 100644 Sources/Db/Schema/v3_0/MigrationRuns.php diff --git a/Sources/Db/Schema/v3_0/Migrations.php b/Sources/Db/Schema/v3_0/MigrationData.php similarity index 96% rename from Sources/Db/Schema/v3_0/Migrations.php rename to Sources/Db/Schema/v3_0/MigrationData.php index 4f3bdda3f8..2ff620f3ee 100644 --- a/Sources/Db/Schema/v3_0/Migrations.php +++ b/Sources/Db/Schema/v3_0/MigrationData.php @@ -22,7 +22,7 @@ /** * Defines all the properties for a database table. */ -class Migrations extends Table +class MigrationData extends Table { /**************** * Public methods @@ -33,7 +33,7 @@ class Migrations extends Table */ public function __construct() { - $this->name = 'migrations'; + $this->name = 'migration_data'; $this->columns = [ 'id_entry' => new Column( diff --git a/Sources/Db/Schema/v3_0/MigrationRuns.php b/Sources/Db/Schema/v3_0/MigrationRuns.php new file mode 100644 index 0000000000..292fb9a901 --- /dev/null +++ b/Sources/Db/Schema/v3_0/MigrationRuns.php @@ -0,0 +1,130 @@ +name = 'migration_runs'; + + $this->columns = [ + 'id_run' => new Column( + name: 'id_run', + type: 'varchar', + size: 36, + not_null: true, + default: '', + ), + 'version_from' => new Column( + name: 'version_from', + type: 'varchar', + size: 20, + not_null: true, + default: '', + ), + 'version_to' => new Column( + name: 'version_to', + type: 'varchar', + size: 20, + not_null: true, + default: '', + ), + 'step' => new Column( + name: 'step', + type: 'smallint', + unsigned: true, + not_null: true, + default: 0, + ), + 'substep' => new Column( + name: 'substep', + type: 'int', + unsigned: true, + not_null: true, + default: 0, + ), + 'substep_start' => new Column( + name: 'substep_start', + type: 'int', + unsigned: true, + not_null: true, + default: 0, + ), + 'id_member' => new Column( + name: 'id_member', + type: 'int', + unsigned: true, + not_null: true, + default: 0, + ), + 'time_started' => new Column( + name: 'time_started', + type: 'bigint', + unsigned: true, + not_null: true, + default: 0, + ), + 'time_updated' => new Column( + name: 'time_updated', + type: 'bigint', + unsigned: true, + not_null: true, + default: 0, + ), + 'time_finished' => new Column( + name: 'time_finished', + type: 'bigint', + unsigned: true, + not_null: true, + default: 0, + ), + ]; + + $this->indexes = [ + 'primary' => new DbIndex( + type: 'primary', + columns: [ + [ + 'name' => 'id_run', + ], + ], + ), + 'idx_time_finished' => new DbIndex( + name: 'idx_time_finished', + columns: [ + [ + 'name' => 'time_finished', + ], + ], + ), + ]; + } +} diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index c8ec6188dd..6f1d410fd7 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -16,7 +16,8 @@ namespace SMF\Maintenance; use SMF\Db\DatabaseApi as Db; -use SMF\Db\Schema\v3_0\Migrations; +use SMF\Db\Schema\v3_0\MigrationData as MigrationDataTable; +use SMF\Db\Schema\v3_0\MigrationRuns as MigrationRunsTable; /** * What a migration needs to still know later. @@ -70,7 +71,7 @@ public static function save(string $run, string $migration, string $type, string Db::$db->insert( 'insert', - '{db_prefix}migrations', + '{db_prefix}migration_data', [ 'id_run' => 'string-36', 'migration' => 'string-255', @@ -104,7 +105,7 @@ public static function get(string $run, string $type, string $key): ?string $request = Db::$db->query( 'SELECT data - FROM {db_prefix}migrations + FROM {db_prefix}migration_data WHERE id_run = {string:run} AND data_type = {string:type} AND data_key = {string:key} @@ -139,7 +140,7 @@ public static function all(string $run, string $type): array $request = Db::$db->query( 'SELECT data_key, data - FROM {db_prefix}migrations + FROM {db_prefix}migration_data WHERE id_run = {string:run} AND data_type = {string:type} ORDER BY data_key', @@ -172,7 +173,7 @@ public static function forget(string $run, string $type, string $key): void } Db::$db->query( - 'DELETE FROM {db_prefix}migrations + 'DELETE FROM {db_prefix}migration_data WHERE id_run = {string:run} AND data_type = {string:type} AND data_key = {string:key}', @@ -184,6 +185,133 @@ public static function forget(string $run, string $type, string $key): void ); } + /** + * The run that is under way, if there is one. + * + * A run is under way until something says it finished, so a process that + * was killed leaves its row behind and the next attempt finds it. That is + * what makes a restart the same run rather than a new one, and it is why + * this is asked of the database rather than of the progress data in + * Settings.php, which the command line never writes. + * + * @return string The run's id, or an empty string if none is open. + */ + public static function currentRun(): string + { + if (!self::exists()) { + return ''; + } + + $request = Db::$db->query( + 'SELECT id_run + FROM {db_prefix}migration_runs + WHERE time_finished = {int:unfinished} + ORDER BY time_started DESC + LIMIT 1', + [ + 'unfinished' => 0, + ], + ); + + $row = Db::$db->fetch_assoc($request); + Db::$db->free_result($request); + + return $row === false || $row === null ? '' : (string) $row['id_run']; + } + + /** + * Opens a run. + * + * @param string $run The id to give it. + * @param string $from The version the forum is on now. + * @param int $member Who started it, if that is known. + * @return bool Whether it was recorded. + */ + public static function startRun(string $run, string $from, int $member = 0): bool + { + if (!self::exists()) { + return false; + } + + Db::$db->insert( + 'ignore', + '{db_prefix}migration_runs', + [ + 'id_run' => 'string-36', + 'version_from' => 'string-20', + 'id_member' => 'int', + 'time_started' => 'int', + 'time_updated' => 'int', + ], + [ + [$run, $from, $member, time(), time()], + ], + ['id_run'], + ); + + return true; + } + + /** + * Records how far a run has got. + * + * The upgrader keeps its place in the query string, which is gone the + * moment the process is. This is the copy that outlives it. + * + * @param string $run The run. + * @param int $step Which step it is on. + * @param int $substep Which substep of that step. + * @param int $start How far into the substep. + */ + public static function recordPosition(string $run, int $step, int $substep, int $start): void + { + if ($run === '' || !self::exists()) { + return; + } + + Db::$db->query( + 'UPDATE {db_prefix}migration_runs + SET step = {int:step}, + substep = {int:substep}, + substep_start = {int:start}, + time_updated = {int:now} + WHERE id_run = {string:run}', + [ + 'step' => $step, + 'substep' => $substep, + 'start' => $start, + 'now' => time(), + 'run' => $run, + ], + ); + } + + /** + * Closes a run, so that the next one is a new one. + * + * @param string $run The run. + * @param string $to The version the forum is on now. + */ + public static function finishRun(string $run, string $to): void + { + if ($run === '' || !self::exists()) { + return; + } + + Db::$db->query( + 'UPDATE {db_prefix}migration_runs + SET version_to = {string:to}, + time_updated = {int:now}, + time_finished = {int:now} + WHERE id_run = {string:run}', + [ + 'to' => $to, + 'now' => time(), + 'run' => $run, + ], + ); + } + /** * Whether there is anywhere to record this. * @@ -201,7 +329,7 @@ public static function exists(): bool { static $exists = false; - return $exists = $exists || (new Migrations())->exists(true); + return $exists = $exists || ((new MigrationDataTable())->exists(true) && (new MigrationRunsTable())->exists(true)); } /** @@ -219,7 +347,8 @@ public static function ensure(): bool return true; } - (new Migrations())->normalize(); + (new MigrationDataTable())->normalize(); + (new MigrationRunsTable())->normalize(); return self::exists(); } diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 510eab0eb0..580bcf0bbd 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -1221,14 +1221,13 @@ public function finalize(): bool Utils::$context['form_action'] = Config::$boardurl . '/index.php'; - // Update the database with the new SMF version. Forgetting which run - // this was goes with it: whatever upgrades this forum next is a + // Update the database with the new SMF version. + $this->updateModSettings(['smfVersion' => SMF_VERSION]); + + // Closing the run goes with it. Whatever upgrades this forum next is a // different one, and records what it finds rather than reading the // notes this one left. - $this->updateModSettings([ - 'smfVersion' => SMF_VERSION, - 'upgrade_run' => '', - ]); + MigrationData::finishRun($this->getRunId(), SMF_VERSION); // Clean any old cache files away. CacheApi::load(); @@ -1425,12 +1424,12 @@ public function doBackupTable($table): bool */ private function recordDefinition(string $table): void { - if (!MigrationData::ensure()) { + $run = $this->getRunId(); + + if ($run === '') { return; } - $run = $this->getRunId(); - if (MigrationData::get($run, MigrationData::TYPE_DEFINITION, $table) !== null) { return; } @@ -1447,11 +1446,11 @@ private function recordDefinition(string $table): void /** * What identifies this upgrade, making one if there is not one yet. * - * It is kept in the settings table rather than in the progress data in - * Settings.php, because that is written by preExit(), which the command - * line reaches only once the upgrade has finished and a killed process - * never reaches at all. The database is the one place both ways of running - * this can leave something behind. + * A run stays open until something says it finished, so a process that was + * killed leaves its row behind and this finds it again. The progress data + * in Settings.php could not do this: it is written by preExit(), which the + * command line reaches only once the upgrade has finished, and which a + * killed process never reaches at all. * * @return string The run's id. */ @@ -1461,12 +1460,16 @@ private function getRunId(): string return $this->id_run; } - $this->id_run = (string) (Config::$modSettings['upgrade_run'] ?? ''); + if (!MigrationData::ensure()) { + return ''; + } + + $this->id_run = MigrationData::currentRun(); if ($this->id_run === '') { $this->id_run = (string) Uuid::create(); - $this->updateModSettings(['upgrade_run' => $this->id_run]); + MigrationData::startRun($this->id_run, $this->start_smf_version, $this->user['id'] ?? 0); } return $this->id_run; @@ -1764,6 +1767,16 @@ private function performSubsteps(array $substeps, int $offset = 0, ?int $total = while (Maintenance::getCurrentSubStep() - $offset < \count($substeps)) { $substep = $substeps[Maintenance::getCurrentSubStep() - $offset]; + // Where this run has got to, somewhere a killed process cannot + // take with it. The step and substep themselves live in the query + // string, which goes when the request does. + MigrationData::recordPosition( + $this->getRunId(), + Maintenance::getCurrentStep(), + Maintenance::getCurrentSubStep(), + Maintenance::getCurrentStart(), + ); + $this->logProgress(' +++ ' . $substep->name, true); // If this is not a canidate for us to execute, skip it. From 2f94acb7837a2e45bee297504861e6ba95a60642 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 15:23:14 +0200 Subject: [PATCH 05/22] Leaves a backup this run already took alone backup_table() drops the backup table before it writes it, so an upgrade that is started again after being interrupted replaces a copy of the database as the admin had it with a copy of it half migrated. The run now records each table it has copied, and passes over the ones it did. This is what the copies are for. The definitions taken alongside them describe the tables as they were, and neither is worth much if the rows beside them describe something else. Signed-off-by: albertlast --- Sources/Maintenance/MigrationData.php | 6 ++++++ Sources/Maintenance/Tools/Upgrade.php | 20 +++++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index 6f1d410fd7..2a0815cac6 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -46,6 +46,12 @@ class MigrationData */ public const TYPE_DEFINITION = 'definition'; + /** + * That a table has been copied to a backup_ one by this run, so that a run + * which starts again does not copy over the copy it already made. + */ + public const TYPE_BACKUP = 'backup'; + /*********************** * Public static methods ***********************/ diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 580bcf0bbd..25583a1ca1 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -1398,9 +1398,27 @@ public function backupRecommended(): bool */ public function doBackupTable($table): bool { + $run = $this->getRunId(); + + // backup_table() drops the backup before it writes it, so a run that + // is started again would replace a copy of the database as it was with + // a copy of it half migrated. The copy this run already made is the + // one worth having. + if ($run !== '' && MigrationData::get($run, MigrationData::TYPE_BACKUP, $table) !== null) { + return true; + } + $this->recordDefinition($table); - return Db::$db->backup_table($table, 'backup_' . $table) !== false; + if (Db::$db->backup_table($table, 'backup_' . $table) === false) { + return false; + } + + if ($run !== '') { + MigrationData::save($run, static::class, MigrationData::TYPE_BACKUP, $table, (string) time()); + } + + return true; } /****************** From 8d297870485c59f75fc26fc7ffad428b7a430dba Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 15:29:12 +0200 Subject: [PATCH 06/22] Records the functions a table's definition leans on An index can be built over an expression rather than a column, and members has one over indexable_month_day(birthdate). The SQL that rebuilds the table names the function without saying what it is, so a definition on its own cannot rebuild the table it describes. The upgrader's own tables are left out of the backup at the same time. A copy of the record of what was copied helps nobody putting a forum back, and restoring it would put back an older account of the run doing the restoring. Signed-off-by: albertlast --- Sources/Maintenance/MigrationData.php | 8 ++++ Sources/Maintenance/Tools/Upgrade.php | 68 ++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index 2a0815cac6..8a46ca6984 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -52,6 +52,14 @@ class MigrationData */ public const TYPE_BACKUP = 'backup'; + /** + * A function the database held before the migrations reach it, as the SQL + * that would create it again. A table's definition can lean on one -- an + * index over an expression names the function it calls -- so the two are + * only useful together. + */ + public const TYPE_ROUTINE = 'routine'; + /*********************** * Public static methods ***********************/ diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 25583a1ca1..e109aad185 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -346,6 +346,14 @@ class Upgrade extends ToolsBase implements ToolsInterface */ protected string $id_run = ''; + /** + * @var bool + * + * Whether the database's functions have been looked at yet. They are the + * same for every table, so they are read once rather than per table. + */ + protected bool $routines_recorded = false; + /** * @var null|string * @@ -1065,9 +1073,13 @@ public function backupDatabase(): bool $tables = Db::$db->list_tables($db, $filter); - // Filter out backup tables. + // Filter out backup tables, and the upgrader's own bookkeeping. A copy + // of the record of what was copied is of no use to anybody putting a + // forum back, and restoring it would put back an older account of the + // run doing the restoring. $table_names = array_filter($tables, function ($table) { - return !str_starts_with($table, 'backup_'); + return !str_starts_with($table, 'backup_') + && !str_starts_with($table, Config::$db_prefix . 'migration_'); }); Maintenance::$total_substeps = \count($table_names); @@ -1408,6 +1420,7 @@ public function doBackupTable($table): bool return true; } + $this->recordRoutines(); $this->recordDefinition($table); if (Db::$db->backup_table($table, 'backup_' . $table) === false) { @@ -1425,6 +1438,57 @@ public function doBackupTable($table): bool * Internal methods ******************/ + /** + * Records the functions the database held before the migrations reach it. + * + * A table's definition is not enough on its own. An index can be built over + * an expression rather than a column -- members has one over + * indexable_month_day(birthdate) -- and the SQL that rebuilds the table + * names the function without saying what it is. Recording them together is + * what makes the pair worth keeping. + * + * Only PostgreSQL has anything to record here. MySQL is given none of its + * own, and the functions SMF adds to PostgreSQL are the ones in the public + * schema, since everything the server ships with lives in pg_catalog. + */ + private function recordRoutines(): void + { + if ($this->routines_recorded || Db::$db->title !== POSTGRE_TITLE) { + return; + } + + $this->routines_recorded = true; + + $run = $this->getRunId(); + + if ($run === '' || MigrationData::all($run, MigrationData::TYPE_ROUTINE) !== []) { + return; + } + + $request = Db::$db->query( + 'SELECT p.oid::regprocedure AS signature, pg_get_functiondef(p.oid) AS definition + FROM pg_proc AS p + INNER JOIN pg_namespace AS n ON (n.oid = p.pronamespace) + WHERE n.nspname = {string:schema} + ORDER BY signature', + [ + 'schema' => 'public', + ], + ); + + while ($row = Db::$db->fetch_assoc($request)) { + MigrationData::save( + $run, + static::class, + MigrationData::TYPE_ROUTINE, + $row['signature'], + $row['definition'], + ); + } + + Db::$db->free_result($request); + } + /** * Records what a table looked like before the migrations reach it. * From 3ddd6ac2cd6433a9a1b2c95190ab027c0697b2b6 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 15:37:05 +0200 Subject: [PATCH 07/22] Notes what a setting held before the upgrade changed it A database put back to the shape it had is not a working forum if Settings.php still describes the one it was upgraded to. db_character_set and db_mb4 say what the database is, and after a rollback they would be saying it about a database that is no longer there. Only the settings the upgrade writes are noted, and only the first time each is touched, so this is a note of what to put back rather than a copy of the file. Settings.php holds the database password, and a copy of that inside the database would be in every dump taken from then on, so it and its like are left out. Signed-off-by: albertlast --- Sources/Maintenance/MigrationData.php | 6 ++ Sources/Maintenance/Tools/Upgrade.php | 101 +++++++++++++++++++++++--- 2 files changed, 98 insertions(+), 9 deletions(-) diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index 8a46ca6984..257fc6b78b 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -60,6 +60,12 @@ class MigrationData */ public const TYPE_ROUTINE = 'routine'; + /** + * What a setting in Settings.php held before the upgrade changed it, + * as JSON, with whether it was there at all. + */ + public const TYPE_SETTING = 'setting'; + /*********************** * Public static methods ***********************/ diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index e109aad185..282b02daa0 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -206,6 +206,32 @@ class Upgrade extends ToolsBase implements ToolsInterface ], ]; + /** + * @var string + * + * Identifies this upgrade, and stays the same when it is started again + * after being interrupted. What a migration records against it therefore + * describes the database as this upgrade found it, not as a later attempt + * found it half changed. Read through getRunId(), which knows where it + * lives. + */ + /** + * @var array + * + * Settings that are not recorded before being changed. The upgrade's own + * progress data is its bookkeeping and means nothing afterwards; the rest + * are things that should not be sitting in a database table, since a copy + * of the database password inside the database would be in every dump + * taken from then on. + */ + public const UNRECORDED_SETTINGS = [ + 'maintenance_tool_progress', + 'db_passwd', + 'db_user', + 'image_proxy_secret', + 'auth_secret', + ]; + /******************* * Public properties *******************/ @@ -335,15 +361,6 @@ class Upgrade extends ToolsBase implements ToolsInterface */ protected string $start_smf_version = ''; - /** - * @var string - * - * Identifies this upgrade, and stays the same when it is started again - * after being interrupted. What a migration records against it therefore - * describes the database as this upgrade found it, not as a later attempt - * found it half changed. Read through getRunId(), which knows where it - * lives. - */ protected string $id_run = ''; /** @@ -1434,10 +1451,76 @@ public function doBackupTable($table): bool return true; } + /** + * Writes settings to Settings.php, noting what they held first. + * + * @param array $config_vars The settings to write. + * @param bool|null $keep_quotes Whether to keep quotes in the values. + * @param bool $rebuild Whether to rebuild the file from scratch. + * @return bool Whether the file was written. + */ + public function updateSettingsFile(array $config_vars, ?bool $keep_quotes = null, bool $rebuild = false): bool + { + $this->recordSettings(array_keys($config_vars)); + + return parent::updateSettingsFile($config_vars, $keep_quotes, $rebuild); + } + /****************** * Internal methods ******************/ + /** + * Records what the settings being written held beforehand. + * + * A database put back to the shape it had is not a forum that works if + * Settings.php still describes the one it was upgraded to: db_character_set + * and db_mb4 in particular say what the database is, and after a rollback + * they would be saying it about a database that no longer exists. + * + * Only the settings the upgrade is about to change are recorded, and only + * the first time each is touched, so this is a note of what to put back + * rather than a copy of the file. Settings that are nobody else's business + * are left out: Settings.php holds the database password, and a copy of it + * inside the database would be in every dump taken from then on. + * + * @param array $names Names of the settings about to be written. + */ + private function recordSettings(array $names): void + { + $run = $this->getRunId(); + + if ($run === '') { + return; + } + + $current = Config::getCurrentSettings(); + + if (!\is_array($current)) { + return; + } + + foreach ($names as $name) { + if ( + \in_array($name, self::UNRECORDED_SETTINGS) + || MigrationData::get($run, MigrationData::TYPE_SETTING, $name) !== null + ) { + continue; + } + + MigrationData::save( + $run, + static::class, + MigrationData::TYPE_SETTING, + $name, + (string) json_encode([ + 'set' => \array_key_exists($name, $current), + 'value' => $current[$name] ?? null, + ]), + ); + } + } + /** * Records the functions the database held before the migrations reach it. * From feb383e157bc6d80e23745e2fb6b6e7d2c6e28de Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 16:01:39 +0200 Subject: [PATCH 08/22] Puts a database back the way an upgrade found it Everything needed was written down while the upgrade ran: what each table looked like, the functions its indexes call, the rows in the backup_ tables, and what the settings held. This puts them back in the order they depend on each other, drops the tables the upgrade added, and notes on the run that it has been undone. The upgrader's own tables are left alone throughout. They are where the answer is read from, and a rollback that took them with it could not record that it had happened. Nothing outside the database is touched. Signed-off-by: albertlast --- Sources/Db/Schema/v3_0/MigrationRuns.php | 7 + Sources/Maintenance/MigrationData.php | 26 ++ Sources/Maintenance/MigrationRollback.php | 334 ++++++++++++++++++++++ 3 files changed, 367 insertions(+) create mode 100644 Sources/Maintenance/MigrationRollback.php diff --git a/Sources/Db/Schema/v3_0/MigrationRuns.php b/Sources/Db/Schema/v3_0/MigrationRuns.php index 292fb9a901..ca9d437e09 100644 --- a/Sources/Db/Schema/v3_0/MigrationRuns.php +++ b/Sources/Db/Schema/v3_0/MigrationRuns.php @@ -106,6 +106,13 @@ public function __construct() not_null: true, default: 0, ), + 'time_rolled_back' => new Column( + name: 'time_rolled_back', + type: 'bigint', + unsigned: true, + not_null: true, + default: 0, + ), ]; $this->indexes = [ diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index 257fc6b78b..f522d2ad29 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -332,6 +332,32 @@ public static function finishRun(string $run, string $to): void ); } + /** + * Notes that a run has been undone. + * + * The row stays where it is. What the run did is still worth knowing about + * after it has been put back, and a forum that was upgraded and rolled back + * is a different thing from one that was never upgraded. + * + * @param string $run The run that was undone. + */ + public static function recordRollback(string $run): void + { + if ($run === '' || !self::exists()) { + return; + } + + Db::$db->query( + 'UPDATE {db_prefix}migration_runs + SET time_rolled_back = {int:now} + WHERE id_run = {string:run}', + [ + 'now' => time(), + 'run' => $run, + ], + ); + } + /** * Whether there is anywhere to record this. * diff --git a/Sources/Maintenance/MigrationRollback.php b/Sources/Maintenance/MigrationRollback.php new file mode 100644 index 0000000000..5d15d79a67 --- /dev/null +++ b/Sources/Maintenance/MigrationRollback.php @@ -0,0 +1,334 @@ +error = 'that run recorded nothing to put back'; + + return false; + } + + $backed_up = MigrationData::all($run, MigrationData::TYPE_BACKUP); + + // The rows are what makes this worth doing. Without them the tables + // would come back empty, which is worse than leaving things alone. + $missing = array_diff(array_keys($definitions), array_keys($backed_up)); + + if ($missing !== []) { + $this->error = 'no backup was taken of ' . implode(', ', \array_slice($missing, 0, 5)); + + return false; + } + + foreach ($this->routines($run) as $name => $sql) { + $this->execute($sql); + $this->log[] = 'function ' . $name; + } + + foreach ($definitions as $table => $sql) { + $this->execute($sql); + $this->refill($table); + $this->log[] = 'table ' . $table; + } + + foreach ($this->added($definitions) as $table) { + Db::$db->drop_table($table); + $this->log[] = 'dropped ' . $table; + } + + $this->restoreSettings($run); + + MigrationData::recordRollback($run); + + return true; + } + + /** + * The runs that could be undone, newest first. + * + * @return array Rows from the migration_runs table. + */ + public function candidates(): array + { + if (!MigrationData::exists()) { + return []; + } + + $runs = []; + + $request = Db::$db->query( + 'SELECT id_run, version_from, version_to, time_started, time_finished + FROM {db_prefix}migration_runs + WHERE time_rolled_back = {int:never} + ORDER BY time_started DESC', + [ + 'never' => 0, + ], + ); + + while ($row = Db::$db->fetch_assoc($request)) { + $runs[] = $row; + } + + Db::$db->free_result($request); + + return $runs; + } + + /****************** + * Internal methods + ******************/ + + /** + * The functions a run recorded. + * + * @param string $run The run. + * @return array The SQL that creates each, keyed by its signature. + */ + private function routines(string $run): array + { + return MigrationData::all($run, MigrationData::TYPE_ROUTINE); + } + + /** + * Tables that are here now and were not when the run started. + * + * The upgrader's own tables are not among them however this is counted: + * they are where the answer is being read from, and a rollback that took + * them with it could not record that it had happened. + * + * @param array $definitions What the run recorded, keyed by table name. + * @return array Names of the tables to drop, with the prefix on them. + */ + private function added(array $definitions): array + { + $added = []; + + foreach (Db::$db->list_tables() as $table) { + if ( + isset($definitions[$table]) + || str_starts_with($table, 'backup_') + || str_starts_with($table, Config::$db_prefix . 'migration_') + || !str_starts_with($table, Config::$db_prefix) + ) { + continue; + } + + $added[] = $table; + } + + return $added; + } + + /** + * Puts a table's rows back from its backup. + * + * @param string $table Name of the table, with the prefix on it. + */ + private function refill(string $table): void + { + if (Db::$db->list_tables(false, 'backup_' . $table) === []) { + return; + } + + Db::$db->query( + 'INSERT INTO {raw:table} + SELECT * FROM {raw:backup}', + [ + 'table' => $table, + 'backup' => 'backup_' . $table, + ], + ); + } + + /** + * Puts the settings in Settings.php back. + * + * A setting the run found missing is taken out again rather than written + * as an empty one, which is what the note beside it is for. + * + * @param string $run The run. + */ + private function restoreSettings(string $run): void + { + $settings = MigrationData::all($run, MigrationData::TYPE_SETTING); + + if ($settings === []) { + return; + } + + $put_back = []; + $remove = []; + + foreach ($settings as $name => $noted) { + $noted = json_decode($noted, true); + + if (!\is_array($noted)) { + continue; + } + + if (empty($noted['set'])) { + $remove[] = $name; + } else { + $put_back[$name] = $noted['value']; + } + } + + if ($put_back !== []) { + Config::updateSettingsFile($put_back); + $this->log[] = 'settings ' . implode(', ', array_keys($put_back)); + } + + if ($remove !== []) { + Config::updateSettingsFile(array_fill_keys($remove, null), rebuild: true); + $this->log[] = 'removed ' . implode(', ', $remove); + } + } + + /** + * Runs SQL that may be more than one statement. + * + * A recorded definition is a small script rather than a single statement, + * and the database layer takes one at a time, so it is split here. + * + * @param string $sql The SQL to run. + */ + private function execute(string $sql): void + { + foreach ($this->statements($sql) as $statement) { + Db::$db->query($statement, ['db_error_skip' => true]); + } + } + + /** + * Splits SQL into the statements it is made of. + * + * Quoting has to be respected while splitting: a function body is one + * string, and the semicolons inside it end nothing. + * + * @param string $sql The SQL. + * @return array The statements, without the semicolons between them. + */ + private function statements(string $sql): array + { + $statements = []; + $current = ''; + $quote = ''; + $length = \strlen($sql); + + for ($i = 0; $i < $length; $i++) { + $char = $sql[$i]; + + if ($quote !== '') { + // Inside a dollar quoted string, only its own tag ends it. + if ($quote[0] === '$') { + if (substr($sql, $i, \strlen($quote)) === $quote) { + $current .= $quote; + $i += \strlen($quote) - 1; + $quote = ''; + + continue; + } + } elseif ($char === '\\' && $i + 1 < $length) { + $current .= $char . $sql[++$i]; + + continue; + } elseif ($char === $quote) { + $quote = ''; + } + + $current .= $char; + + continue; + } + + if ($char === "'" || $char === '"' || $char === '`') { + $quote = $char; + } elseif ($char === '$' && preg_match('~^\$[A-Za-z_]*\$~', substr($sql, $i), $matches) === 1) { + $quote = $matches[0]; + $current .= $quote; + $i += \strlen($quote) - 1; + + continue; + } elseif ($char === ';') { + if (trim($current) !== '') { + $statements[] = trim($current); + } + + $current = ''; + + continue; + } + + $current .= $char; + } + + if (trim($current) !== '') { + $statements[] = trim($current); + } + + return $statements; + } +} From fa7136281815dfe35e15b9210254538f382ad3de Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 16:10:56 +0200 Subject: [PATCH 09/22] Lets the recorded SQL past the checks meant for assembled queries The database layer refuses a query holding a quote, and refuses one holding a semicolon whatever else it is told, so a recorded CREATE TABLE and the body of a recorded function were both turned away. Neither is built around anything supplied from outside: they are the database's own account of itself, read back. The installer and the migrations turn the same checks off around their own DDL. This puts the setting back as it found it. Signed-off-by: albertlast --- Sources/Maintenance/MigrationRollback.php | 24 ++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/Sources/Maintenance/MigrationRollback.php b/Sources/Maintenance/MigrationRollback.php index 5d15d79a67..0114e3c974 100644 --- a/Sources/Maintenance/MigrationRollback.php +++ b/Sources/Maintenance/MigrationRollback.php @@ -82,6 +82,15 @@ public function rollback(string $run): bool return false; } + // The recorded SQL is the database's own account of itself, and the + // checks that keep a query from being assembled out of user input have + // nothing to look at here: they refuse the quoting a CREATE TABLE is + // full of, and the semicolons inside a function body. The installer and + // the migrations turn them off around their own DDL for the same + // reason. + $checking = Db::$db->disableQueryCheck; + Db::$db->disableQueryCheck = true; + foreach ($this->routines($run) as $name => $sql) { $this->execute($sql); $this->log[] = 'function ' . $name; @@ -98,6 +107,8 @@ public function rollback(string $run): bool $this->log[] = 'dropped ' . $table; } + Db::$db->disableQueryCheck = $checking; + $this->restoreSettings($run); MigrationData::recordRollback($run); @@ -199,6 +210,7 @@ private function refill(string $table): void [ 'table' => $table, 'backup' => 'backup_' . $table, + 'db_error_skip' => true, ], ); } @@ -258,7 +270,17 @@ private function restoreSettings(string $run): void private function execute(string $sql): void { foreach ($this->statements($sql) as $statement) { - Db::$db->query($statement, ['db_error_skip' => true]); + // These are whole statements rather than something built around + // values, so the checks that keep a query from being assembled out + // of user input have nothing to look at here and reject the + // quoting a CREATE TABLE is full of. + Db::$db->query( + $statement, + [ + 'security_override' => true, + 'db_error_skip' => true, + ], + ); } } From e8d907086a1d35f0a3b1e73f11ff00d7ab961f1c Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 16:19:05 +0200 Subject: [PATCH 10/22] Finds the tables when the prefix names the database too On MySQL the prefix can be `smf`.smf_ while list_tables() answers with bare names, so anything comparing one against the other never finds a table that is sitting right there. The upgrader saw a plain prefix and recorded everything; a rollback asked for through SSI saw the qualified one and reported there was nothing to put back. Table::exists() compares them that way too, which is worth knowing about separately. Signed-off-by: albertlast --- Sources/Maintenance/MigrationData.php | 25 ++++++++++++++++++++++- Sources/Maintenance/MigrationRollback.php | 4 ++-- Sources/Maintenance/Tools/Upgrade.php | 2 +- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index f522d2ad29..6d97fb4046 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -375,7 +375,30 @@ public static function exists(): bool { static $exists = false; - return $exists = $exists || ((new MigrationDataTable())->exists(true) && (new MigrationRunsTable())->exists(true)); + if ($exists) { + return true; + } + + $tables = Db::$db->list_tables(); + $prefix = self::prefix(); + + return $exists = \in_array($prefix . 'migration_data', $tables) + && \in_array($prefix . 'migration_runs', $tables); + } + + /** + * The table prefix, without the database in front of it. + * + * On MySQL the prefix can name the database as well, as `smf`.smf_, while + * list_tables() answers with bare names. Anything comparing one against the + * other has to take the database off first, or it never finds a table that + * is sitting right there. + * + * @return string The prefix on its own. + */ + public static function prefix(): string + { + return preg_match('~^`(.+?)`\.(.+?)$~', Db::$db->prefix, $match) !== 0 ? $match[2] : Db::$db->prefix; } /** diff --git a/Sources/Maintenance/MigrationRollback.php b/Sources/Maintenance/MigrationRollback.php index 0114e3c974..c19cd21627 100644 --- a/Sources/Maintenance/MigrationRollback.php +++ b/Sources/Maintenance/MigrationRollback.php @@ -181,8 +181,8 @@ private function added(array $definitions): array if ( isset($definitions[$table]) || str_starts_with($table, 'backup_') - || str_starts_with($table, Config::$db_prefix . 'migration_') - || !str_starts_with($table, Config::$db_prefix) + || str_starts_with($table, MigrationData::prefix() . 'migration_') + || !str_starts_with($table, MigrationData::prefix()) ) { continue; } diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 282b02daa0..e59339184c 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -1096,7 +1096,7 @@ public function backupDatabase(): bool // run doing the restoring. $table_names = array_filter($tables, function ($table) { return !str_starts_with($table, 'backup_') - && !str_starts_with($table, Config::$db_prefix . 'migration_'); + && !str_starts_with($table, MigrationData::prefix() . 'migration_'); }); Maintenance::$total_substeps = \count($table_names); From ca0ba28c514d4d44efb7b6d91ac572cf2ee91c9f Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 16:40:03 +0200 Subject: [PATCH 11/22] Reports what a rollback could not run, and finds the tables to run it on A prefix that names the database means nothing ever selected one, since every query SMF makes says which database it means. The recorded SQL does not, having been written while the prefix was plain, so on MySQL every statement was refused with 'No database selected' and the rollback reported itself done having restored nothing. Errors are still skipped, so that one bad statement does not abandon a half restored forum, but what was refused is kept and told to the caller. Skipped is not the same as unnoticed. Signed-off-by: albertlast --- Sources/Maintenance/MigrationRollback.php | 40 ++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/Sources/Maintenance/MigrationRollback.php b/Sources/Maintenance/MigrationRollback.php index c19cd21627..e141916068 100644 --- a/Sources/Maintenance/MigrationRollback.php +++ b/Sources/Maintenance/MigrationRollback.php @@ -50,6 +50,14 @@ class MigrationRollback */ public string $error = ''; + /** + * @var array + * + * Statements the database refused, shortened. A rollback that reports + * itself done while these are not empty did not put everything back. + */ + public array $failures = []; + /**************** * Public methods ****************/ @@ -91,6 +99,17 @@ public function rollback(string $run): bool $checking = Db::$db->disableQueryCheck; Db::$db->disableQueryCheck = true; + // A prefix that names the database, as `smf`.smf_ does, means nothing + // ever selected one: every query says which database it means. The + // recorded SQL does not, since the upgrader wrote it while the prefix + // was a plain one, so the database has to be chosen before any of it + // will run at all. + $database = $this->database(); + + if ($database !== '') { + Db::$db->select($database); + } + foreach ($this->routines($run) as $name => $sql) { $this->execute($sql); $this->log[] = 'function ' . $name; @@ -163,6 +182,17 @@ private function routines(string $run): array return MigrationData::all($run, MigrationData::TYPE_ROUTINE); } + /** + * The database the prefix names, if it names one. + * + * @return string The database's name, or an empty string if the prefix is + * a plain one and a database has already been chosen. + */ + private function database(): string + { + return preg_match('~^`(.+?)`\.~', Db::$db->prefix, $match) !== 0 ? $match[1] : ''; + } + /** * Tables that are here now and were not when the run started. * @@ -274,13 +304,21 @@ private function execute(string $sql): void // values, so the checks that keep a query from being assembled out // of user input have nothing to look at here and reject the // quoting a CREATE TABLE is full of. - Db::$db->query( + $result = Db::$db->query( $statement, [ 'security_override' => true, 'db_error_skip' => true, ], ); + + // The errors are skipped so that one statement failing does not + // end the whole thing, which would leave a forum half put back. + // Skipped is not the same as unnoticed, though: what did not run + // is the difference between a rollback and the appearance of one. + if ($result === false) { + $this->failures[] = preg_replace('~\s+~', ' ', substr($statement, 0, 120)); + } } } From f241e1b9c3bc64acacc55289696ff9b1bf6307db Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 16:51:57 +0200 Subject: [PATCH 12/22] Puts a sequence where recreating it would have started it Dropping a table does not drop the sequence feeding it, so a sequence being put back is already there and asking for it again is refused. All 41 of them were refused on every rollback, and nothing noticed, because the sequences were already at usable values and the comparison saw nothing wrong. What the statement was for is the number it would have started at, and setval still gives that, whether or not the sequence survived. Signed-off-by: albertlast --- Sources/Maintenance/MigrationRollback.php | 34 ++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/Sources/Maintenance/MigrationRollback.php b/Sources/Maintenance/MigrationRollback.php index e141916068..ec5893c496 100644 --- a/Sources/Maintenance/MigrationRollback.php +++ b/Sources/Maintenance/MigrationRollback.php @@ -316,12 +316,44 @@ private function execute(string $sql): void // end the whole thing, which would leave a forum half put back. // Skipped is not the same as unnoticed, though: what did not run // is the difference between a rollback and the appearance of one. - if ($result === false) { + if ($result === false && !$this->positionSequence($statement)) { $this->failures[] = preg_replace('~\s+~', ' ', substr($statement, 0, 120)); } } } + /** + * Puts a sequence where a CREATE SEQUENCE would have started it. + * + * Dropping a table does not drop the sequence feeding it, so a sequence + * being put back is nearly always already there and asking for it again is + * refused. What the statement was for is the number it would have started + * at, and that can still be had. + * + * @param string $statement The statement that was refused. + * @return bool Whether this was a CREATE SEQUENCE that has now been dealt + * with another way. + */ + private function positionSequence(string $statement): bool + { + if (preg_match('~^CREATE SEQUENCE ([^\s]+) START WITH (\d+)~i', trim($statement), $match) !== 1) { + return false; + } + + // The third argument says the value has not been handed out yet, so + // the next id is the one the statement asked to start at. + $result = Db::$db->query( + 'SELECT setval({string:sequence}, {int:start}, false)', + [ + 'sequence' => $match[1], + 'start' => (int) $match[2], + 'db_error_skip' => true, + ], + ); + + return $result !== false; + } + /** * Splits SQL into the statements it is made of. * From 90da2952897c911e6f8a708c95e534328bd56fd1 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 19:27:46 +0200 Subject: [PATCH 13/22] Keeps the record of a run whole from the first write to the last Four things kept a run from describing the upgrade that made it. The list of tables to back up is indexed by substep, and array_filter leaves a hole where each name it dropped had been. The command line walks the dense list of substeps instead, so only the browser reached the gap, where it stopped mid backup on an undefined index. Config::getCurrentSettings() refuses a Settings.php touched since the request began. An upgrade writes that file more than once, so from the second write onwards it was reading its own work and giving up, and db_character_set and db_mb4 -- the two the rollback most needs -- were never written down. The run closed before that last write rather than after it, and the write then opened a run of its own to record settings into: a row with no backup behind it, left behind by every upgrade that finished. The run now opens where the admin presses Continue, so the settings that step changes have somewhere to go, and closes after the last thing the upgrade writes. Signed-off-by: albertlast --- Sources/Maintenance/Tools/Upgrade.php | 50 +++++++++++++++++++-------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index e59339184c..e2f3f9f057 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -927,6 +927,11 @@ public function upgradeOptions(): bool Db::load(); Db::$db->setSqlMode('strict'); + // The admin has pressed Continue, so the upgrade is underway and the + // run it belongs to starts here. Opening it before anything is written + // is what gives the settings this step changes somewhere to be recorded. + $this->getRunId(); + $file_settings = []; $db_settings = []; @@ -1094,10 +1099,12 @@ public function backupDatabase(): bool // of the record of what was copied is of no use to anybody putting a // forum back, and restoring it would put back an older account of the // run doing the restoring. - $table_names = array_filter($tables, function ($table) { + // array_values because the substep is used as an index into this list, + // and array_filter leaves a hole where each name it dropped had been. + $table_names = array_values(array_filter($tables, function ($table) { return !str_starts_with($table, 'backup_') && !str_starts_with($table, MigrationData::prefix() . 'migration_'); - }); + })); Maintenance::$total_substeps = \count($table_names); @@ -1253,11 +1260,6 @@ public function finalize(): bool // Update the database with the new SMF version. $this->updateModSettings(['smfVersion' => SMF_VERSION]); - // Closing the run goes with it. Whatever upgrades this forum next is a - // different one, and records what it finds rather than reading the - // notes this one left. - MigrationData::finishRun($this->getRunId(), SMF_VERSION); - // Clean any old cache files away. CacheApi::load(); CacheApi::clean(); @@ -1347,6 +1349,12 @@ public function finalize(): bool $this->updateSettingsFile($file_settings); + // The run closes after the last thing the upgrade writes, so that + // db_character_set and db_mb4 are recorded while it is still open. + // Whatever upgrades this forum next is a different run, and records + // what it finds rather than reading the notes this one left. + MigrationData::finishRun($this->getRunId(), SMF_VERSION); + // We're done! $this->logProgress(Lang::getTxt('log_upgrade_complete', file: 'Maintenance')); Maintenance::$overall_percent = 100; @@ -1488,13 +1496,21 @@ public function updateSettingsFile(array $config_vars, ?bool $keep_quotes = null */ private function recordSettings(array $names): void { - $run = $this->getRunId(); + // A run of its own is no use here. The settings are put back beside the + // tables the run copied, so one that copied nothing has nothing to put + // them back into, and the last thing an upgrade should leave behind is + // a run that was opened by the act of finishing. + $run = $this->getRunId(false); if ($run === '') { return; } - $current = Config::getCurrentSettings(); + // Read the file as it stands rather than as it stood when the request + // began. An upgrade writes Settings.php more than once, and the default + // refuses a file touched since TIME_START -- which, from the second + // write onwards, is a file this upgrade wrote itself. + $current = Config::getCurrentSettings(@filemtime(SMF_SETTINGS_FILE) ?: null); if (!\is_array($current)) { return; @@ -1619,7 +1635,7 @@ private function recordDefinition(string $table): void * * @return string The run's id. */ - private function getRunId(): string + private function getRunId(bool $start = true): string { if ($this->id_run !== '') { return $this->id_run; @@ -1629,15 +1645,19 @@ private function getRunId(): string return ''; } - $this->id_run = MigrationData::currentRun(); + $run = MigrationData::currentRun(); + + if ($run === '' && !$start) { + return ''; + } - if ($this->id_run === '') { - $this->id_run = (string) Uuid::create(); + if ($run === '') { + $run = (string) Uuid::create(); - MigrationData::startRun($this->id_run, $this->start_smf_version, $this->user['id'] ?? 0); + MigrationData::startRun($run, $this->start_smf_version, $this->user['id'] ?? 0); } - return $this->id_run; + return $this->id_run = $run; } /** From 30e0375c105502f2fb34f666b917c7aa4bd3b2c9 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 19:47:51 +0200 Subject: [PATCH 14/22] Covers the splitting a rollback depends on statements() is the only part of the rollback that needs no database, and it is the part where a mistake is silent: a function body split on the semicolons inside it becomes fragments that parse as nothing, and the index leaning on that function never comes back. Signed-off-by: albertlast --- tests/Unit/MigrationRollbackTest.php | 121 +++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 tests/Unit/MigrationRollbackTest.php diff --git a/tests/Unit/MigrationRollbackTest.php b/tests/Unit/MigrationRollbackTest.php new file mode 100644 index 0000000000..59d3889ee5 --- /dev/null +++ b/tests/Unit/MigrationRollbackTest.php @@ -0,0 +1,121 @@ +statements($sql); + + $this->assertCount(2, $statements); + $this->assertStringContainsString('EXTRACT(DAY FROM $1)', $statements[0]); + $this->assertStringStartsWith('CREATE INDEX', $statements[1]); + } + + // Note: the section banner above must not be the first thing in this group when + // the first member carries an attribute. The SMF/section_comments fixer inserts + // the banner between the attribute and its method, which is why the data provider + // case is second rather than first. + #[DataProvider('sqlProvider')] + public function testItSplitsOnlyTheSemicolonsThatEndAStatement(string $sql, array $expected): void + { + $this->assertSame($expected, $this->statements($sql)); + } + + /*********************** + * Public static methods + ***********************/ + + /** + * @return array}> + */ + public static function sqlProvider(): array + { + return [ + 'nothing at all' => ['', []], + 'semicolons alone' => [';;;', []], + 'one statement without a semicolon' => [ + 'CREATE TABLE smf_x (id int)', + ['CREATE TABLE smf_x (id int)'], + ], + 'a trailing semicolon adds no empty statement' => [ + 'CREATE TABLE smf_x (id int);', + ['CREATE TABLE smf_x (id int)'], + ], + 'whitespace between statements is dropped' => [ + "CREATE TABLE smf_x (id int);\n\n CREATE TABLE smf_y (id int);\n", + ['CREATE TABLE smf_x (id int)', 'CREATE TABLE smf_y (id int)'], + ], + 'a semicolon inside a single quoted default' => [ + "ALTER TABLE smf_x ALTER c SET DEFAULT 'a;b';ALTER TABLE smf_x ALTER d SET DEFAULT 'c'", + [ + "ALTER TABLE smf_x ALTER c SET DEFAULT 'a;b'", + "ALTER TABLE smf_x ALTER d SET DEFAULT 'c'", + ], + ], + 'a semicolon inside a double quoted identifier' => [ + 'CREATE TABLE "we;ird" (id int);CREATE TABLE smf_y (id int)', + ['CREATE TABLE "we;ird" (id int)', 'CREATE TABLE smf_y (id int)'], + ], + 'a semicolon inside a backquoted identifier' => [ + 'CREATE TABLE `we;ird` (id int);CREATE TABLE smf_y (id int)', + ['CREATE TABLE `we;ird` (id int)', 'CREATE TABLE smf_y (id int)'], + ], + 'an escaped quote does not end the string' => [ + "INSERT INTO smf_x VALUES ('it\\'s; fine');SELECT 1", + ["INSERT INTO smf_x VALUES ('it\\'s; fine')", 'SELECT 1'], + ], + 'a tagged dollar quote is closed by its own tag' => [ + 'CREATE FUNCTION f() RETURNS int AS $body$ SELECT 1; $body$ LANGUAGE SQL;SELECT 2', + [ + 'CREATE FUNCTION f() RETURNS int AS $body$ SELECT 1; $body$ LANGUAGE SQL', + 'SELECT 2', + ], + ], + 'a positional parameter is not a dollar quote' => [ + 'SELECT $1; SELECT $2', + ['SELECT $1', 'SELECT $2'], + ], + ]; + } + + /****************** + * Internal methods + ******************/ + + /** + * Calls the private helper under test. + * + * @param string $sql The SQL to split. + * @return array The statements it is made of. + */ + private function statements(string $sql): array + { + $method = new \ReflectionMethod(MigrationRollback::class, 'statements'); + + return $method->invoke(new MigrationRollback(), $sql); + } +} From dc97399cfa5c37d8cbbdff8cb7ff2d7029f4f09f Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 19:51:36 +0200 Subject: [PATCH 15/22] Takes back the routines an upgrade added A rollback put the tables back and left SMF 3.0's own functions behind, so a database calling itself 2.1.7 still held group_concat, its final function and migrate_inet. Every routine in the public schema is now named in the record, whether or not it could be written down, since the names are what says which ones the upgrade went on to add. Those are dropped once the tables are, aggregates before the plain functions they are built from. Signed-off-by: albertlast --- Sources/Maintenance/MigrationRollback.php | 62 ++++++++++++++++++++++- Sources/Maintenance/Tools/Upgrade.php | 26 +++++++++- 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/Sources/Maintenance/MigrationRollback.php b/Sources/Maintenance/MigrationRollback.php index ec5893c496..63bec63ec4 100644 --- a/Sources/Maintenance/MigrationRollback.php +++ b/Sources/Maintenance/MigrationRollback.php @@ -126,6 +126,13 @@ public function rollback(string $run): bool $this->log[] = 'dropped ' . $table; } + // After the tables, since a table the upgrade added can have an index + // built over a function it added alongside it. + foreach ($this->addedRoutines($run) as $name => $sql) { + $this->execute($sql); + $this->log[] = 'dropped function ' . $name; + } + Db::$db->disableQueryCheck = $checking; $this->restoreSettings($run); @@ -179,7 +186,60 @@ public function candidates(): array */ private function routines(string $run): array { - return MigrationData::all($run, MigrationData::TYPE_ROUTINE); + // A routine recorded by name alone is one the run found but could not + // write down. Its name still counts as having been there, which is + // what keeps it off the list of things the upgrade added, but there is + // no SQL to put back. + return array_filter(MigrationData::all($run, MigrationData::TYPE_ROUTINE)); + } + + /** + * The routines the upgrade added, newest kind first. + * + * An aggregate is dropped before the plain functions are, since it is + * built out of one of them and PostgreSQL will not let the parts go while + * something is made of them. + * + * @param string $run The run. + * @return array The DROP statements, keyed by signature. + */ + private function addedRoutines(string $run): array + { + if (Db::$db->title !== POSTGRE_TITLE) { + return []; + } + + $recorded = MigrationData::all($run, MigrationData::TYPE_ROUTINE); + + if ($recorded === []) { + return []; + } + + $drops = []; + + $request = Db::$db->query( + 'SELECT p.oid::regprocedure AS signature, p.prokind + FROM pg_proc AS p + INNER JOIN pg_namespace AS n ON (n.oid = p.pronamespace) + WHERE n.nspname = {string:schema} + ORDER BY p.prokind = {string:plain}, signature', + [ + 'schema' => 'public', + 'plain' => 'f', + ], + ); + + while ($row = Db::$db->fetch_assoc($request)) { + if (isset($recorded[$row['signature']])) { + continue; + } + + $drops[$row['signature']] = 'DROP ' . ($row['prokind'] === 'a' ? 'AGGREGATE' : 'FUNCTION') . ' ' . $row['signature']; + } + + Db::$db->free_result($request); + + return $drops; } /** diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index e2f3f9f057..5d28f12e6b 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -1564,8 +1564,32 @@ private function recordRoutines(): void return; } + // Every routine is named, because the names are what says which ones + // the upgrade went on to add. Only a plain function can be written + // down though: pg_get_functiondef() refuses an aggregate, and an + // aggregate SMF did not create is one it has no business rebuilding. + $definitions = []; + $request = Db::$db->query( 'SELECT p.oid::regprocedure AS signature, pg_get_functiondef(p.oid) AS definition + FROM pg_proc AS p + INNER JOIN pg_namespace AS n ON (n.oid = p.pronamespace) + WHERE n.nspname = {string:schema} + AND p.prokind = {string:plain}', + [ + 'schema' => 'public', + 'plain' => 'f', + ], + ); + + while ($row = Db::$db->fetch_assoc($request)) { + $definitions[$row['signature']] = $row['definition']; + } + + Db::$db->free_result($request); + + $request = Db::$db->query( + 'SELECT p.oid::regprocedure AS signature FROM pg_proc AS p INNER JOIN pg_namespace AS n ON (n.oid = p.pronamespace) WHERE n.nspname = {string:schema} @@ -1581,7 +1605,7 @@ private function recordRoutines(): void static::class, MigrationData::TYPE_ROUTINE, $row['signature'], - $row['definition'], + $definitions[$row['signature']] ?? '', ); } From d47145e07dc1539249b2c809b6ef5952bf4f7f8c Mon Sep 17 00:00:00 2001 From: albertlast Date: Mon, 7 Sep 2026 06:16:18 +0200 Subject: [PATCH 16/22] Puts the docblock for id_run above id_run It was left behind above UNRECORDED_SETTINGS, which has a docblock of its own, so the constant carried two and the property carried none. Signed-off-by: albertlast --- Sources/Maintenance/Tools/Upgrade.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 5d28f12e6b..72def7eb8f 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -206,15 +206,6 @@ class Upgrade extends ToolsBase implements ToolsInterface ], ]; - /** - * @var string - * - * Identifies this upgrade, and stays the same when it is started again - * after being interrupted. What a migration records against it therefore - * describes the database as this upgrade found it, not as a later attempt - * found it half changed. Read through getRunId(), which knows where it - * lives. - */ /** * @var array * @@ -361,6 +352,15 @@ class Upgrade extends ToolsBase implements ToolsInterface */ protected string $start_smf_version = ''; + /** + * @var string + * + * Identifies this upgrade, and stays the same when it is started again + * after being interrupted. What a migration records against it therefore + * describes the database as this upgrade found it, not as a later attempt + * found it half changed. Read through getRunId(), which knows where it + * lives. + */ protected string $id_run = ''; /** From 8e0e139585540a00027595c8145b0d534f809c3d Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 16:42:30 +0200 Subject: [PATCH 17/22] Offers to put the database back when an upgrade did not finish An upgrade that stopped part way leaves an admin in front of a forum that is neither one version nor the other, with two ways out: carry on, or put things back. Only the second was missing. The offer is made on the options page, and only for a run that stopped part way and took a backup, since nothing else has the rows to put back. An upgrade that finished is not offered: undoing a working forum is something to go looking for, not something to be suggested. On the command line the same thing is asked for with --rollback. Signed-off-by: albertlast --- Languages/en_US/Maintenance.php | 8 ++++ Sources/Maintenance/MigrationRollback.php | 30 ++++++++++++++ Sources/Maintenance/Tools/Upgrade.php | 48 +++++++++++++++++++++++ Themes/default/UpgradeTemplate.php | 23 +++++++++++ 4 files changed, 109 insertions(+) diff --git a/Languages/en_US/Maintenance.php b/Languages/en_US/Maintenance.php index 21dceeb9f5..fd8e7d0a9d 100644 --- a/Languages/en_US/Maintenance.php +++ b/Languages/en_US/Maintenance.php @@ -405,6 +405,14 @@ // Upgrade options $txt['upgrade_areyouready'] = 'Before the upgrade gets underway, please review the options below and press "Continue" when you are ready to begin.'; $txt['upgrade_backup_table'] = 'Backup SMF tables in your database using the prefix {0}'; +$txt['upgrade_rollback_title'] = 'An unfinished upgrade'; +$txt['upgrade_rollback_offer'] = 'An upgrade from {version} was started on {date} and did not finish. You can carry on with it, or put the database back the way it was before it started. Only the database is put back: the files on disk are left alone.'; +$txt['upgrade_rollback_button'] = 'Put the database back'; +$txt['upgrade_rollback_done'] = 'The database has been put back the way it was before the upgrade started.'; +$txt['log_rollback_starting'] = 'Putting the database back to {version}'; +$txt['log_rollback_done'] = 'Put back {count} things.'; +$txt['log_rollback_failed'] = 'Nothing was put back: {error}'; +$txt['log_rollback_refused'] = 'The database refused: {statement}'; $txt['upgrade_maintenance'] = 'Put the forum into maintenance mode during upgrade.'; $txt['upgrade_maintenance_title'] = 'Maintenance Title:'; $txt['upgrade_maintenance_message'] = 'Maintenance Message:'; diff --git a/Sources/Maintenance/MigrationRollback.php b/Sources/Maintenance/MigrationRollback.php index 63bec63ec4..e44db6fcc8 100644 --- a/Sources/Maintenance/MigrationRollback.php +++ b/Sources/Maintenance/MigrationRollback.php @@ -174,6 +174,36 @@ public function candidates(): array return $runs; } + /** + * The run worth offering to undo, if there is one. + * + * An upgrade that finished is not offered: putting a working forum back is + * something an admin should have to go looking for, not something the + * upgrader suggests. One that stopped part way is the other case entirely, + * and the admin standing in front of it has two ways out -- carry on, or + * put things back as they were. + * + * Only a run that took a backup can be offered, since nothing else has the + * rows to put back. + * + * @return array|null The run, or null if there is nothing to offer. + */ + public function unfinished(): ?array + { + foreach ($this->candidates() as $run) { + if ( + (int) $run['time_finished'] !== 0 + || MigrationData::all($run['id_run'], MigrationData::TYPE_BACKUP) === [] + ) { + continue; + } + + return $run; + } + + return null; + } + /****************** * Internal methods ******************/ diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 72def7eb8f..5086097777 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -26,6 +26,7 @@ use SMF\Maintenance\Maintenance; use SMF\Maintenance\Migration; use SMF\Maintenance\MigrationData; +use SMF\Maintenance\MigrationRollback; use SMF\Maintenance\Step; use SMF\Maintenance\Utf8ConverterStep; use SMF\QueryString; @@ -913,6 +914,18 @@ public function upgradeOptions(): bool Utils::$context['sm_stats_configured'] = !empty(Config::$modSettings['allow_sm_stats']) || !empty(Config::$modSettings['enable_sm_stats']); + // An upgrade that stopped part way leaves the admin with two ways out. + // Carrying on is the one the rest of this page is about; putting the + // database back as it was is the other, and is only worth offering when + // there is a backup to put back. + $rollback = new MigrationRollback(); + + Utils::$context['rollback_offer'] = $rollback->unfinished(); + + if (!empty($_POST['rollback']) && Utils::$context['rollback_offer'] !== null) { + return $this->rollBackUpgrade($rollback, Utils::$context['rollback_offer']); + } + // If we've not submitted then we're done. if (!Sapi::isCLI() && empty($_POST['upcont'])) { Utils::$context['continue'] = true; @@ -1478,6 +1491,41 @@ public function updateSettingsFile(array $config_vars, ?bool $keep_quotes = null * Internal methods ******************/ + /** + * Undoes an upgrade that stopped part way, and stops. + * + * Whatever happens, this does not carry on into the rest of the upgrade. + * An admin who asked for the database to be put back did not ask for it to + * be upgraded again straight afterwards. + * + * @param MigrationRollback $rollback The thing that does the work. + * @param array $run The run being undone. + * @return bool Always false, since the upgrade is not going any further. + */ + private function rollBackUpgrade(MigrationRollback $rollback, array $run): bool + { + $this->logProgress(Lang::getTxt('log_rollback_starting', ['version' => $run['version_from']], file: 'Maintenance')); + + Db::load(); + + if (!$rollback->rollback($run['id_run'])) { + Maintenance::$fatal_error = Lang::getTxt('log_rollback_failed', ['error' => $rollback->error], file: 'Maintenance'); + + return false; + } + + foreach ($rollback->failures as $failure) { + Maintenance::$warnings[] = Lang::getTxt('log_rollback_refused', ['statement' => $failure], file: 'Maintenance'); + } + + $this->logProgress(Lang::getTxt('log_rollback_done', ['count' => \count($rollback->log)], file: 'Maintenance')); + + Utils::$context['rollback_done'] = true; + Utils::$context['continue'] = false; + + return false; + } + /** * Records what the settings being written held beforehand. * diff --git a/Themes/default/UpgradeTemplate.php b/Themes/default/UpgradeTemplate.php index 93350854ca..cb03965102 100644 --- a/Themes/default/UpgradeTemplate.php +++ b/Themes/default/UpgradeTemplate.php @@ -19,6 +19,7 @@ use SMF\Lang; use SMF\Maintenance\Maintenance; use SMF\Sapi; +use SMF\Time; use SMF\Utils; /** @@ -306,6 +307,28 @@ public static function upgradeOptions(): void return; } + // An upgrade that stopped part way is the only time putting the database + // back is offered. The admin is standing in front of a half upgraded + // forum and has two ways out of it. + if (!empty(Utils::$context['rollback_done'])) { + echo ' +
', Lang::getTxt('upgrade_rollback_done', file: 'Maintenance'), '
'; + + return; + } + + if (!empty(Utils::$context['rollback_offer'])) { + echo ' +
+ ', Lang::getTxt('upgrade_rollback_title', file: 'Maintenance'), '
+ ', Lang::getTxt('upgrade_rollback_offer', [ + 'version' => Utils::$context['rollback_offer']['version_from'], + 'date' => Time::create('@' . Utils::$context['rollback_offer']['time_started'])->format(), + ], file: 'Maintenance'), '
+ +
'; + } + echo '
From 8a84d2781a19bf556a68f2032c017e5a239266f2 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 16:55:13 +0200 Subject: [PATCH 18/22] Adds a way to exercise the offer to put the database back Upgrades a baseline, marks the run as one that did not finish, and asks the upgrader for --rollback. Timing a kill so that it lands after the backup and before the end is a race, and what is being checked here is the offer rather than the interruption, which interrupt-upgrade.sh already covers. Signed-off-by: albertlast --- .docker/try-rollback-offer.sh | 77 +++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 .docker/try-rollback-offer.sh diff --git a/.docker/try-rollback-offer.sh b/.docker/try-rollback-offer.sh new file mode 100644 index 0000000000..4167d18326 --- /dev/null +++ b/.docker/try-rollback-offer.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# Upgrades a 2.1 database, marks the run as one that did not finish, and then +# asks the upgrader to put the database back with --rollback. +# +# .docker/try-rollback-offer.sh --engine postgresql --baseline ../SMF-2.1/.docker/baseline/artifacts/2.1.7-1/small/postgres.sql +# +# This exists to exercise the offer the upgrader makes when it finds a run that +# stopped part way. Killing a real upgrade at the right moment is a race, and +# what is being tested here is the offer rather than the interruption, which is +# covered by interrupt-upgrade.sh. +set -euo pipefail + +. "$(dirname -- "${BASH_SOURCE[0]}")/lib.sh" +. "$(dirname -- "${BASH_SOURCE[0]}")/upgrade-readings.sh" + +ENGINE='' +BASELINE='' +OUT="$DOCKER_DIR/offer" + +while [ $# -gt 0 ]; do + case "$1" in + --engine) ENGINE="$2"; shift 2 ;; + --baseline) BASELINE="$2"; shift 2 ;; + --out) OUT="$2"; shift 2 ;; + *) die "unknown argument: $1" ;; + esac +done + +[ -n "$ENGINE" ] || die 'need --engine' +[ -n "$BASELINE" ] || die 'need --baseline' + +cd "$BOARD_DIR" +mkdir -p "$OUT" +OUT=$(cd -- "$OUT" && pwd) + +log "${ENGINE}: emptying the database" +"$DOCKER_DIR/reset.sh" --engine "$ENGINE" >/dev/null + +log "${ENGINE}: loading $(basename -- "$BASELINE")" +load_baseline "$ENGINE" "$BASELINE" + +log "${ENGINE}: upgrading" +UPGRADE_ARGS='--backup' run_upgrade "$ENGINE" "$OUT/upgrade-${ENGINE}.log" \ + || die "${ENGINE}: the upgrade failed -- $OUT/upgrade-${ENGINE}.log" + +log "${ENGINE}: upgraded to SMF $(installed_version "$ENGINE")" +snapshot "$ENGINE" upgraded "$OUT" + +# What a killed process leaves behind is a run with no finishing time on it. +# Setting that here is the difference between testing the offer and testing +# how well a kill can be timed. +log "${ENGINE}: marking the run as one that did not finish" + +if [ "$ENGINE" = 'mysql' ]; then + docker compose exec -T -e MYSQL_PWD="$DB_PASSWORD" mysql \ + mysql -u"$DB_USER" -D "$DB_NAME" -e "UPDATE ${DB_PREFIX}migration_runs SET time_finished = 0;" +else + docker compose exec -T postgres \ + psql -q -U "$DB_USER" -d "$DB_NAME" -c "UPDATE ${DB_PREFIX}migration_runs SET time_finished = 0;" +fi + +log "${ENGINE}: asking the upgrader to put it back" +rm -f "$BOARD_DIR/install.php" +cp "$BOARD_DIR/other/upgrade.php" "$BOARD_DIR/upgrade.php" + +status=0 +docker compose exec -T web php upgrade.php --rollback > "$OUT/rollback-${ENGINE}.log" 2>&1 || status=$? + +rm -f "$BOARD_DIR/upgrade.php" + +printf '\n' +cat "$OUT/rollback-${ENGINE}.log" +printf '\n' + +log "${ENGINE}: the forum is now SMF $(installed_version "$ENGINE")" + +exit "$status" From 3e33b26e029eaf4745a6a778b8a3e677ce726525 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 17:18:20 +0200 Subject: [PATCH 19/22] Starts a new run when upgrading again after a rollback A run that has been undone was still being treated as one under way, so upgrading again picked it up where it left off: the tables it had already backed up were passed over, and the definitions describing the database it had already put back were kept. The second attempt ran with no backup of its own at all. Being undone finishes a run whatever its finishing time says. Signed-off-by: albertlast --- Sources/Maintenance/MigrationData.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index 6d97fb4046..dc01a6116d 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -214,6 +214,12 @@ public static function forget(string $run, string $type, string $key): void * this is asked of the database rather than of the progress data in * Settings.php, which the command line never writes. * + * A run that has been undone is finished business whatever its finishing + * time says. Upgrading again after a rollback is a new attempt on a + * database that has been put back, and it needs a backup and a set of + * definitions of its own rather than the ones describing a state that has + * already been restored. + * * @return string The run's id, or an empty string if none is open. */ public static function currentRun(): string @@ -226,10 +232,12 @@ public static function currentRun(): string 'SELECT id_run FROM {db_prefix}migration_runs WHERE time_finished = {int:unfinished} + AND time_rolled_back = {int:not_undone} ORDER BY time_started DESC LIMIT 1', [ 'unfinished' => 0, + 'not_undone' => 0, ], ); From ff68507dca6a22ed970e9d87df16845bd3f7c2e5 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 17:24:09 +0200 Subject: [PATCH 20/22] Leaves nothing behind that would misdirect the next upgrade The rollback runs inside the upgrader, which started while the forum was on the version it has just been taken off. Stopping at step two, that process wrote its progress data out saying so, and the next upgrade read it, believed the work was done and skipped every v2_1 migration on a database that had just been put back to 2.1. It still said the upgrade was complete: the indexes kept their old names and twelve tables were left as 2.1 had them. Asking for the rollback also opened a run of its own, since writing any setting asks which run is under way. It copied nothing, and left open the next upgrade would take it up as unfinished work and pass over the backup it never made. Signed-off-by: albertlast --- Sources/Maintenance/MigrationData.php | 27 +++++++++++++++++++++++++++ Sources/Maintenance/Tools/Upgrade.php | 12 ++++++++++++ 2 files changed, 39 insertions(+) diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index dc01a6116d..149612a2ac 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -340,6 +340,33 @@ public static function finishRun(string $run, string $to): void ); } + /** + * Throws away runs that recorded nothing. + * + * Asking the upgrader to put a database back opens a run of its own, since + * writing any setting asks which run is under way. That run copies nothing + * and describes nothing, and leaving it open would have the next upgrade + * take it up as unfinished work and skip the backup it never made. + */ + public static function discardEmptyRuns(): void + { + if (!self::exists()) { + return; + } + + Db::$db->query( + 'DELETE FROM {db_prefix}migration_runs + WHERE time_finished = {int:unfinished} + AND id_run NOT IN ( + SELECT id_run + FROM {db_prefix}migration_data + )', + [ + 'unfinished' => 0, + ], + ); + } + /** * Notes that a run has been undone. * diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index 5086097777..cdddc19ab7 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -1520,6 +1520,18 @@ private function rollBackUpgrade(MigrationRollback $rollback, array $run): bool $this->logProgress(Lang::getTxt('log_rollback_done', ['count' => \count($rollback->log)], file: 'Maintenance')); + // This process started while the forum was on the version it has just + // been taken off, and the progress data would say so on the way out. + // The next upgrade would read that, believe the work was already done + // and skip the migrations the database now needs again. + $this->start_smf_version = str_replace(' ', '.', strtolower((string) (Config::$modSettings['smfVersion'] ?? $this->start_smf_version))); + + $this->updateSettingsFile(['maintenance_tool_progress' => '']); + + // Asking for a rollback opened a run of its own, which copied nothing. + // Left open, the next upgrade would take it up as unfinished work. + MigrationData::discardEmptyRuns(); + Utils::$context['rollback_done'] = true; Utils::$context['continue'] = false; From 0674f6ed16cd462be2589f37b90ed93e533c12aa Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 17:28:21 +0200 Subject: [PATCH 21/22] Takes the version to go back to from the run, not from memory Config::$modSettings was read before the rollback and still names the version that has just gone, so the progress data was written out saying the forum was on 3.0 when it had been put back to 2.1, and the next upgrade skipped every v2_1 migration. The run itself knows what the forum was on before it touched anything, which is what it is on again. A run that never reached the backup step describes no table, so noting a setting or two on the way does not make it one worth keeping. Signed-off-by: albertlast --- Sources/Maintenance/MigrationData.php | 8 ++++++-- Sources/Maintenance/Tools/Upgrade.php | 7 +++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/Sources/Maintenance/MigrationData.php b/Sources/Maintenance/MigrationData.php index 149612a2ac..6795a3ab14 100644 --- a/Sources/Maintenance/MigrationData.php +++ b/Sources/Maintenance/MigrationData.php @@ -345,8 +345,10 @@ public static function finishRun(string $run, string $to): void * * Asking the upgrader to put a database back opens a run of its own, since * writing any setting asks which run is under way. That run copies nothing - * and describes nothing, and leaving it open would have the next upgrade - * take it up as unfinished work and skip the backup it never made. + * never reaches the backup step, so it describes no table, and leaving it + * open would have the next upgrade take it up as unfinished work and pass + * over the backup it never made. Having noted a setting or two on the way + * is not enough to make it a run worth keeping. */ public static function discardEmptyRuns(): void { @@ -360,9 +362,11 @@ public static function discardEmptyRuns(): void AND id_run NOT IN ( SELECT id_run FROM {db_prefix}migration_data + WHERE data_type = {string:definition} )', [ 'unfinished' => 0, + 'definition' => self::TYPE_DEFINITION, ], ); } diff --git a/Sources/Maintenance/Tools/Upgrade.php b/Sources/Maintenance/Tools/Upgrade.php index cdddc19ab7..160001fb6c 100644 --- a/Sources/Maintenance/Tools/Upgrade.php +++ b/Sources/Maintenance/Tools/Upgrade.php @@ -1523,8 +1523,11 @@ private function rollBackUpgrade(MigrationRollback $rollback, array $run): bool // This process started while the forum was on the version it has just // been taken off, and the progress data would say so on the way out. // The next upgrade would read that, believe the work was already done - // and skip the migrations the database now needs again. - $this->start_smf_version = str_replace(' ', '.', strtolower((string) (Config::$modSettings['smfVersion'] ?? $this->start_smf_version))); + // and skip the migrations the database now needs again. The run knows + // what the forum was on before it touched anything, which is what it is + // on again now; the copy in Config::$modSettings was read before the + // rollback and still names the version that has just gone. + $this->start_smf_version = str_replace(' ', '.', strtolower((string) $run['version_from'])); $this->updateSettingsFile(['maintenance_tool_progress' => '']); From 043862d508ff4c67eaa4f54152ba0211323b8258 Mon Sep 17 00:00:00 2001 From: albertlast Date: Sun, 6 Sep 2026 19:28:02 +0200 Subject: [PATCH 22/22] Says what to do next once the database is back The database is 2.1 again and the files on disk are still 3.0, so the forum will not run until one of them is changed. The notice that the rollback had happened said none of this and offered nowhere to go. Signed-off-by: albertlast --- Languages/en_US/Maintenance.php | 4 ++-- Themes/default/UpgradeTemplate.php | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Languages/en_US/Maintenance.php b/Languages/en_US/Maintenance.php index fd8e7d0a9d..9487d382c9 100644 --- a/Languages/en_US/Maintenance.php +++ b/Languages/en_US/Maintenance.php @@ -406,9 +406,9 @@ $txt['upgrade_areyouready'] = 'Before the upgrade gets underway, please review the options below and press "Continue" when you are ready to begin.'; $txt['upgrade_backup_table'] = 'Backup SMF tables in your database using the prefix {0}'; $txt['upgrade_rollback_title'] = 'An unfinished upgrade'; -$txt['upgrade_rollback_offer'] = 'An upgrade from {version} was started on {date} and did not finish. You can carry on with it, or put the database back the way it was before it started. Only the database is put back: the files on disk are left alone.'; +$txt['upgrade_rollback_offer'] = 'An upgrade from {version} was started {date} and did not finish. You can carry on with it, or put the database back the way it was before it started. Only the database is put back: the files on disk are left alone.'; $txt['upgrade_rollback_button'] = 'Put the database back'; -$txt['upgrade_rollback_done'] = 'The database has been put back the way it was before the upgrade started.'; +$txt['upgrade_rollback_done'] = 'The database has been put back the way it was before the upgrade started. The files on disk are still {version}, so the forum will not run until you either start the upgrade again or put your old files back from a backup.'; $txt['log_rollback_starting'] = 'Putting the database back to {version}'; $txt['log_rollback_done'] = 'Put back {count} things.'; $txt['log_rollback_failed'] = 'Nothing was put back: {error}'; diff --git a/Themes/default/UpgradeTemplate.php b/Themes/default/UpgradeTemplate.php index cb03965102..bb2c32c7bc 100644 --- a/Themes/default/UpgradeTemplate.php +++ b/Themes/default/UpgradeTemplate.php @@ -312,7 +312,10 @@ public static function upgradeOptions(): void // forum and has two ways out of it. if (!empty(Utils::$context['rollback_done'])) { echo ' -
', Lang::getTxt('upgrade_rollback_done', file: 'Maintenance'), '
'; +
', Lang::getTxt('upgrade_rollback_done', [ + 'version' => SMF_FULL_VERSION, + 'url' => Maintenance::getSelf(), + ], file: 'Maintenance'), '
'; return; }