Skip to content

fix(brainztableinator): widen MusicBrainz Discogs ID columns to BIGINT #404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
SimplicityGuy wants to merge 1 commit into from
Closed

fix(brainztableinator): widen MusicBrainz Discogs ID columns to BIGINT #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions brainztableinator/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ CREATE TABLE musicbrainz.artists (
begin_area TEXT,
end_area TEXT,
disambiguation TEXT,
discogs_artist_id INTEGER,
discogs_artist_id BIGINT,
aliases JSONB,
tags JSONB,
data JSONB,
Expand All @@ -102,7 +102,7 @@ CREATE TABLE musicbrainz.labels (
ended BOOLEAN DEFAULT FALSE,
area TEXT,
disambiguation TEXT,
discogs_label_id INTEGER,
discogs_label_id BIGINT,
data JSONB,
created_at TIMESTAMPTZ DEFAULT NOW(),
updated_at TIMESTAMPTZ DEFAULT NOW()
Expand All @@ -118,7 +118,7 @@ CREATE TABLE musicbrainz.releases (
barcode TEXT,
status TEXT,
release_group_mbid UUID,
discogs_release_id INTEGER,
discogs_release_id BIGINT,
data JSONB,
created_at TIMESTAMPTZ DEFAULT NOW(),
updated_at TIMESTAMPTZ DEFAULT NOW()
Expand All @@ -135,7 +135,7 @@ CREATE TABLE musicbrainz.release_groups (
secondary_types JSONB,
first_release_date TEXT,
disambiguation TEXT,
discogs_master_id INTEGER,
discogs_master_id BIGINT,
data JSONB,
created_at TIMESTAMPTZ DEFAULT NOW(),
updated_at TIMESTAMPTZ DEFAULT NOW()
Expand Down
46 changes: 40 additions & 6 deletions schema-init/postgres_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@
begin_area TEXT,
end_area TEXT,
disambiguation TEXT,
discogs_artist_id INTEGER,
discogs_artist_id BIGINT,
aliases JSONB,
tags JSONB,
data JSONB,
Expand All @@ -559,7 +559,7 @@
ended BOOLEAN DEFAULT FALSE,
area TEXT,
disambiguation TEXT,
discogs_label_id INTEGER,
discogs_label_id BIGINT,
data JSONB,
created_at TIMESTAMPTZ DEFAULT NOW(),
updated_at TIMESTAMPTZ DEFAULT NOW()
Expand All @@ -573,7 +573,7 @@
barcode TEXT,
status TEXT,
release_group_mbid UUID,
discogs_release_id INTEGER,
discogs_release_id BIGINT,
data JSONB,
created_at TIMESTAMPTZ DEFAULT NOW(),
updated_at TIMESTAMPTZ DEFAULT NOW()
Expand All @@ -588,7 +588,7 @@
secondary_types JSONB,
first_release_date TEXT,
disambiguation TEXT,
discogs_master_id INTEGER,
discogs_master_id BIGINT,
data JSONB,
created_at TIMESTAMPTZ DEFAULT NOW(),
updated_at TIMESTAMPTZ DEFAULT NOW()
Expand Down Expand Up @@ -626,6 +626,35 @@
]


# MusicBrainz migrations — widen Discogs ID columns to BIGINT on existing
# deployments. The CREATE TABLE statements above already use BIGINT, but
# `CREATE TABLE IF NOT EXISTS` never alters a pre-existing table, so tables
# created before this change keep their INTEGER (int4) columns. Discogs IDs have
# crossed the int4 limit (2,147,483,647), causing "integer out of range" on
# insert in brainztableinator. `ALTER COLUMN ... TYPE BIGINT` is a no-op (no
# table rewrite) when the column is already BIGINT, so this is safe to run on
# every startup. Must run AFTER the CREATE TABLE statements and BEFORE the
# indexes that reference these columns.
_MUSICBRAINZ_MIGRATIONS: list[tuple[str, str]] = [
(
"musicbrainz.artists.discogs_artist_id -> BIGINT",
"ALTER TABLE musicbrainz.artists ALTER COLUMN discogs_artist_id TYPE BIGINT",
),
(
"musicbrainz.labels.discogs_label_id -> BIGINT",
"ALTER TABLE musicbrainz.labels ALTER COLUMN discogs_label_id TYPE BIGINT",
),
(
"musicbrainz.releases.discogs_release_id -> BIGINT",
"ALTER TABLE musicbrainz.releases ALTER COLUMN discogs_release_id TYPE BIGINT",
),
(
"musicbrainz.release_groups.discogs_master_id -> BIGINT",
"ALTER TABLE musicbrainz.release_groups ALTER COLUMN discogs_master_id TYPE BIGINT",
),
]


# MusicBrainz indexes — optimized queries for cross-database lookups
_MUSICBRAINZ_INDEXES: list[tuple[str, str]] = [
(
Expand Down Expand Up @@ -772,8 +801,12 @@ async def create_postgres_schema(pool: Any) -> int:
logger.error(f"❌ Failed to create schema object '{name}': {e}")
failure_count += 1

# ── MusicBrainz tables ────────────────────────────────────────
for name, stmt in _MUSICBRAINZ_TABLES + _MUSICBRAINZ_INDEXES:
# ── MusicBrainz tables, migrations, indexes ───────────────────
# Order matters: tables first (so they exist), then column-widening
# migrations, then indexes that reference the widened columns.
for name, stmt in (
_MUSICBRAINZ_TABLES + _MUSICBRAINZ_MIGRATIONS + _MUSICBRAINZ_INDEXES
):
try:
await cursor.execute(stmt)
logger.info(f"✅ Schema: {name}")
Expand All @@ -788,6 +821,7 @@ async def create_postgres_schema(pool: Any) -> int:
+ len(_USER_TABLES)
+ len(_INSIGHTS_TABLES)
+ len(_MUSICBRAINZ_TABLES)
+ len(_MUSICBRAINZ_MIGRATIONS)
+ len(_MUSICBRAINZ_INDEXES)
)
logger.info(
Expand Down
62 changes: 60 additions & 2 deletions tests/schema-init/test_postgres_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""Tests for schema-init/postgres_schema.py."""

from typing import Any
from typing import Any, ClassVar
from unittest.mock import AsyncMock, MagicMock

import pytest
Expand All @@ -9,6 +9,7 @@
_ENTITY_TABLES,
_INSIGHTS_TABLES,
_MUSICBRAINZ_INDEXES,
_MUSICBRAINZ_MIGRATIONS,
_MUSICBRAINZ_TABLES,
_SPECIFIC_INDEXES,
_USER_TABLES,
Expand Down Expand Up @@ -107,6 +108,56 @@ def test_insights_tables_are_defined(self) -> None:
assert "insights.computation_log table" in table_names


class TestMusicBrainzDiscogsIdColumns:
"""Discogs IDs exceed the int4 limit (2,147,483,647), so the MusicBrainz
columns that store them must be BIGINT. Regression guard for the
brainztableinator 'integer out of range' overflow on releases.
"""

_DISCOGS_ID_COLUMNS: ClassVar[list[tuple[str, str]]] = [
("musicbrainz.artists table", "discogs_artist_id"),
("musicbrainz.labels table", "discogs_label_id"),
("musicbrainz.releases table", "discogs_release_id"),
("musicbrainz.release_groups table", "discogs_master_id"),
]

_MIGRATION_NAMES: ClassVar[list[str]] = [
"musicbrainz.artists.discogs_artist_id -> BIGINT",
"musicbrainz.labels.discogs_label_id -> BIGINT",
"musicbrainz.releases.discogs_release_id -> BIGINT",
"musicbrainz.release_groups.discogs_master_id -> BIGINT",
]

@pytest.mark.parametrize(("table_name", "column"), _DISCOGS_ID_COLUMNS)
def test_create_table_uses_bigint(self, table_name: str, column: str) -> None:
"""Fresh deployments: the CREATE TABLE column must be BIGINT, not INTEGER."""
sql = dict(_MUSICBRAINZ_TABLES)[table_name]
assert f"{column} BIGINT" in sql, f"{column} in {table_name} must be BIGINT"
assert f"{column} INTEGER" not in sql, f"{column} in {table_name} is still INTEGER"

@pytest.mark.parametrize("statement_name", _MIGRATION_NAMES)
def test_widening_migration_present(self, statement_name: str) -> None:
"""Existing deployments: an ALTER COLUMN ... TYPE BIGINT migration must exist.

CREATE TABLE IF NOT EXISTS never alters a pre-existing table.
"""
sql = dict(_MUSICBRAINZ_MIGRATIONS)[statement_name]
assert "ALTER COLUMN" in sql and "TYPE BIGINT" in sql

def test_one_migration_per_discogs_id_column(self) -> None:
assert len(_MUSICBRAINZ_MIGRATIONS) == len(self._DISCOGS_ID_COLUMNS)

def test_migrations_run_after_tables_and_before_indexes(self) -> None:
"""Execution order: tables exist, then widen columns, then build indexes."""
names = [name for name, _ in _MUSICBRAINZ_TABLES + _MUSICBRAINZ_MIGRATIONS + _MUSICBRAINZ_INDEXES]
last_table = max(i for i, n in enumerate(names) if n in dict(_MUSICBRAINZ_TABLES))
first_migration = min(i for i, n in enumerate(names) if n.endswith("-> BIGINT"))
last_migration = max(i for i, n in enumerate(names) if n.endswith("-> BIGINT"))
first_index = min(i for i, n in enumerate(names) if n.startswith("idx_mb_"))
assert last_table < first_migration
assert last_migration < first_index


class TestAppTokensTable:
"""Schema-shape tests for the app_tokens third-party auth table."""

Expand Down Expand Up @@ -186,6 +237,7 @@ async def test_runs_all_statements(self, mock_pool: MagicMock) -> None:
+ len(_USER_TABLES)
+ len(_INSIGHTS_TABLES)
+ len(_MUSICBRAINZ_TABLES)
+ len(_MUSICBRAINZ_MIGRATIONS)
+ len(_MUSICBRAINZ_INDEXES)
)
assert cursor.execute.await_count == expected_calls
Expand Down Expand Up @@ -213,6 +265,7 @@ async def flaky(*args: Any, **kwargs: Any) -> None: # noqa: ARG001
+ len(_USER_TABLES)
+ len(_INSIGHTS_TABLES)
+ len(_MUSICBRAINZ_TABLES)
+ len(_MUSICBRAINZ_MIGRATIONS)
+ len(_MUSICBRAINZ_INDEXES)
)
assert cursor.execute.await_count == expected_calls
Expand All @@ -232,7 +285,11 @@ async def capture(stmt: Any, *_: Any, **__: Any) -> None:

for stmt in captured:
upper = stmt.upper()
assert "IF NOT EXISTS" in upper, f"Statement is not idempotent: {stmt[:80]}..."
# `ALTER TABLE ... ALTER COLUMN ... TYPE` is inherently idempotent:
# PostgreSQL performs no table rewrite when the column is already the
# target type, so re-running is a no-op (no IF NOT EXISTS form exists).
is_idempotent_alter_type = "ALTER COLUMN" in upper and "TYPE " in upper
assert "IF NOT EXISTS" in upper or is_idempotent_alter_type, f"Statement is not idempotent: {stmt[:80]}..."
# A multi-statement blob could still hide an un-guarded DROP that would
# not be idempotent — any DROP must be guarded with IF EXISTS.
if "DROP " in upper:
Expand All @@ -253,6 +310,7 @@ async def test_all_fail_gracefully(self, mock_pool: MagicMock) -> None:
+ len(_USER_TABLES)
+ len(_INSIGHTS_TABLES)
+ len(_MUSICBRAINZ_TABLES)
+ len(_MUSICBRAINZ_MIGRATIONS)
+ len(_MUSICBRAINZ_INDEXES)
)
assert cursor.execute.await_count == expected_calls
Expand Down