From c454da460c24e4dd7d61c23bb02defc8f97501b9 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Wed, 24 Jun 2026 15:58:22 -0700 Subject: [PATCH 1/2] Drop coveragerecords and equivalentscoveragerecords tables Release-2 half of the CoverageProvider retirement. The previous release removed the coverage_records relationships, which is what actually stopped the ORM reading these tables, so N-1 no longer maps either one and both can go. - Remove the CoverageRecord and EquivalencyCoverageRecord models from sqlalchemy/model/coverage.py, leaving the Timestamp model and its separate service_type enum. - Add a migration dropping coveragerecords and equivalentscoveragerecords along with the now-orphaned coverage_status enum; the downgrade recreates both tables with their indexes, constraints and foreign keys. - Adapt the previous release's lock_timeout test. It ran the truncate migration against the schema built by create_all, which no longer has coveragerecords now that the model is gone, so it steps back to that revision first -- the drop's downgrade recreates the table. The test still earns its place: a database older than that revision upgrades through it and this drop in a single transaction, which is exactly the leak the reset guards against. Co-Authored-By: Claude Opus 5 --- ..._5ca948100902_drop_coveragerecords_and_.py | 199 ++++++++++++++++++ .../manager/sqlalchemy/model/coverage.py | 95 +-------- ...0916_58ebd34c5092_empty_coveragerecords.py | 14 +- ...60916_5ca948100902_drop_coverage_tables.py | 38 ++++ 4 files changed, 250 insertions(+), 96 deletions(-) create mode 100644 alembic/versions/20260916_5ca948100902_drop_coveragerecords_and_.py create mode 100644 tests/migration/test_20260916_5ca948100902_drop_coverage_tables.py diff --git a/alembic/versions/20260916_5ca948100902_drop_coveragerecords_and_.py b/alembic/versions/20260916_5ca948100902_drop_coveragerecords_and_.py new file mode 100644 index 0000000000..a32b5c70e7 --- /dev/null +++ b/alembic/versions/20260916_5ca948100902_drop_coveragerecords_and_.py @@ -0,0 +1,199 @@ +"""Drop coveragerecords and equivalentscoveragerecords tables + +The CoverageProvider machinery that read and wrote ``coveragerecords`` was retired +two releases ago, and the previous release removed the ``coverage_records`` +relationships that were still making SQLAlchemy read the table on every parent +delete (see 58ebd34c5092). The equivalent-identifiers refresh that wrote +``equivalentscoveragerecords`` moved to a Celery task before either. N-1 therefore +maps neither table, so both are dropped here along with their shared +``coverage_status`` enum type. + +The ``timestamps`` table and its separate ``service_type`` enum are unaffected. + +Revision ID: 5ca948100902 +Revises: 58ebd34c5092 +Create Date: 2026-09-16 00:00:00.000000+00:00 + +""" + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision = "5ca948100902" +down_revision = "58ebd34c5092" +branch_labels = None +depends_on = None + +# The status enum shared by the two dropped tables. We manage the Postgres type +# explicitly (create_type=False) so it is created/dropped exactly once rather +# than once per table. +coverage_status = postgresql.ENUM( + "success", + "transient failure", + "persistent failure", + "registered", + name="coverage_status", + create_type=False, +) + + +def upgrade() -> None: + op.drop_index( + op.f("ix_equivalentscoveragerecords_equivalency_id"), + table_name="equivalentscoveragerecords", + ) + op.drop_index( + op.f("ix_equivalentscoveragerecords_operation"), + table_name="equivalentscoveragerecords", + ) + op.drop_index( + op.f("ix_equivalentscoveragerecords_status"), + table_name="equivalentscoveragerecords", + ) + op.drop_index( + op.f("ix_equivalentscoveragerecords_timestamp"), + table_name="equivalentscoveragerecords", + ) + op.drop_table("equivalentscoveragerecords") + + op.drop_index( + op.f("ix_coveragerecords_data_source_id_operation_identifier_id"), + table_name="coveragerecords", + ) + op.drop_index(op.f("ix_coveragerecords_exception"), table_name="coveragerecords") + op.drop_index( + op.f("ix_coveragerecords_identifier_id"), table_name="coveragerecords" + ) + op.drop_index(op.f("ix_coveragerecords_status"), table_name="coveragerecords") + op.drop_index(op.f("ix_coveragerecords_timestamp"), table_name="coveragerecords") + op.drop_index( + "ix_identifier_id_data_source_id_operation", table_name="coveragerecords" + ) + op.drop_index( + "ix_identifier_id_data_source_id_operation_collection_id", + table_name="coveragerecords", + ) + op.drop_table("coveragerecords") + + # The coverage_status enum was used only by the two tables just dropped. + coverage_status.drop(op.get_bind(), checkfirst=False) + + +def downgrade() -> None: + coverage_status.create(op.get_bind(), checkfirst=False) + + op.create_table( + "coveragerecords", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("identifier_id", sa.Integer(), nullable=True), + sa.Column("data_source_id", sa.Integer(), nullable=True), + sa.Column("operation", sa.String(length=255), nullable=True), + sa.Column("timestamp", sa.DateTime(timezone=True), nullable=True), + sa.Column("status", coverage_status, nullable=True), + sa.Column("exception", sa.Unicode(), nullable=True), + sa.Column("collection_id", sa.Integer(), nullable=True), + sa.ForeignKeyConstraint( + ["collection_id"], + ["collections.id"], + name=op.f("coveragerecords_collection_id_fkey"), + ), + sa.ForeignKeyConstraint( + ["data_source_id"], + ["datasources.id"], + name=op.f("coveragerecords_data_source_id_fkey"), + ), + sa.ForeignKeyConstraint( + ["identifier_id"], + ["identifiers.id"], + name=op.f("coveragerecords_identifier_id_fkey"), + ), + sa.PrimaryKeyConstraint("id", name=op.f("coveragerecords_pkey")), + ) + op.create_index( + "ix_identifier_id_data_source_id_operation_collection_id", + "coveragerecords", + ["identifier_id", "data_source_id", "operation", "collection_id"], + unique=True, + ) + op.create_index( + "ix_identifier_id_data_source_id_operation", + "coveragerecords", + ["identifier_id", "data_source_id", "operation"], + unique=True, + postgresql_where=sa.text("collection_id IS NULL"), + ) + op.create_index( + op.f("ix_coveragerecords_timestamp"), + "coveragerecords", + ["timestamp"], + unique=False, + ) + op.create_index( + op.f("ix_coveragerecords_status"), "coveragerecords", ["status"], unique=False + ) + op.create_index( + op.f("ix_coveragerecords_identifier_id"), + "coveragerecords", + ["identifier_id"], + unique=False, + ) + op.create_index( + op.f("ix_coveragerecords_exception"), + "coveragerecords", + ["exception"], + unique=False, + ) + op.create_index( + op.f("ix_coveragerecords_data_source_id_operation_identifier_id"), + "coveragerecords", + ["data_source_id", "operation", "identifier_id"], + unique=False, + ) + + op.create_table( + "equivalentscoveragerecords", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("equivalency_id", sa.Integer(), nullable=False), + sa.Column("operation", sa.String(length=255), nullable=True), + sa.Column("timestamp", sa.DateTime(timezone=True), nullable=True), + sa.Column("status", coverage_status, nullable=True), + sa.Column("exception", sa.Unicode(), nullable=True), + sa.ForeignKeyConstraint( + ["equivalency_id"], + ["equivalents.id"], + name=op.f("equivalentscoveragerecords_equivalency_id_fkey"), + ondelete="CASCADE", + ), + sa.PrimaryKeyConstraint("id", name=op.f("equivalentscoveragerecords_pkey")), + sa.UniqueConstraint( + "equivalency_id", + "operation", + name=op.f("equivalentscoveragerecords_equivalency_id_operation_key"), + ), + ) + op.create_index( + op.f("ix_equivalentscoveragerecords_timestamp"), + "equivalentscoveragerecords", + ["timestamp"], + unique=False, + ) + op.create_index( + op.f("ix_equivalentscoveragerecords_status"), + "equivalentscoveragerecords", + ["status"], + unique=False, + ) + op.create_index( + op.f("ix_equivalentscoveragerecords_operation"), + "equivalentscoveragerecords", + ["operation"], + unique=False, + ) + op.create_index( + op.f("ix_equivalentscoveragerecords_equivalency_id"), + "equivalentscoveragerecords", + ["equivalency_id"], + unique=False, + ) diff --git a/src/palace/manager/sqlalchemy/model/coverage.py b/src/palace/manager/sqlalchemy/model/coverage.py index eb02302ac1..041492e477 100644 --- a/src/palace/manager/sqlalchemy/model/coverage.py +++ b/src/palace/manager/sqlalchemy/model/coverage.py @@ -1,4 +1,4 @@ -# BaseCoverageRecord, Timestamp, CoverageRecord +# BaseCoverageRecord, Timestamp from __future__ import annotations import datetime @@ -11,7 +11,6 @@ DateTime, Enum, ForeignKey, - Index, Integer, String, Unicode, @@ -306,95 +305,3 @@ def recording(self) -> Generator[Self]: self.finish = utc_now() __table_args__ = (UniqueConstraint("service", "collection_id"),) - - -class CoverageRecord(Base, BaseCoverageRecord): - """A record of a Identifier being used as input into some process. - - Dormant: nothing reads or writes this table. The class is retained only so - ``create_all`` keeps emitting the table for freshly initialized databases, - which N-1 app servers still expect; it and the table are removed together in - a follow-up PR. It is deliberately just a table definition -- without the - parent relationships, a row written here could not be cleaned up when its - Identifier, DataSource or Collection is deleted, and would instead make that - delete fail on a foreign key. - """ - - __tablename__ = "coveragerecords" - - id: Mapped[int] = Column(Integer, primary_key=True) - identifier_id = Column(Integer, ForeignKey("identifiers.id"), index=True) - - # If applicable, this is the ID of the data source that took the - # Identifier as input. - data_source_id = Column(Integer, ForeignKey("datasources.id")) - operation = Column(String(255), default=None) - - timestamp = Column(DateTime(timezone=True), index=True) - - status = Column(BaseCoverageRecord.status_enum, index=True) - exception = Column(Unicode, index=True) - - # If applicable, this is the ID of the collection for which - # coverage has taken place. This is currently only applicable - # for Metadata Wrangler coverage. - collection_id = Column(Integer, ForeignKey("collections.id"), nullable=True) - - __table_args__ = ( - Index( - "ix_identifier_id_data_source_id_operation", - identifier_id, - data_source_id, - operation, - unique=True, - postgresql_where=collection_id.is_(None), - ), - Index( - "ix_identifier_id_data_source_id_operation_collection_id", - identifier_id, - data_source_id, - operation, - collection_id, - unique=True, - ), - ) - - -Index( - "ix_coveragerecords_data_source_id_operation_identifier_id", - CoverageRecord.data_source_id, - CoverageRecord.operation, - CoverageRecord.identifier_id, -) - - -class EquivalencyCoverageRecord(Base, BaseCoverageRecord): - """Dormant model retained only so the ``equivalentscoveragerecords`` table - stays in the schema for one more release. - - The equivalent-identifiers refresh no longer reads or writes this table — it - was replaced by the Redis dirty-set queue and the ``equivalent_identifiers_refresh`` - Celery task. But per our online-migration convention the table cannot be dropped - in the same release that stops using it: during a rolling deploy, N-1 app servers - still run the old listener that writes here, so dropping the table now would make - them error. The table and this model will be removed in a follow-up PR that ships - after this release. See https://github.com/ThePalaceProject/circulation/pull/3459. - """ - - __tablename__ = "equivalentscoveragerecords" - - id: Mapped[int] = Column(Integer, primary_key=True) - - equivalency_id: Mapped[int] = Column( - Integer, - ForeignKey("equivalents.id", ondelete="CASCADE"), - index=True, - nullable=False, - ) - - operation = Column(String(255), index=True, default=None) - timestamp = Column(DateTime(timezone=True), index=True) - status = Column(BaseCoverageRecord.status_enum, index=True) - exception = Column(Unicode) - - __table_args__ = (UniqueConstraint(equivalency_id, operation),) diff --git a/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py b/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py index 9bfee20618..c832a90b39 100644 --- a/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py +++ b/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py @@ -67,20 +67,30 @@ def test_empties_coveragerecords( assert _coveragerecord_count(alembic_engine) == 1 -def test_upgrade_does_not_leak_lock_timeout(alembic_engine: Engine) -> None: +def test_upgrade_does_not_leak_lock_timeout( + alembic_runner: MigrationContext, + alembic_engine: Engine, +) -> None: """upgrade() leaves lock_timeout exactly as it found it. The TRUNCATE runs under a short ``lock_timeout``, set with ``SET LOCAL``, which lasts to the end of the *transaction* rather than the end of this revision. ``alembic/env.py`` runs every pending revision inside a single ``context.begin_transaction()``, so without an explicit reset the timeout - would silently apply to every later revision in the same upgrade. + would silently apply to every later revision in the same upgrade. That is + not hypothetical after this release: a database older than this revision + upgrades through it and the following drop in one transaction. Driving the migration through ``alembic_runner`` could not catch that: it commits between revisions, which discards the setting regardless of what the migration did. So run ``upgrade()`` against a transaction we hold open ourselves, which is the situation env.py actually creates. """ + # The next revision drops coveragerecords, so it is absent from the schema + # built from the current models. Step back to this revision, whose + # downgrade recreates it, to give the TRUNCATE something to act on. + alembic_runner.migrate_down_to(REVISION) + revision = _load_revision() with alembic_engine.begin() as connection: diff --git a/tests/migration/test_20260916_5ca948100902_drop_coverage_tables.py b/tests/migration/test_20260916_5ca948100902_drop_coverage_tables.py new file mode 100644 index 0000000000..f744edb9fd --- /dev/null +++ b/tests/migration/test_20260916_5ca948100902_drop_coverage_tables.py @@ -0,0 +1,38 @@ +from __future__ import annotations + +from pytest_alembic import MigrationContext +from sqlalchemy import inspect +from sqlalchemy.engine import Engine + +REVISION = "5ca948100902" +DOWN_REVISION = "58ebd34c5092" + + +def test_drop_coverage_tables( + alembic_runner: MigrationContext, + alembic_engine: Engine, +) -> None: + """The migration drops coveragerecords and equivalentscoveragerecords. + + Stepping the migration down recreates both tables (and the shared + ``coverage_status`` enum); stepping it back up drops them again while + leaving the unrelated ``timestamps`` table in place. + """ + alembic_runner.migrate_down_to(REVISION) + # Step down once more so the tables exist again. + alembic_runner.migrate_down_one() + assert alembic_runner.current == DOWN_REVISION + + tables = set(inspect(alembic_engine).get_table_names()) + assert "coveragerecords" in tables + assert "equivalentscoveragerecords" in tables + + # Apply the drop. + alembic_runner.migrate_up_one() + assert alembic_runner.current == REVISION + + tables = set(inspect(alembic_engine).get_table_names()) + assert "coveragerecords" not in tables + assert "equivalentscoveragerecords" not in tables + # The unrelated timestamps table is left in place. + assert "timestamps" in tables From 325e5db812c9ef87d60464fb0a5f89393472e805 Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 29 Jun 2026 17:23:39 -0700 Subject: [PATCH 2/2] Remove the now-orphaned BaseCoverageRecord mixin With the CoverageProvider machinery and the CoverageRecord / EquivalencyCoverageRecord models gone, BaseCoverageRecord had no remaining users except the workcoveragerecords-removal migration (01b1e464a9d1), which imported it only for the coverage_status enum values. Inline those values in that migration so it is self-contained, then drop the mixin. coverage.py now contains only the Timestamp model. Co-Authored-By: Claude Opus 4.8 --- ...01b1e464a9d1_remove_work_coverage_table.py | 9 +++++---- .../manager/sqlalchemy/model/coverage.py | 19 +------------------ 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/alembic/versions/20250528_01b1e464a9d1_remove_work_coverage_table.py b/alembic/versions/20250528_01b1e464a9d1_remove_work_coverage_table.py index 0db4b218ac..cf36686730 100644 --- a/alembic/versions/20250528_01b1e464a9d1_remove_work_coverage_table.py +++ b/alembic/versions/20250528_01b1e464a9d1_remove_work_coverage_table.py @@ -12,8 +12,6 @@ from sqlalchemy import DateTime, ForeignKey, Integer, String, Unicode from sqlalchemy.dialects import postgresql -from palace.manager.sqlalchemy.model.coverage import BaseCoverageRecord - # revision identifiers, used by Alembic. revision = "01b1e464a9d1" down_revision = "d671b95566fb" @@ -80,8 +78,11 @@ def downgrade() -> None: sa.Column( "status", postgresql.ENUM( - *BaseCoverageRecord.status_enum.enums, # type: ignore[attr-defined] - name=BaseCoverageRecord.status_enum.name, + "success", + "transient failure", + "persistent failure", + "registered", + name="coverage_status", create_type=False, ), index=True, diff --git a/src/palace/manager/sqlalchemy/model/coverage.py b/src/palace/manager/sqlalchemy/model/coverage.py index 041492e477..f973c323f1 100644 --- a/src/palace/manager/sqlalchemy/model/coverage.py +++ b/src/palace/manager/sqlalchemy/model/coverage.py @@ -1,4 +1,4 @@ -# BaseCoverageRecord, Timestamp +# Timestamp from __future__ import annotations import datetime @@ -29,23 +29,6 @@ from palace.manager.sqlalchemy.model.collection import Collection -class BaseCoverageRecord: - """Holds the ``coverage_status`` enum shared by the two dormant coverage models.""" - - SUCCESS = "success" - TRANSIENT_FAILURE = "transient failure" - PERSISTENT_FAILURE = "persistent failure" - REGISTERED = "registered" - - status_enum = Enum( - SUCCESS, - TRANSIENT_FAILURE, - PERSISTENT_FAILURE, - REGISTERED, - name="coverage_status", - ) - - class Timestamp(Base): """Tracks the activities of Monitors, CoverageProviders, and general scripts.