Skip to content

[3.0] Records what an upgrade found, so it can be put back (part 1 of 2) - #9656

Open
albertlast wants to merge 16 commits into
SimpleMachines:release-3.0from
albertlast:3.0/migration-data-table
Open

[3.0] Records what an upgrade found, so it can be put back (part 1 of 2)#9656
albertlast wants to merge 16 commits into
SimpleMachines:release-3.0from
albertlast:3.0/migration-data-table

Conversation

@albertlast

@albertlast albertlast commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Description

An upgrade that stops part way leaves a database that is neither the version it
was nor the version it was going to be. The upgrader already copies every table
into backup_smf_* before it starts, but a copy of the rows is not enough to put
a forum back: nothing records what shape those rows were in, which functions the
indexes over them call, or what the settings said beforehand.

This is the record. It is part 1 of 2 — this part writes everything down and can
put it back; [part 2] offers that to the admin and adds --rollback.

Two tables

migration_runs is one row per upgrade: which version it started from, which it
reached, where it got to, who ran it, and when it started, finished or was undone.
migration_data hangs off it, holding four kinds of thing:

data_type one row per holds
definition table the SQL that rebuilds it, from table_sql()
backup table that a copy was taken, and when
routine PostgreSQL routine its definition, or its name alone for an aggregate
setting setting in Settings.php what it held before the upgrade changed it

Several runs can sit side by side, so a forum upgraded, put back and upgraded again
keeps an honest account of each attempt rather than overwriting the first.

What is deliberately not recorded

Settings.php holds db_passwd. A copy of it inside the database would be in
every dump taken from then on, so db_passwd, db_user, image_proxy_secret,
auth_secret and maintenance_tool_progress are excluded.

Putting it back

MigrationRollback replays the routines, then each table's definition, refills it
from its backup, drops the tables the upgrade added, drops the routines it added
(aggregates before the plain functions they are built from), puts the settings back
and stamps the run as undone. The upgrader's own two tables are excluded everywhere,
since a rollback that took them with it could not record that it had happened.

Nothing outside the database is touched. A forum that has been put back still has
the newer code on disk.

Verified

Full 2.1.7 -> 3.0 -> rollback -> 3.0 cycles on MySQL 8.4 and PostgreSQL 17,
from the 2.1.7 baseline forum, with the database compared against a single clean
upgrade each time:

  • MySQL: 72 tables, 0 differing, once an explicit ROW_FORMAT=DYNAMIC clause
    restating the server default is set aside.
  • PostgreSQL: the rollback leaves exactly the 17 routines a pristine 2.1.7 has,
    and the re-upgrade restores all 20.
  • Each run records its own 73 backups, 73 definitions and 7 settings, plus 17 or 19
    routines on PostgreSQL. Both runs report version_from = 2.1.7.
  • Also driven end to end through a browser, since the upgrader runs one step per
    request there and the command line does not.
Tests

statements() is the only part of the rollback that needs no database, and it is
the part where a mistake is silent: a PostgreSQL function body split on the
semicolons inside it becomes fragments that parse as nothing, and the index leaning
on that function never comes back. tests/Unit/MigrationRollbackTest.php covers it
through ReflectionMethod — dollar quoting, escaped quotes, backticks and
positional parameters. The rest needs Db::$db and is out of the suite's reach.

Depends on

table_sql() is what records a table's shape, and it has four bugs that stop it
round-tripping. All four are open separately and this does not work without them:

Known limits
  • Table::exists() compares a database-qualified prefix against bare table names,
    so it is always false on MySQL when the prefix is `smf`.smf_. MigrationData::prefix()
    works around it here; the underlying bug is elsewhere and is not fixed by this.
  • Only the database is put back. The files on disk are the admin's problem.

Issues References (Fixes|Related|Closes)

@albertlast
Keeps what a table looked like before the migrations
e04fe3c 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Copies rows into a backup without copying its source's sequence
cbba1eb 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Keeps the run's identity where a killed process cannot take it
9e65c10 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Gives each upgrade a row of its own
c65554b 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Leaves a backup this run already took alone
2f94acb 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Records the functions a table's definition leans on
8d29787 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Notes what a setting held before the upgrade changed it
3ddd6ac 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Puts a database back the way an upgrade found it
feb383e 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Lets the recorded SQL past the checks meant for assembled queries
fa71362 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Finds the tables when the prefix names the database too
e8d9070 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Reports what a rollback could not run, and finds the tables to run it on
ca0ba28 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Puts a sequence where recreating it would have started it
f241e1b 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Keeps the record of a run whole from the first write to the last
90da295 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Covers the splitting a rollback depends on
30e0375 
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 <mathiaspapealbert@hotmail.com>
@albertlast
Takes back the routines an upgrade added
dc97399 
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 <mathiaspapealbert@hotmail.com>
@albertlast albertlast mentioned this pull request Sep 6, 2026
Open
live627
live627 reviewed Sep 6, 2026
View reviewed changes
Comment thread Sources/Maintenance/Tools/Upgrade.php
@albertlast
Puts the docblock for id_run above id_run
d47145e 
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 <mathiaspapealbert@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@live627 live627 live627 left review comments

Assignees

No one assigned

Labels

Database Installer PostgreSQL Unit Testing Upgrader

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@albertlast @live627