fix(db): don't use STR_TO_DATE in a generated column (MariaDB 12.3)#3879
Conversation
MariaDB 12.3 enforces that STR_TO_DATE (which reads lc_time_names) cannot appear in a GENERATED ALWAYS AS clause, so migration 0098 failed with error 1901 and RomM could not start. Earlier releases up to 12.2 accepted it, which is why the problem only surfaced on 12.3. The gamelist value is already pinned to a fixed-width "YYYYMMDDThhmmss" by the regex guard, so it is reshaped into a canonical datetime literal with SUBSTRING/CONCAT and fed to the existing TIMESTAMPDIFF. Stored values and their time-zone independence are unchanged. Fixes #3873 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR makes the MariaDB release-date generated column compatible with MariaDB 12.3. The main changes are:
Confidence Score: 4/5The calendar-invalid input path needs a guard before merging.
backend/alembic/versions/0098_generated_metadata_columns.py Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
backend/alembic/versions/0098_generated_metadata_columns.py:137
**Invalid Dates Can Abort Writes**
The regex accepts calendar-invalid values such as `20201301T000000`, which this expression reshapes into an invalid datetime string. On MariaDB configurations that reject invalid implicit datetime conversions, evaluating the stored column can abort the migration for an existing row or reject a later insert instead of producing `NULL`; validate the calendar components before passing the value to `TIMESTAMPDIFF`.
Reviews (1): Last reviewed commit: "fix(db): don't use STR_TO_DATE in a gene..." | Re-trigger Greptile |
The 8+6 digit regex still admits values like 20201301T000000, and feeding one to TIMESTAMPDIFF raises "Incorrect datetime value" under the default STRICT_TRANS_TABLES, aborting the migration on an existing row or a later insert. CAST(... AS DATETIME), which would yield NULL, is itself barred from a generated column, so the components are range-checked (including leap years) in the CASE guard and invalid dates fall through to NULL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I know we basically already talked about this. But figured I'd throw my findings below for an automatic upgrade path for the few users who moved to alpha.1 and are currently on MariaDB v11 but may migrate to v12 in the future. Anyways, here's Claude lol. Fix looks correct, and I like that the One thing worth considering though: the edit only helps installs that haven't run 0098 yet. A DB that already ran the original 0098 on MariaDB < 12.3 keeps the def upgrade() -> None:
connection = op.get_bind()
if is_postgresql(connection):
return # PostgreSQL never used STR_TO_DATE
# information_schema reads don't open the table, so this is safe even on a
# 12.3 server where the table itself is unopenable.
current = connection.execute(
sa.text(
"SELECT GENERATION_EXPRESSION FROM information_schema.COLUMNS "
"WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = 'roms' "
"AND COLUMN_NAME = 'generated_first_release_date'"
)
).scalar()
if not current or "str_to_date" not in current.lower():
return # already portable (fresh install / fixed 0098) -> no-op
# Same expression 0098 now builds; MODIFY keeps the index in place.
expr = _maria_first_release_date()
op.execute(
"ALTER TABLE roms MODIFY COLUMN generated_first_release_date "
f"BIGINT GENERATED ALWAYS AS ({expr}) STORED"
)Key properties, all verified against real MariaDB containers:
Totally fine to leave it out for the alpha if the still-on-11.x population isn't worth the extra migration; just flagging the option and the exact shape it would take if we do want it. |
Description
Migration
0098_generated_metadata_columnsparsed the gamelistYYYYMMDDThhmmssdate withSTR_TO_DATEinside aGENERATED ALWAYS AS ... STOREDcolumn. MariaDB treatsSTR_TO_DATEas non-deterministic (it readslc_time_names) and forbids it there, so the migration aborts with error 1901 and RomM never starts.The regex guard already pins that value to a fixed-width
YYYYMMDDThhmmss, so no parsing function is needed: the string is reshaped into a canonicalYYYY-MM-DD hh:mm:ssliteral withSUBSTRING/CONCATand handed to the existingTIMESTAMPDIFF(SECOND, '1970-01-01 00:00:00', ...). Stored values and their session-time-zone independence are unchanged. PostgreSQL is untouched.Why this only hit some users
The restriction is only enforced starting in MariaDB 12.3, which is why the alpha migrated fine for most people. Tested against the original expression:
STR_TO_DATEvcolHeads-up for anyone already on 5.1.0-alpha.1
The check runs every time MariaDB opens the table, not just at DDL time. So a
romstable created by the original 0098 on MariaDB < 12.3 keeps working on that server, but becomes completely unopenable the moment MariaDB itself is upgraded to 12.3+:Even
ALTERandDROP COLUMNfail, so it cannot be repaired in place once the server is on 12.3. This PR fixes fresh installs only; existing databases need the manual fix below (or a backup restore). We deliberately skipped shipping a repair migration since the release is in alpha and users have backups.MANUAL FIX for existing databases (click to expand)
Take a backup first. This only applies to MariaDB/MySQL; PostgreSQL is unaffected.
Case 1 - still on MariaDB < 12.3 (RomM is running fine). Do this before you ever upgrade MariaDB. Run against your RomM database:
With Docker Compose:
Verify with
CHECK TABLE roms;- it should reportOK.This ALTER is a full table rebuild - plan for it
Changing a STORED generated column's expression can only run with
ALGORITHM=COPY;INSTANT,NOCOPYandINPLACEare all rejected by MariaDB, so there is no online variant and the table is write-locked for the duration. Cost scales with the total size ofroms(the metadata JSON blobs dominate), because every generated column is re-evaluated for every row.Measured on MariaDB 11.3.2 in a container: ~4s for 131k rows / 47MB, ~35s for 1M rows / 273MB. A large library with big metadata blobs can take considerably longer.
Things that do not help (measured, no meaningful difference):
SET GLOBAL innodb_flush_log_at_trx_commit=0,SET SESSION sql_log_bin=0, or droppingidx_roms_generated_first_release_datebeforehand. It is CPU-bound on JSON re-evaluation, not IO-bound.What actually helps:
ALTERbefore it even starts copying, and its writes will block once it does.You should see
copy to tmp tablewithpct_in_stageclimbing. Let it finish - interrupting it leaves the original table intact but wastes the work.Case 2 - already upgraded to MariaDB 12.3+ and RomM won't start. The table cannot be altered in this state, so roll MariaDB back first:
mariadb:12.2) and start only the database.ALTER TABLEfrom Case 1.CHECK TABLE roms;reportsOK.Restoring the pre-upgrade backup and applying Case 1 before re-upgrading works equally well.
Case 3 - never successfully ran 0098 (the migration aborted). Nothing to do; the
ALTERfails as a unit, so no partial columns are left behind. Just update to a build containing this PR.Checklist
Testing
Verified against real MariaDB containers, not just static review:
ALTER TABLEfrom the migration applies with no other expression rejected.+05:30,-08:00and+09:00:19980521T134500->895758300000and20240229T235959->1709251199000, both matching Python's UTC computation. Malformed andnullgamelist dates ->NULL; the integer branches (igdb x1000, manual x1) are unaffected.STRICT_TRANS_TABLESa single pre-existing20201301T000000row aborts the migrationALTERwithIncorrect datetime value.CAST(... AS DATETIME)(which would yieldNULL) is itself barred from a generated column, so the components are range-checked in the CASE guard instead.NULLwith no error on MariaDB 12.3.2 and MySQL 8.4.10: month 13, day 30 in February, day 31 in April, Feb 29 in a non-leap year, Feb 29 in 1900 (century non-leap), day 0, all-zeroes, hour 25, minute 60. Leap-year handling verified both ways: 2020/2024/2000 Feb 29 accepted, 2021/1900 rejected.ALTERnow succeeds on a table already holding invalid rows; they land asNULLwhile valid siblings compute correctly.ALTERabove -> re-upgrade to 12.3.2 ->SELECT,INSERTandCHECK TABLE romsall OK with values preserved.No unit test added: the failure only reproduces against a live MariaDB 12.3 server, which CI does not currently run.
AI assistance disclosure
This change was written with AI assistance (Claude Code). The AI diagnosed the root cause, identified the 12.3 version boundary and the table-open failure mode, wrote the replacement expression and the manual fix, and ran the MariaDB container testing described above. All of it was reviewed by me before submitting.