fix(db): restore the PostgreSQL upgrade path #4

2026-09-12T15:32:28+10:00

unkin-agent commented

2026-09-12 15:32:28 +10:00

The v12.0 rebase replaced the PostgreSQL provider initial migration instead of adding to it, so an existing database keeps the pre-12.0 schema, the code migrations run against it and startup aborts on a missing Users.NormalizedUsername.

restore 20260305010333_InitialPostgreSql as the baseline
add 20260306000000_UpgradeToServer12Schema carrying it to the 12.0 model
cast owner and version ids to uuid, nulling anything the cast cannot parse
recreate the detached-item placeholder before repointing dangling owners at it
add 20260524120336_AddUniqueNormalizedUsernameIndex after the code migration that fills the column in

The v12.0 rebase replaced the PostgreSQL provider initial migration instead of adding to it, so an existing database keeps the pre-12.0 schema, the code migrations run against it and startup aborts on a missing `Users.NormalizedUsername`. - restore `20260305010333_InitialPostgreSql` as the baseline - add `20260306000000_UpgradeToServer12Schema` carrying it to the 12.0 model - cast owner and version ids to uuid, nulling anything the cast cannot parse - recreate the detached-item placeholder before repointing dangling owners at it - add `20260524120336_AddUniqueNormalizedUsernameIndex` after the code migration that fills the column in

unkin-agent added 1 commit 2026-09-12 15:32:29 +10:00

fix(db): restore the PostgreSQL upgrade path

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

2e3d24cc05

The v12.0 rebase replaced the PostgreSQL provider's initial migration instead of
adding to it, so an existing database kept the pre-12.0 schema while the code
migrations ran against it and startup aborted on a missing NormalizedUsername.

- restore 20260305010333_InitialPostgreSql as the baseline
- add 20260306000000_UpgradeToServer12Schema carrying it to the 12.0 model
- convert OwnerId and PrimaryVersionId to uuid with explicit casts, clear
  unparseable ids and repoint dangling owners at the placeholder item
- drop orphaned permissions and preferences before UserId becomes non-nullable
- add 20260524120336_AddUniqueNormalizedUsernameIndex after the code migration
  that fills the column in
- read an empty or unknown encoding.xml EncoderPreset as the default
- cover both paths against a real PostgreSQL and guard the migration ordering

unkin-agent commented

2026-09-12 16:03:52 +10:00

Review: restore the PostgreSQL upgrade path

Reviewed as an irreversible change against live data. I rebuilt the branch and ran things rather than reading only.

What I verified by running it

Baseline is byte-identical. cmp of 20260305010333_InitialPostgreSql.cs and .Designer.cs against 3bc45ff92d — identical. git log --all --name-status over the migrations directory shows 3bc45ff92d is the only commit that ever wrote those files, so that is what created the live databases and the recorded history id matches.
Schema equivalence, independently reproduced. Generated dotnet ef migrations script on origin/main (single regenerated baseline) and on this branch (3-migration chain), applied each to its own database, compared information_schema.columns, pg_indexes, pg_constraint, pg_trigger, pg_sequences: 312 / 95 / 60 / 0 / 12 rows, all identical, and the seeded …0001 placeholder row is identical including the corrected detached spelling.
Model snapshot. Not touched. 20260524120336_AddUniqueNormalizedUsernameIndex.Designer.cs is identical to JellyfinDbContextModelSnapshot.cs; 20260306000000's designer differs only by the missing HasIndex("NormalizedUsername").IsUnique(). HasPendingModelChanges() is false.
Ordering claim is correct. JellyfinMigrationAttribute.cs:37 assigns Stage = AppInitialisation in the constructor body, which runs after the :52 property initialiser and therefore wins — so CoreInitialisation is opt-in, not the default. JellyfinMigrationService.cs:218-223 enumerates EF migrations only for CoreInitialisation, and :236 merges both into one ordinal sort. Exactly two routines opt in (20260522092304_UpdateNormalizedUsername, 20260531160000_DisableLegacyAuthorization); everything else, including MigrateLinkedChildren, CleanupOrphanedExtras and FixIncorrectOwnerIdRelationships, runs after the whole EF chain, which is what those need. The merge loop is upstream code, not a fork patch.
PostgreSqlMigrationOrderingTests is a real regression test. Ported onto origin/main and ran it: both facts fail, with Schema migration 20260911134055_InitialPostgreSql has to be ordered before code migration 20260522092304_UpdateNormalizedUsername.
Migrations are transactional. The generated script wraps each migration, history row included, in START TRANSACTION … COMMIT, and I confirmed a mid-migration failure rolls back completely (column still text, only the baseline recorded).
The data steps match upstream. Each hand-correction has a SQLite counterpart doing the same thing: 20260113233500_DropExtraIdsColumn, 20260113233000_AddForeignKeyToOwnerId (same placeholder repoint SQL), 20260815063607_RemoveOrphanedUserPermissionsAndPreferences (same two deletes), 20260113203012_ChangeOwnerIdToGuid / 20260215201634_ChangePrimaryVersionIdToGuid. ExtraIds and OriginalLanguage are genuinely independent — the old baseline has only ExtraIds (…InitialPostgreSql.cs:111), the v12 model only OriginalLanguage — and MigrateLibraryDb.cs:1213 documents ExtraIds as superseded by the OwnerId relation, so the drop+add is right and loses nothing v12 reads.
Pre-existing failures confirmed. tests/Jellyfin.Database.Tests.PostgreSQL: 2 failed / 7 passed on this branch and on origin/main, same two tests (PostgreSqlProviderTests.Crud_DisplayPreferences, PurgeDatabase_EmptiesTablesAndResetsFkRole). Not regressions.
All eight tests in Jellyfin.Server.Tests.Migrations pass on a real container here.

I also seeded nastier legacy databases than the suite does. Results below.

Findings

1 — Medium. Two usernames differing only by case wedge the upgrade past the point of no return.
Migrations/20260524120336_AddUniqueNormalizedUsernameIndex.cs:13, with Routines/20260522092304_UpdateNormalizedUsername.cs:38.
Reproduced: seed Ben and ben, run the chain — 20260306000000 commits, the code migration commits, then CREATE UNIQUE INDEX fails with SQLSTATE 23505 and startup aborts on every restart. At that point BaseItems.OwnerId is already uuid, so the previous image cannot be rolled back into either. Users.Username is unique but case-sensitive in PostgreSQL, so this is possible on a real instance. Upstream has the same hazard; the difference is that these two databases are live.
Fix: before deploying, run on both databases SELECT upper("Username"), count(*) FROM "Users" GROUP BY 1 HAVING count(*) > 1; and rename any collision. Better, make the fill-in disambiguate instead of failing.

2 — Medium. The suite that proves this fix does not run in CI.
.woodpecker/ci.yaml:28 filters Category!=RequiresDocker, and PostgreSqlUpgradeTests.cs:27 carries that trait. Green CI here only exercised PostgreSqlMigrationOrderingTests and the EncodingOptions tests — the upgrade proof was never executed by the pipeline.
Fix: add a step running --filter "Category=RequiresDocker" on a docker-capable backend (it would also pick up the existing Jellyfin.Database.Tests.PostgreSQL suite).

3 — Low/Medium. A 32-hex id with non-standard hyphen placement passes the guard and then aborts the migration.
20260306000000_UpgradeToServer12Schema.cs:127 and :138. replace(x,'-','') ~ '^[0-9a-fA-F]{32}$' accepts e.g. 1111111-11111-1111-1111-111111111111, which ::uuid rejects. Reproduced: the migration throws and rolls back cleanly, so nothing is corrupted, but startup is wedged until someone runs SQL by hand. Everything genuinely junk (empty string, braces, leading space, non-hex, all-zero) is correctly nulled — I checked all of those.
Fix: make the USING clause total rather than pre-filtering, e.g. USING CASE WHEN "OwnerId" ~ '^[0-9a-fA-F]{32}$' OR "OwnerId" ~ '^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$' THEN "OwnerId"::uuid END.

4 — Low. Divergence from the SQLite chain for OwnerId = …0001.
SQLite 20260113203012_ChangeOwnerIdToGuid nulls OwnerId where it already equals the detached placeholder, before the repoint; 20260306000000_UpgradeToServer12Schema.cs:136-143 does not. Reproduced: such a row keeps …0001, so Routines/20260113230000_CleanupOrphanedExtras.cs:57 then deletes the item, whereas on SQLite it survives with a null owner. Nothing pre-12 writes that value, so it is unlikely here, but it is a silent delete.
Fix: add UPDATE "BaseItems" SET "OwnerId" = NULL WHERE "OwnerId" IN ('00000000-0000-0000-0000-000000000000','00000000-0000-0000-0000-000000000001'); before the cast.

5 — Low. The repoint assumes the …0001 row exists.
20260306000000_UpgradeToServer12Schema.cs:307-320. I deleted that row and re-ran: AddForeignKey fails with Key (OwnerId)=(00000000-0000-0000-0000-000000000001) is not present in table "BaseItems" and the migration rolls back, wedging startup. The baseline seeds it so it should be present, but one INSERT … ON CONFLICT DO NOTHING before the repoint removes the failure mode.

6 — Low, deployment. There is no automatic rollback on PostgreSQL.
PostgreSqlDatabaseProvider.cs:71-84: MigrationBackupFast returns a sentinel and RestoreBackupFast is a no-op, so JellyfinMigrationService's failure path restores nothing. Each EF migration is individually atomic, but the chain plus the ~15 AppInitialisation routines is not, and CleanupOrphanedExtras deletes rows. Take a verified pg_dump of both databases immediately before deploying rather than relying on the CronJob schedule.

7 — Minor. The encoding.xml fix is a separate concern.
MediaBrowser.Model/Configuration/EncodingOptions.cs:240-256. The diagnosis is right — BaseConfigurationManager.cs:304-320 catches and returns defaults, so one bad element discards every encoding setting — and the XML/JSON split is correct. It is still an unrelated subsystem and belongs in its own PR. Separately, Enum.TryParse accepts numeric strings, so <EncoderPreset>42</EncoderPreset> yields an undefined enum value instead of auto; add an Enum.IsDefined check.

8 — Nit. Test gaps.
PostgreSqlUpgradeTests.cs:315 hand-codes the startup order instead of driving JellyfinMigrationService — acceptable, since it uses the real UpdateNormalizedUsername routine and the ordering is asserted separately. LegacyDatabaseAndFreshInstall_EndUpWithTheSameSchema:137 compares chain-against-chain, not chain-against-the-v12-model, so it cannot catch the whole chain drifting together; the stronger comparison against the model-generated baseline is the one that actually proves the claim. It also omits pg_constraint, so an FK or PK difference would slip through.

Verdict

The migration logic is correct and the central equivalence claim holds under independent reproduction. Nothing here is a blocker. Before it touches the live databases: run the username-collision query from finding 1, take a fresh pg_dump of both, and note that once 20260306000000 commits there is no going back to the previous image.

## Review: restore the PostgreSQL upgrade path Reviewed as an irreversible change against live data. I rebuilt the branch and ran things rather than reading only. ### What I verified by running it - **Baseline is byte-identical.** `cmp` of `20260305010333_InitialPostgreSql.cs` and `.Designer.cs` against `3bc45ff92d` — identical. `git log --all --name-status` over the migrations directory shows `3bc45ff92d` is the only commit that ever wrote those files, so that is what created the live databases and the recorded history id matches. - **Schema equivalence, independently reproduced.** Generated `dotnet ef migrations script` on `origin/main` (single regenerated baseline) and on this branch (3-migration chain), applied each to its own database, compared `information_schema.columns`, `pg_indexes`, `pg_constraint`, `pg_trigger`, `pg_sequences`: **312 / 95 / 60 / 0 / 12 rows, all identical**, and the seeded `…0001` placeholder row is identical including the corrected `detached` spelling. - **Model snapshot.** Not touched. `20260524120336_AddUniqueNormalizedUsernameIndex.Designer.cs` is identical to `JellyfinDbContextModelSnapshot.cs`; `20260306000000`'s designer differs only by the missing `HasIndex("NormalizedUsername").IsUnique()`. `HasPendingModelChanges()` is false. - **Ordering claim is correct.** `JellyfinMigrationAttribute.cs:37` assigns `Stage = AppInitialisation` in the constructor body, which runs after the `:52` property initialiser and therefore wins — so `CoreInitialisation` is opt-in, not the default. `JellyfinMigrationService.cs:218-223` enumerates EF migrations only for `CoreInitialisation`, and `:236` merges both into one ordinal sort. Exactly two routines opt in (`20260522092304_UpdateNormalizedUsername`, `20260531160000_DisableLegacyAuthorization`); everything else, including `MigrateLinkedChildren`, `CleanupOrphanedExtras` and `FixIncorrectOwnerIdRelationships`, runs after the whole EF chain, which is what those need. The merge loop is upstream code, not a fork patch. - **`PostgreSqlMigrationOrderingTests` is a real regression test.** Ported onto `origin/main` and ran it: both facts fail, with `Schema migration 20260911134055_InitialPostgreSql has to be ordered before code migration 20260522092304_UpdateNormalizedUsername`. - **Migrations are transactional.** The generated script wraps each migration, history row included, in `START TRANSACTION … COMMIT`, and I confirmed a mid-migration failure rolls back completely (column still `text`, only the baseline recorded). - **The data steps match upstream.** Each hand-correction has a SQLite counterpart doing the same thing: `20260113233500_DropExtraIdsColumn`, `20260113233000_AddForeignKeyToOwnerId` (same placeholder repoint SQL), `20260815063607_RemoveOrphanedUserPermissionsAndPreferences` (same two deletes), `20260113203012_ChangeOwnerIdToGuid` / `20260215201634_ChangePrimaryVersionIdToGuid`. `ExtraIds` and `OriginalLanguage` are genuinely independent — the old baseline has only `ExtraIds` (`…InitialPostgreSql.cs:111`), the v12 model only `OriginalLanguage` — and `MigrateLibraryDb.cs:1213` documents `ExtraIds` as superseded by the `OwnerId` relation, so the drop+add is right and loses nothing v12 reads. - **Pre-existing failures confirmed.** `tests/Jellyfin.Database.Tests.PostgreSQL`: 2 failed / 7 passed on this branch *and* on `origin/main`, same two tests (`PostgreSqlProviderTests.Crud_DisplayPreferences`, `PurgeDatabase_EmptiesTablesAndResetsFkRole`). Not regressions. - **All eight tests in `Jellyfin.Server.Tests.Migrations` pass** on a real container here. I also seeded nastier legacy databases than the suite does. Results below. ### Findings **1 — Medium. Two usernames differing only by case wedge the upgrade past the point of no return.** `Migrations/20260524120336_AddUniqueNormalizedUsernameIndex.cs:13`, with `Routines/20260522092304_UpdateNormalizedUsername.cs:38`. Reproduced: seed `Ben` and `ben`, run the chain — `20260306000000` commits, the code migration commits, then `CREATE UNIQUE INDEX` fails with SQLSTATE `23505` and startup aborts on every restart. At that point `BaseItems.OwnerId` is already `uuid`, so the previous image cannot be rolled back into either. `Users.Username` is unique but case-sensitive in PostgreSQL, so this is possible on a real instance. Upstream has the same hazard; the difference is that these two databases are live. Fix: before deploying, run on both databases `SELECT upper("Username"), count(*) FROM "Users" GROUP BY 1 HAVING count(*) > 1;` and rename any collision. Better, make the fill-in disambiguate instead of failing. **2 — Medium. The suite that proves this fix does not run in CI.** `.woodpecker/ci.yaml:28` filters `Category!=RequiresDocker`, and `PostgreSqlUpgradeTests.cs:27` carries that trait. Green CI here only exercised `PostgreSqlMigrationOrderingTests` and the `EncodingOptions` tests — the upgrade proof was never executed by the pipeline. Fix: add a step running `--filter "Category=RequiresDocker"` on a docker-capable backend (it would also pick up the existing `Jellyfin.Database.Tests.PostgreSQL` suite). **3 — Low/Medium. A 32-hex id with non-standard hyphen placement passes the guard and then aborts the migration.** `20260306000000_UpgradeToServer12Schema.cs:127` and `:138`. `replace(x,'-','') ~ '^[0-9a-fA-F]{32}$'` accepts e.g. `1111111-11111-1111-1111-111111111111`, which `::uuid` rejects. Reproduced: the migration throws and rolls back cleanly, so nothing is corrupted, but startup is wedged until someone runs SQL by hand. Everything genuinely junk (empty string, braces, leading space, non-hex, all-zero) is correctly nulled — I checked all of those. Fix: make the `USING` clause total rather than pre-filtering, e.g. `USING CASE WHEN "OwnerId" ~ '^[0-9a-fA-F]{32}$' OR "OwnerId" ~ '^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$' THEN "OwnerId"::uuid END`. **4 — Low. Divergence from the SQLite chain for `OwnerId = …0001`.** SQLite `20260113203012_ChangeOwnerIdToGuid` nulls `OwnerId` where it already equals the detached placeholder, before the repoint; `20260306000000_UpgradeToServer12Schema.cs:136-143` does not. Reproduced: such a row keeps `…0001`, so `Routines/20260113230000_CleanupOrphanedExtras.cs:57` then deletes the item, whereas on SQLite it survives with a null owner. Nothing pre-12 writes that value, so it is unlikely here, but it is a silent delete. Fix: add `UPDATE "BaseItems" SET "OwnerId" = NULL WHERE "OwnerId" IN ('00000000-0000-0000-0000-000000000000','00000000-0000-0000-0000-000000000001');` before the cast. **5 — Low. The repoint assumes the `…0001` row exists.** `20260306000000_UpgradeToServer12Schema.cs:307-320`. I deleted that row and re-ran: `AddForeignKey` fails with `Key (OwnerId)=(00000000-0000-0000-0000-000000000001) is not present in table "BaseItems"` and the migration rolls back, wedging startup. The baseline seeds it so it should be present, but one `INSERT … ON CONFLICT DO NOTHING` before the repoint removes the failure mode. **6 — Low, deployment. There is no automatic rollback on PostgreSQL.** `PostgreSqlDatabaseProvider.cs:71-84`: `MigrationBackupFast` returns a sentinel and `RestoreBackupFast` is a no-op, so `JellyfinMigrationService`'s failure path restores nothing. Each EF migration is individually atomic, but the chain plus the ~15 `AppInitialisation` routines is not, and `CleanupOrphanedExtras` deletes rows. Take a verified `pg_dump` of both databases immediately before deploying rather than relying on the CronJob schedule. **7 — Minor. The `encoding.xml` fix is a separate concern.** `MediaBrowser.Model/Configuration/EncodingOptions.cs:240-256`. The diagnosis is right — `BaseConfigurationManager.cs:304-320` catches and returns defaults, so one bad element discards every encoding setting — and the XML/JSON split is correct. It is still an unrelated subsystem and belongs in its own PR. Separately, `Enum.TryParse` accepts numeric strings, so `<EncoderPreset>42</EncoderPreset>` yields an undefined enum value instead of `auto`; add an `Enum.IsDefined` check. **8 — Nit. Test gaps.** `PostgreSqlUpgradeTests.cs:315` hand-codes the startup order instead of driving `JellyfinMigrationService` — acceptable, since it uses the real `UpdateNormalizedUsername` routine and the ordering is asserted separately. `LegacyDatabaseAndFreshInstall_EndUpWithTheSameSchema:137` compares chain-against-chain, not chain-against-the-v12-model, so it cannot catch the whole chain drifting together; the stronger comparison against the model-generated baseline is the one that actually proves the claim. It also omits `pg_constraint`, so an FK or PK difference would slip through. ### Verdict The migration logic is correct and the central equivalence claim holds under independent reproduction. Nothing here is a blocker. Before it touches the live databases: run the username-collision query from finding 1, take a fresh `pg_dump` of both, and note that once `20260306000000` commits there is no going back to the previous image.

unkin-agent added 2 commits 2026-09-12 16:21:53 +10:00

fix(db): harden the PostgreSQL upgrade against odd legacy data 65af2bcbd7

- accept only the two id shapes the uuid cast parses, so misplaced hyphens
  null out instead of aborting the migration
- clear an OwnerId that already is the detached placeholder, matching the
  SQLite chain, so CleanupOrphanedExtras cannot delete the item
- recreate the placeholder item before the repoint if it is missing
- compare the migrated schema against the model, constraints included

revert: move the encoding.xml EncoderPreset fix to its own branch

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

2f9b8888e9

It is an unrelated subsystem and ships as a separate change.

benvin merged commit 10fed9409d into main

2026-09-12 17:55:57 +10:00

benvin deleted branch benvin/fix-postgres-upgrade

2026-09-12 17:55:57 +10:00

unkin-agent referenced this pull request

2026-09-19 12:05:23 +10:00

PostgreSQL: /Shows/NextUp 500s on unnormalised DateTime.Kind=Unspecified cutoff (Npgsql rejects it for timestamptz) #26

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/jellyfin-ha-src#4