From 599915f8763e00026ee7042fa45703f1788fadcd Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Wed, 16 Sep 2026 13:35:02 -0700 Subject: [PATCH 1/2] Stop using the coveragerecords table The CoverageProvider machinery that read and wrote coveragerecords was retired last release, but the table was not actually dormant: Identifier, DataSource and Collection still mapped a coverage_records relationship. SQLAlchemy loads a relationship whenever its parent is deleted -- to cascade the delete, or to null the child's foreign key -- so every session.delete() on one of those parents still SELECTed from coveragerecords. Dropping the table while those relationships existed would break N-1 app servers during a rolling deploy. - Remove the coverage_records relationships from Identifier, DataSource and Collection, and the matching back_populates on CoverageRecord. - Empty coveragerecords in a migration. Its foreign keys carry no ON DELETE clause, so with the relationships gone a surviving row would make deleting its parent fail; the rows are dead data, so TRUNCATE is cheaper than teaching a soon-to-be-dropped table to cascade. The TRUNCATE runs under a lock_timeout: N-1 servers still read the table, so its ACCESS EXCLUSIVE lock can queue behind an in-flight parent delete and stall every later reader -- failing fast and retrying beats blocking instance startup. The timeout is reset right afterwards: SET LOCAL lasts to the end of the transaction, and env.py runs every pending revision in one transaction, so without the reset later revisions in the same upgrade would inherit a timeout they never asked for. - Reduce CoverageRecord to a bare table definition. Nothing called lookup, add_for, bulk_add or the helpers around them, and with the parent relationships gone a row they wrote could no longer be cleaned up when its parent is deleted -- it would just make that delete fail on a foreign key. Dropping them removes the footgun, along with BaseCoverageRecord.not_covered and the constants that only fed it. - Remove the CoverageRecord tests and the db.coverage_record fixture, so the next release's backwards-compatibility gate does not run them against a schema where the table is gone. The CoverageRecord and EquivalencyCoverageRecord models stay for one more release: a fresh database's schema is built with create_all from the models rather than by replaying migrations, so deleting the classes now would drop the table out from under N-1 servers on new installs. The models and the tables are removed together in the stacked follow-up. equivalentscoveragerecords needs no such change -- it has no ORM relationships and its one foreign key already declares ON DELETE CASCADE. Co-Authored-By: Claude Opus 5 --- ...mpty_coveragerecords_before_dropping_it.py | 66 ++++ .../manager/sqlalchemy/model/collection.py | 8 +- .../manager/sqlalchemy/model/coverage.py | 328 ++---------------- .../manager/sqlalchemy/model/datasource.py | 6 - .../manager/sqlalchemy/model/identifier.py | 6 - tests/fixtures/database.py | 29 -- .../manager/data_layer/test_bibliographic.py | 17 - .../sqlalchemy/model/test_collection.py | 14 - .../manager/sqlalchemy/model/test_coverage.py | 315 ----------------- ...0916_58ebd34c5092_empty_coveragerecords.py | 45 +++ 10 files changed, 139 insertions(+), 695 deletions(-) create mode 100644 alembic/versions/20260916_58ebd34c5092_empty_coveragerecords_before_dropping_it.py create mode 100644 tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py diff --git a/alembic/versions/20260916_58ebd34c5092_empty_coveragerecords_before_dropping_it.py b/alembic/versions/20260916_58ebd34c5092_empty_coveragerecords_before_dropping_it.py new file mode 100644 index 0000000000..ef7ace2a7c --- /dev/null +++ b/alembic/versions/20260916_58ebd34c5092_empty_coveragerecords_before_dropping_it.py @@ -0,0 +1,66 @@ +"""Empty coveragerecords before dropping it + +The ``coverage_records`` relationships on Identifier, DataSource and Collection are +removed in this release, which is what finally stops the application reading the +``coveragerecords`` table. Those relationships were also the only thing keeping parent +deletes working: ``coveragerecords`` has plain foreign keys to ``identifiers``, +``datasources`` and ``collections`` with no ``ON DELETE`` clause, so SQLAlchemy had to +load the children and cascade (or null their FK) by hand. With the relationships gone +nothing does that any more, and any surviving row would make deleting its parent fail +with a foreign-key violation. + +The rows are dead data -- the CoverageProvider machinery that wrote them was retired a +release ago -- so we empty the table rather than add ``ON DELETE`` clauses to a table +that is dropped in the next release. + +TRUNCATE rather than DELETE: the table can hold tens of millions of rows, and TRUNCATE +reclaims them in constant time without generating row-level WAL. Nothing references +``coveragerecords``, so no CASCADE is needed. + +The ACCESS EXCLUSIVE lock TRUNCATE takes is *not* guaranteed to be uncontended: N-1 +servers still map the ``coverage_records`` relationships this release removes, so they +take an ACCESS SHARE lock on the table whenever they delete an Identifier, DataSource +or Collection -- that is the very behaviour this release exists to stop. If such a +delete is in flight the TRUNCATE waits, and a waiting ACCESS EXCLUSIVE request queues +ahead of every later reader, so the whole table stalls behind it. The wait should be +short, but ``lock_timeout`` bounds it: the migration fails fast and is retried rather +than blocking instance startup. + +The timeout is reset immediately afterwards. ``SET LOCAL`` lasts to the end of the +*transaction*, not the end of this revision, and ``alembic/env.py`` runs every pending +revision inside a single ``context.begin_transaction()`` (it does not set +``transaction_per_migration``). Without the reset, any revision applied after this one +in the same ``alembic upgrade`` -- the stacked drop in the next release, or anything +added before this one ships -- would silently inherit a 5s lock timeout it never asked +for, and could roll the whole upgrade back while waiting on a busy table. + +``equivalentscoveragerecords`` deliberately gets no such treatment: it has no ORM +relationships pointing at it, and its one foreign key already declares +``ON DELETE CASCADE``, so the database cleans it up on its own. + +Revision ID: 58ebd34c5092 +Revises: 5b1f4f3c7979 +Create Date: 2026-09-16 20:32:09.926471+00:00 + +""" + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "58ebd34c5092" +down_revision = "5b1f4f3c7979" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.execute("SET LOCAL lock_timeout = '5s'") + op.execute("TRUNCATE TABLE coveragerecords") + # Scope the timeout to the statement above; see the module docstring. + op.execute("SET LOCAL lock_timeout = DEFAULT") + + +def downgrade() -> None: + # The rows are gone for good; emptying a dead table is not reversible. The + # downgrade is a no-op so the migration can still be stepped back over. + pass diff --git a/src/palace/manager/sqlalchemy/model/collection.py b/src/palace/manager/sqlalchemy/model/collection.py index 1e854cd2b2..828b094d24 100644 --- a/src/palace/manager/sqlalchemy/model/collection.py +++ b/src/palace/manager/sqlalchemy/model/collection.py @@ -34,7 +34,7 @@ from palace.manager.sqlalchemy.hassessioncache import HasSessionCache from palace.manager.sqlalchemy.hybrid import hybrid_property from palace.manager.sqlalchemy.model.base import Base -from palace.manager.sqlalchemy.model.coverage import CoverageRecord, Timestamp +from palace.manager.sqlalchemy.model.coverage import Timestamp from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.identifier import Identifier from palace.manager.sqlalchemy.model.integration import ( @@ -133,12 +133,6 @@ class Collection(Base, HasSessionCache, RedisKeyMixin): "Identifier", secondary="collections_identifiers", back_populates="collections" ) - # A Collection can be associated with multiple CoverageRecords - # for Identifiers in its catalog. - coverage_records: Mapped[list[CoverageRecord]] = relationship( - "CoverageRecord", back_populates="collection", cascade="all" - ) - # A collection may be associated with one or more custom lists. # When a new license pool is added to the collection, it will # also be added to the list. Admins can remove items from the diff --git a/src/palace/manager/sqlalchemy/model/coverage.py b/src/palace/manager/sqlalchemy/model/coverage.py index d21c3a7106..84b04994fd 100644 --- a/src/palace/manager/sqlalchemy/model/coverage.py +++ b/src/palace/manager/sqlalchemy/model/coverage.py @@ -19,7 +19,6 @@ ) from sqlalchemy.orm import Mapped, relationship from sqlalchemy.orm.session import Session -from sqlalchemy.sql.expression import and_, literal, literal_column, or_ from palace.util.datetime_helpers import utc_now @@ -29,28 +28,20 @@ if TYPE_CHECKING: from palace.manager.sqlalchemy.model.collection import Collection - from palace.manager.sqlalchemy.model.datasource import DataSource - from palace.manager.sqlalchemy.model.identifier import Identifier class BaseCoverageRecord: - """Contains useful constants used by CoverageRecord.""" + """Holds the ``coverage_status`` enum shared by the two dormant coverage models. + + Everything else on this mixin went with the queries it supported; it is + removed along with those models and their tables in the follow-up PR. + """ SUCCESS = "success" TRANSIENT_FAILURE = "transient failure" PERSISTENT_FAILURE = "persistent failure" REGISTERED = "registered" - ALL_STATUSES = [REGISTERED, SUCCESS, TRANSIENT_FAILURE, PERSISTENT_FAILURE] - - # Count coverage as attempted if the record is not 'registered'. - PREVIOUSLY_ATTEMPTED = [SUCCESS, TRANSIENT_FAILURE, PERSISTENT_FAILURE] - - # By default, count coverage as present if it ended in - # success or in persistent failure. Do not count coverage - # as present if it ended in transient failure. - DEFAULT_COUNT_AS_COVERED = [SUCCESS, PERSISTENT_FAILURE] - status_enum = Enum( SUCCESS, TRANSIENT_FAILURE, @@ -59,44 +50,6 @@ class BaseCoverageRecord: name="coverage_status", ) - @classmethod - def not_covered( - cls, count_as_covered=None, count_as_not_covered_if_covered_before=None - ): - """Filter a query to find only items without coverage records. - - :param count_as_covered: A list of constants that indicate - types of coverage records that should count as 'coverage' - for purposes of this query. - :param count_as_not_covered_if_covered_before: If a coverage record - exists, but is older than the given date, do not count it as - covered. - :return: A clause that can be passed in to Query.filter(). - """ - - if not count_as_covered: - count_as_covered = cls.DEFAULT_COUNT_AS_COVERED - elif isinstance(count_as_covered, (bytes, str)): - count_as_covered = [count_as_covered] - - # If there is no coverage record, then of course the item is - # not covered. - missing = cls.id == None - - # If we're looking for specific coverage statuses, then a - # record does not count if it has some other status. - missing = or_(missing, ~cls.status.in_(count_as_covered)) - - # If the record's timestamp is before the cutoff time, we - # don't count it as covered, regardless of which status it - # has. - if count_as_not_covered_if_covered_before: - missing = or_( - missing, cls.timestamp < count_as_not_covered_if_covered_before - ) - - return missing - class Timestamp(Base): """Tracks the activities of Monitors, CoverageProviders, @@ -364,33 +317,38 @@ class CoverageRecord(Base, BaseCoverageRecord): Dormant model retained only so the ``coveragerecords`` table stays in the schema for one more release. The CoverageProvider machinery that read and - wrote these records has been retired; nothing in the current code reads or - writes this table. Per our online-migration convention the table cannot be - dropped in the same release that stops using it (N-1 app servers still write - here during a rolling deploy), so this model and the table will be removed in - a follow-up PR that ships after this release. + wrote these records has been retired, and the ``coverage_records`` + relationships on Identifier, DataSource and Collection have been removed, so + nothing in the current code reads or writes this table. + + Removing those relationships is what actually stops the reads: a mapped + relationship is loaded by SQLAlchemy whenever its parent is deleted (to + cascade the delete, or to null the child's foreign key), so while they + existed every ``session.delete(collection)`` still SELECTed from this table. + + The model is kept for one more release because the schema of a freshly + initialized database is built with ``create_all`` from these models, not by + replaying migrations -- dropping the class would remove the table from new + installs immediately, while N-1 app servers still expect it. The model and + the table are removed together in a follow-up PR that ships after this + release. + + Only the columns remain. The query and write helpers (``lookup``, ``add_for``, + ``bulk_add`` and friends) are gone: nothing called them, and with the parent + relationships removed a row they wrote could no longer be cleaned up when its + Identifier, DataSource or Collection is deleted -- it would just make that + delete fail on a foreign key. Keeping the class a bare table definition makes + it impossible to write such a row. """ __tablename__ = "coveragerecords" - REAP_OPERATION = "reap" - IMPORT_OPERATION = "import" - RESOLVE_IDENTIFIER_OPERATION = "resolve-identifier" - REPAIR_SORT_NAME_OPERATION = "repair-sort-name" - METADATA_UPLOAD_OPERATION = "metadata-upload" - id: Mapped[int] = Column(Integer, primary_key=True) identifier_id = Column(Integer, ForeignKey("identifiers.id"), index=True) - identifier: Mapped[Identifier | None] = relationship( - "Identifier", back_populates="coverage_records" - ) # If applicable, this is the ID of the data source that took the # Identifier as input. data_source_id = Column(Integer, ForeignKey("datasources.id")) - data_source: Mapped[DataSource | None] = relationship( - "DataSource", back_populates="coverage_records" - ) operation = Column(String(255), default=None) timestamp = Column(DateTime(timezone=True), index=True) @@ -402,9 +360,6 @@ class CoverageRecord(Base, BaseCoverageRecord): # coverage has taken place. This is currently only applicable # for Metadata Wrangler coverage. collection_id = Column(Integer, ForeignKey("collections.id"), nullable=True) - collection: Mapped[Collection | None] = relationship( - "Collection", back_populates="coverage_records" - ) __table_args__ = ( Index( @@ -425,235 +380,6 @@ class CoverageRecord(Base, BaseCoverageRecord): ), ) - def __repr__(self): - template = '' - return self.human_readable(template) - - def human_readable(self, template): - """Interpolate data into a human-readable template.""" - if self.operation: - operation = ' operation="%s"' % self.operation - else: - operation = "" - if self.exception: - exception = ' exception="%s"' % self.exception - else: - exception = "" - return template % dict( - timestamp=self.timestamp.strftime("%Y-%m-%d %H:%M:%S"), - identifier_type=self.identifier.type, - identifier=self.identifier.identifier, - data_source=self.data_source.name, - operation=operation, - status=self.status, - exception=exception, - ) - - @classmethod - def assert_coverage_operation(cls, operation, collection): - if operation == CoverageRecord.IMPORT_OPERATION and not collection: - raise ValueError( - "An 'import' type coverage must be associated with a collection" - ) - - @classmethod - def lookup( - cls, edition_or_identifier, data_source, operation=None, collection=None - ): - from palace.manager.sqlalchemy.model.datasource import DataSource - from palace.manager.sqlalchemy.model.edition import Edition - from palace.manager.sqlalchemy.model.identifier import Identifier - - cls.assert_coverage_operation(operation, collection) - - _db = Session.object_session(edition_or_identifier) - if isinstance(edition_or_identifier, Identifier): - identifier = edition_or_identifier - elif isinstance(edition_or_identifier, Edition): - identifier = edition_or_identifier.primary_identifier - else: - raise ValueError( - "Cannot look up a coverage record for %r." % edition_or_identifier - ) - - if isinstance(data_source, (bytes, str)): - data_source = DataSource.lookup(_db, data_source) - - return get_one( - _db, - CoverageRecord, - identifier=identifier, - data_source=data_source, - operation=operation, - collection=collection, - on_multiple="interchangeable", - ) - - @classmethod - def add_for( - cls, - edition, - data_source, - operation=None, - timestamp=None, - status=BaseCoverageRecord.SUCCESS, - collection=None, - ): - from palace.manager.sqlalchemy.model.edition import Edition - from palace.manager.sqlalchemy.model.identifier import Identifier - - cls.assert_coverage_operation(operation, collection) - - _db = Session.object_session(edition) - if isinstance(edition, Identifier): - identifier = edition - elif isinstance(edition, Edition): - identifier = edition.primary_identifier - else: - raise ValueError("Cannot create a coverage record for %r." % edition) - timestamp = timestamp or utc_now() - coverage_record, is_new = get_one_or_create( - _db, - CoverageRecord, - identifier=identifier, - data_source=data_source, - operation=operation, - collection=collection, - on_multiple="interchangeable", - ) - coverage_record.status = status - coverage_record.timestamp = timestamp - return coverage_record, is_new - - @classmethod - def bulk_add( - cls, - identifiers, - data_source, - operation=None, - timestamp=None, - status=BaseCoverageRecord.SUCCESS, - exception=None, - collection=None, - force=False, - ): - """Create and update CoverageRecords so that every Identifier in - `identifiers` has an identical record. - """ - from palace.manager.sqlalchemy.model.identifier import Identifier - - if not identifiers: - # Nothing to do. - return - - cls.assert_coverage_operation(operation, collection) - - _db = Session.object_session(identifiers[0]) - timestamp = timestamp or utc_now() - identifier_ids = [i.id for i in identifiers] - - equivalent_record = and_( - cls.operation == operation, - cls.data_source == data_source, - cls.collection == collection, - ) - - updated_or_created_results = list() - if force: - # Make sure that works that previously had a - # CoverageRecord for this operation have their timestamp - # and status updated. - update = ( - cls.__table__.update() - .where( - and_( - cls.identifier_id.in_(identifier_ids), - equivalent_record, - ) - ) - .values(dict(timestamp=timestamp, status=status, exception=exception)) - .returning(cls.id, cls.identifier_id) - ) - updated_or_created_results = _db.execute(update).fetchall() - - already_covered = ( - _db.query(cls.id, cls.identifier_id) - .filter( - equivalent_record, - cls.identifier_id.in_(identifier_ids), - ) - .subquery() - ) - - # Make sure that any identifiers that need a CoverageRecord get one. - # The SELECT part of the INSERT...SELECT query. - data_source_id = data_source.id - collection_id = None - if collection: - collection_id = collection.id - - new_records = ( - _db.query( - Identifier.id.label("identifier_id"), - literal(operation, type_=String(255)).label("operation"), - literal(timestamp, type_=DateTime).label("timestamp"), - literal(status, type_=BaseCoverageRecord.status_enum).label("status"), - literal(exception, type_=Unicode).label("exception"), - literal(data_source_id, type_=Integer).label("data_source_id"), - literal(collection_id, type_=Integer).label("collection_id"), - ) - .select_from(Identifier) - .outerjoin( - already_covered, - Identifier.id == already_covered.c.identifier_id, - ) - .filter(already_covered.c.id == None) - ) - - new_records = new_records.filter(Identifier.id.in_(identifier_ids)) - - # The INSERT part. - insert = ( - cls.__table__.insert() - .from_select( - [ - literal_column("identifier_id"), - literal_column("operation"), - literal_column("timestamp"), - literal_column("status"), - literal_column("exception"), - literal_column("data_source_id"), - literal_column("collection_id"), - ], - new_records, - ) - .returning(cls.id, cls.identifier_id) - ) - - inserts = _db.execute(insert).fetchall() - - updated_or_created_results.extend(inserts) - _db.commit() - - # Default return for the case when all of the identifiers were - # ignored. - new_records = list() - ignored_identifiers = identifiers - - new_and_updated_record_ids = [r[0] for r in updated_or_created_results] - impacted_identifier_ids = [r[1] for r in updated_or_created_results] - - if new_and_updated_record_ids: - new_records = ( - _db.query(cls).filter(cls.id.in_(new_and_updated_record_ids)).all() - ) - - ignored_identifiers = [ - i for i in identifiers if i.id not in impacted_identifier_ids - ] - - return new_records, ignored_identifiers - Index( "ix_coveragerecords_data_source_id_operation_identifier_id", diff --git a/src/palace/manager/sqlalchemy/model/datasource.py b/src/palace/manager/sqlalchemy/model/datasource.py index 96f40d2385..89fb2a7851 100644 --- a/src/palace/manager/sqlalchemy/model/datasource.py +++ b/src/palace/manager/sqlalchemy/model/datasource.py @@ -17,7 +17,6 @@ # This is needed during type checking so we have the # types of related models. from palace.manager.sqlalchemy.model.classification import Classification - from palace.manager.sqlalchemy.model.coverage import CoverageRecord from palace.manager.sqlalchemy.model.credential import Credential from palace.manager.sqlalchemy.model.customlist import CustomList from palace.manager.sqlalchemy.model.edition import Edition @@ -54,11 +53,6 @@ def active_name(self) -> str: "Edition", back_populates="data_source", uselist=True ) - # One DataSource can generate many CoverageRecords. - coverage_records: Mapped[list[CoverageRecord]] = relationship( - "CoverageRecord", back_populates="data_source" - ) - # One DataSource can generate many IDEquivalencies. id_equivalencies: Mapped[list[Equivalency]] = relationship( "Equivalency", back_populates="data_source" diff --git a/src/palace/manager/sqlalchemy/model/identifier.py b/src/palace/manager/sqlalchemy/model/identifier.py index 8436eb446b..be81fdc992 100644 --- a/src/palace/manager/sqlalchemy/model/identifier.py +++ b/src/palace/manager/sqlalchemy/model/identifier.py @@ -41,7 +41,6 @@ from palace.manager.sqlalchemy.constants import IdentifierConstants, LinkRelations from palace.manager.sqlalchemy.model.base import Base from palace.manager.sqlalchemy.model.classification import Classification, Subject -from palace.manager.sqlalchemy.model.coverage import CoverageRecord from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.licensing import ( LicensePool, @@ -260,11 +259,6 @@ def active_type(self) -> str: uselist=True, ) - # One Identifier may have many associated CoverageRecords. - coverage_records: Mapped[list[CoverageRecord]] = relationship( - "CoverageRecord", back_populates="identifier" - ) - def __repr__(self) -> str: records = self.primarily_identifies if records and records[0].title: diff --git a/tests/fixtures/database.py b/tests/fixtures/database.py index 7769a573f5..9fe323a77a 100644 --- a/tests/fixtures/database.py +++ b/tests/fixtures/database.py @@ -84,7 +84,6 @@ ) from palace.manager.sqlalchemy.model.collection import Collection from palace.manager.sqlalchemy.model.contributor import Contributor -from palace.manager.sqlalchemy.model.coverage import CoverageRecord from palace.manager.sqlalchemy.model.credential import Credential from palace.manager.sqlalchemy.model.customlist import CustomList from palace.manager.sqlalchemy.model.datasource import DataSource @@ -1079,34 +1078,6 @@ def subject(self, type, identifier) -> Subject: self.session, Subject, type=type, identifier=identifier )[0] - def coverage_record( - self, - edition, - coverage_source, - operation=None, - status=CoverageRecord.SUCCESS, - collection=None, - exception=None, - ) -> CoverageRecord: - if isinstance(edition, Identifier): - identifier = edition - else: - identifier = edition.primary_identifier - record, ignore = get_one_or_create( - self.session, - CoverageRecord, - identifier=identifier, - data_source=coverage_source, - operation=operation, - collection=collection, - create_method_kwargs=dict( - timestamp=utc_now(), - status=status, - exception=exception, - ), - ) - return record - def identifier(self, identifier_type=Identifier.GUTENBERG_ID, foreign_id=None): if foreign_id: id_value = foreign_id diff --git a/tests/manager/data_layer/test_bibliographic.py b/tests/manager/data_layer/test_bibliographic.py index e42132c0b0..098ce8d728 100644 --- a/tests/manager/data_layer/test_bibliographic.py +++ b/tests/manager/data_layer/test_bibliographic.py @@ -6,7 +6,6 @@ import pytest from freezegun import freeze_time -from sqlalchemy import select from palace.util.datetime_helpers import utc_now from palace.util.exceptions import PalaceValueError @@ -25,7 +24,6 @@ from palace.manager.data_layer.subject import SubjectData from palace.manager.sqlalchemy.model.classification import Subject from palace.manager.sqlalchemy.model.contributor import Contributor -from palace.manager.sqlalchemy.model.coverage import CoverageRecord from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.edition import Edition from palace.manager.sqlalchemy.model.identifier import Identifier @@ -714,21 +712,6 @@ def test_apply_no_value(self, db: DatabaseTransactionFixture): assert edition_new.published == edition_old.published assert edition_new.issued == edition_old.issued - def test_apply_does_not_create_coverage_records( - self, db: DatabaseTransactionFixture - ): - edition, pool = db.edition(with_license_pool=True) - - bibliographic = BibliographicData( - data_source_name=DataSource.OVERDRIVE, title=db.fresh_str() - ) - - bibliographic.apply(db.session, edition, pool.collection) - - # No coverage records were created. - records = db.session.scalars(select(CoverageRecord)).all() - assert len(records) == 0 - def test_apply_no_changes_needed(self, db: DatabaseTransactionFixture): edition, pool = db.edition(with_license_pool=True) edition.title = "Old title" diff --git a/tests/manager/sqlalchemy/model/test_collection.py b/tests/manager/sqlalchemy/model/test_collection.py index 028fe98285..08daf65580 100644 --- a/tests/manager/sqlalchemy/model/test_collection.py +++ b/tests/manager/sqlalchemy/model/test_collection.py @@ -22,7 +22,6 @@ ) from palace.manager.sqlalchemy.model.circulationevent import CirculationEvent from palace.manager.sqlalchemy.model.collection import Collection -from palace.manager.sqlalchemy.model.coverage import CoverageRecord from palace.manager.sqlalchemy.model.customlist import CustomList from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.edition import Edition @@ -656,16 +655,6 @@ def test_delete( pool2 = db.licensepool(None, collection=collection2) work2.license_pools.append(pool2) - record, _ = CoverageRecord.add_for( - work.presentation_edition, collection.data_source, collection=collection - ) - assert ( - CoverageRecord.lookup( - work.presentation_edition, collection.data_source, collection=collection - ) - != None - ) - # If we're meant to test an inactive collection, make it inactive. if is_inactive: db.make_collection_inactive(collection) @@ -695,9 +684,6 @@ def test_delete( # The default library now has no collections. assert [] == library.associated_collections - # The collection based coverage record got deleted - assert db.session.get(CoverageRecord, record.id) == None - # The deletion of the Collection's sole LicensePool has # cascaded to Loan, Hold, License, and # CirculationEvent. diff --git a/tests/manager/sqlalchemy/model/test_coverage.py b/tests/manager/sqlalchemy/model/test_coverage.py index af15c463fc..ec4911fdc4 100644 --- a/tests/manager/sqlalchemy/model/test_coverage.py +++ b/tests/manager/sqlalchemy/model/test_coverage.py @@ -1,20 +1,14 @@ import datetime from unittest.mock import MagicMock -import pytest from freezegun import freeze_time from palace.util.datetime_helpers import datetime_utc, utc_now from palace.manager.core.monitor import TimestampData from palace.manager.sqlalchemy.model.coverage import ( - BaseCoverageRecord, - CoverageRecord, Timestamp, ) -from palace.manager.sqlalchemy.model.datasource import DataSource -from palace.manager.sqlalchemy.model.edition import Edition -from palace.manager.sqlalchemy.model.identifier import Identifier from palace.manager.util.sentinel import SentinelType from tests.fixtures.database import DatabaseTransactionFixture @@ -229,312 +223,3 @@ def test_recording(self) -> None: assert stamp.start == now assert stamp.finish == now + delta assert stamp.exception == "testing" - - -class TestBaseCoverageRecord: - def test_not_covered(self, db: DatabaseTransactionFixture): - source = DataSource.lookup(db.session, DataSource.OCLC) - - # Here are four identifiers with four relationships to a - # certain coverage provider: no coverage at all, successful - # coverage, a transient failure and a permanent failure. - - no_coverage = db.identifier() - - success = db.identifier() - success_record = db.coverage_record(success, source) - success_record.timestamp = utc_now() - datetime.timedelta(seconds=3600) - assert CoverageRecord.SUCCESS == success_record.status - - transient = db.identifier() - transient_record = db.coverage_record( - transient, source, status=CoverageRecord.TRANSIENT_FAILURE - ) - assert CoverageRecord.TRANSIENT_FAILURE == transient_record.status - - persistent = db.identifier() - persistent_record = db.coverage_record( - persistent, source, status=BaseCoverageRecord.PERSISTENT_FAILURE - ) - assert CoverageRecord.PERSISTENT_FAILURE == persistent_record.status - - # Here's a query that finds all four. - qu = db.session.query(Identifier).outerjoin(CoverageRecord) - assert 4 == qu.count() - - def check_not_covered(expect, **kwargs): - missing = CoverageRecord.not_covered(**kwargs) - assert sorted(expect) == sorted(qu.filter(missing).all()) - - # By default, not_covered() only finds the identifier with no - # coverage and the one with a transient failure. - check_not_covered([no_coverage, transient]) - - # If we pass in different values for covered_status, we change what - # counts as 'coverage'. In this case, we allow transient failures - # to count as 'coverage'. - check_not_covered( - [no_coverage], - count_as_covered=[ - CoverageRecord.PERSISTENT_FAILURE, - CoverageRecord.TRANSIENT_FAILURE, - CoverageRecord.SUCCESS, - ], - ) - - # Here, only success counts as 'coverage'. - check_not_covered( - [no_coverage, transient, persistent], - count_as_covered=CoverageRecord.SUCCESS, - ) - - # We can also say that coverage doesn't count if it was achieved before - # a certain time. Here, we'll show that passing in the timestamp - # of the 'success' record means that record still counts as covered. - check_not_covered( - [no_coverage, transient], - count_as_not_covered_if_covered_before=success_record.timestamp, - ) - - # But if we pass in a time one second later, the 'success' - # record no longer counts as covered. - assert isinstance(success_record.timestamp, datetime.datetime) - one_second_after = success_record.timestamp + datetime.timedelta(seconds=1) - check_not_covered( - [success, no_coverage, transient], - count_as_not_covered_if_covered_before=one_second_after, - ) - - -class TestCoverageRecord: - def test_lookup(self, db: DatabaseTransactionFixture): - source = DataSource.lookup(db.session, DataSource.OCLC) - edition = db.edition() - operation = "foo" - collection = db.default_collection() - record = db.coverage_record(edition, source, operation, collection=collection) - - # To find the CoverageRecord, edition, source, operation, - # and collection must all match. - result = CoverageRecord.lookup( - edition, source, operation, collection=collection - ) - assert record == result - - # You can substitute the Edition's primary identifier for the - # Edition iteslf. - lookup = CoverageRecord.lookup( - edition.primary_identifier, - source, - operation, - collection=db.default_collection(), - ) - assert lookup == record - - # Omit the collection, and you find nothing. - result = CoverageRecord.lookup(edition, source, operation) - assert None == result - - # Same for operation. - result = CoverageRecord.lookup(edition, source, collection=collection) - assert None == result - - result = CoverageRecord.lookup( - edition, source, "other operation", collection=collection - ) - assert None == result - - # Same for data source. - other_source = DataSource.lookup(db.session, DataSource.OVERDRIVE) - result = CoverageRecord.lookup( - edition, other_source, operation, collection=collection - ) - assert None == result - - def test_add_for(self, db: DatabaseTransactionFixture): - source = DataSource.lookup(db.session, DataSource.OCLC) - edition = db.edition() - operation = "foo" - record, is_new = CoverageRecord.add_for(edition, source, operation) - assert True == is_new - - # If we call add_for again we get the same record back, but we - # can modify the timestamp. - a_week_ago = utc_now() - datetime.timedelta(days=7) - record2, is_new = CoverageRecord.add_for(edition, source, operation, a_week_ago) - assert record == record2 - assert False == is_new - assert a_week_ago == record2.timestamp - - # If we don't specify an operation we get a totally different - # record. - record3, ignore = CoverageRecord.add_for(edition, source) - assert record3 != record - assert None == record3.operation - seconds = (utc_now() - record3.timestamp).seconds - assert seconds < 10 - - # If we call lookup we get the same record. - record4 = CoverageRecord.lookup(edition.primary_identifier, source) - assert record3 == record4 - - # We can change the status. - record5, is_new = CoverageRecord.add_for( - edition, source, operation, status=CoverageRecord.PERSISTENT_FAILURE - ) - assert record5 == record - assert CoverageRecord.PERSISTENT_FAILURE == record.status - - def test_bulk_add(self, db: DatabaseTransactionFixture): - source = DataSource.lookup(db.session, DataSource.GUTENBERG) - operation = "testing" - - # An untouched identifier. - i1 = db.identifier() - - # An identifier that already has failing coverage. - covered = db.identifier() - existing = db.coverage_record( - covered, - source, - operation=operation, - status=CoverageRecord.TRANSIENT_FAILURE, - exception="Uh oh", - ) - original_timestamp = existing.timestamp - - resulting_records, ignored_identifiers = CoverageRecord.bulk_add( - [i1, covered], source, operation=operation - ) - - # A new coverage record is created for the uncovered identifier. - assert i1.coverage_records == resulting_records - [new_record] = resulting_records - assert source == new_record.data_source - assert operation == new_record.operation - assert CoverageRecord.SUCCESS == new_record.status - assert None == new_record.exception - - # The existing coverage record is untouched. - assert [covered] == ignored_identifiers - assert [existing] == covered.coverage_records - assert CoverageRecord.TRANSIENT_FAILURE == existing.status - assert original_timestamp == existing.timestamp - assert "Uh oh" == existing.exception - - # Newly untouched identifier. - i2 = db.identifier() - - # Force bulk add. - resulting_records, ignored_identifiers = CoverageRecord.bulk_add( - [i2, covered], source, operation=operation, force=True - ) - - # The new identifier has the expected coverage. - [new_record] = i2.coverage_records - assert new_record in resulting_records - - # The existing record has been updated. - assert existing in resulting_records - assert covered not in ignored_identifiers - assert CoverageRecord.SUCCESS == existing.status - assert isinstance(existing.timestamp, datetime.datetime) - assert isinstance(original_timestamp, datetime.datetime) - assert existing.timestamp > original_timestamp - assert None == existing.exception - - # If no records are created or updated, no records are returned. - resulting_records, ignored_identifiers = CoverageRecord.bulk_add( - [i2, covered], source, operation=operation - ) - - assert [] == resulting_records - assert sorted([i2, covered]) == sorted(ignored_identifiers) - - def test_bulk_add_with_collection(self, db: DatabaseTransactionFixture): - source = DataSource.lookup(db.session, DataSource.GUTENBERG) - operation = "testing" - - c1 = db.collection() - c2 = db.collection() - - # An untouched identifier. - i1 = db.identifier() - - # An identifier with coverage for a different collection. - covered = db.identifier() - existing = db.coverage_record( - covered, - source, - operation=operation, - status=CoverageRecord.TRANSIENT_FAILURE, - collection=c1, - exception="Danger, Will Robinson", - ) - original_timestamp = existing.timestamp - - resulting_records, ignored_identifiers = CoverageRecord.bulk_add( - [i1, covered], source, operation=operation, collection=c1, force=True - ) - - assert 2 == len(resulting_records) - assert [] == ignored_identifiers - - # A new record is created for the new identifier. - [new_record] = i1.coverage_records - assert new_record in resulting_records - assert source == new_record.data_source - assert operation == new_record.operation - assert CoverageRecord.SUCCESS == new_record.status - assert c1 == new_record.collection - - # The existing record has been updated. - assert existing in resulting_records - assert CoverageRecord.SUCCESS == existing.status - assert isinstance(existing.timestamp, datetime.datetime) - assert isinstance(original_timestamp, datetime.datetime) - assert existing.timestamp > original_timestamp - assert None == existing.exception - - # Bulk add for a different collection. - resulting_records, ignored_identifiers = CoverageRecord.bulk_add( - [covered], - source, - operation=operation, - collection=c2, - status=CoverageRecord.TRANSIENT_FAILURE, - exception="Oh no", - ) - - # A new record has been added to the identifier. - assert existing not in resulting_records - [new_record] = resulting_records - assert covered == new_record.identifier - assert CoverageRecord.TRANSIENT_FAILURE == new_record.status - assert source == new_record.data_source - assert operation == new_record.operation - assert "Oh no" == new_record.exception - - def test_assert_coverage_operation(self, db: DatabaseTransactionFixture): - """Ensure all the methods that should raise errors, do raise the errors""" - edition: Edition = db.edition() - with pytest.raises(ValueError): - CoverageRecord.add_for( - edition, - edition.data_source, - CoverageRecord.IMPORT_OPERATION, - ) - - with pytest.raises(ValueError): - CoverageRecord.lookup( - edition, - edition.data_source, - CoverageRecord.IMPORT_OPERATION, - ) - - with pytest.raises(ValueError): - CoverageRecord.bulk_add( - [edition.primary_identifier], - edition.data_source, - CoverageRecord.IMPORT_OPERATION, - ) diff --git a/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py b/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py new file mode 100644 index 0000000000..0d6e9da70a --- /dev/null +++ b/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py @@ -0,0 +1,45 @@ +from __future__ import annotations + +from pytest_alembic import MigrationContext +from sqlalchemy import text +from sqlalchemy.engine import Engine + +REVISION = "58ebd34c5092" + + +def _coveragerecord_count(engine: Engine) -> int: + with engine.begin() as connection: + return connection.execute( + text("SELECT count(*) FROM coveragerecords") + ).scalar_one() + + +def test_empties_coveragerecords( + alembic_runner: MigrationContext, + alembic_engine: Engine, +) -> None: + """The migration removes every row from coveragerecords. + + Every column except the primary key is nullable, so a bare insert is enough + to stand in for a leftover row written by the retired CoverageProvider. + """ + alembic_runner.migrate_down_to(REVISION) + alembic_runner.migrate_down_one() + + with alembic_engine.begin() as connection: + connection.execute( + text("INSERT INTO coveragerecords (operation) VALUES ('import')") + ) + assert _coveragerecord_count(alembic_engine) == 1 + + alembic_runner.migrate_up_one() + assert alembic_runner.current == REVISION + + assert _coveragerecord_count(alembic_engine) == 0 + + # The table itself survives this release -- it is dropped in the next one. + with alembic_engine.begin() as connection: + connection.execute( + text("INSERT INTO coveragerecords (operation) VALUES ('import')") + ) + assert _coveragerecord_count(alembic_engine) == 1 From 664289f256f3b297ca13abcbbea38e40e112d9bb Mon Sep 17 00:00:00 2001 From: Daniel Bernstein Date: Mon, 28 Sep 2026 15:36:48 -0700 Subject: [PATCH 2/2] Address review: fix lock_timeout rationale, trim comments, test the reset Tim's review points: - The migration docstring claimed a failed migration "is retried rather than blocking instance startup". Nothing retries it, and a lock_timeout abort raises OperationalError, which initialize_database does not catch (it only catches CommandError), so the deploy does fail and has to be re-run. Say that instead: the timeout buys a loud, bounded failure rather than a TRUNCATE that queues ahead of every reader and stalls them indefinitely. - Drop the BaseCoverageRecord docstring's account of what was deleted, and cut the CoverageRecord docstring from four paragraphs to one. Explaining removed code is PR commentary, not something a future reader of this file needs. - Add a regression test for the lock_timeout reset. Driving the migration through alembic_runner cannot catch a leak, because it commits between revisions and discards the setting either way; the test runs upgrade() against a transaction it holds open, which is what env.py actually does. Verified it fails ('5s' != '0') with the reset removed. Co-Authored-By: Claude Opus 5 --- ...mpty_coveragerecords_before_dropping_it.py | 54 ++++++++----------- .../manager/sqlalchemy/model/coverage.py | 37 +++---------- ...0916_58ebd34c5092_empty_coveragerecords.py | 53 ++++++++++++++++++ 3 files changed, 83 insertions(+), 61 deletions(-) diff --git a/alembic/versions/20260916_58ebd34c5092_empty_coveragerecords_before_dropping_it.py b/alembic/versions/20260916_58ebd34c5092_empty_coveragerecords_before_dropping_it.py index ef7ace2a7c..0862b9d59f 100644 --- a/alembic/versions/20260916_58ebd34c5092_empty_coveragerecords_before_dropping_it.py +++ b/alembic/versions/20260916_58ebd34c5092_empty_coveragerecords_before_dropping_it.py @@ -1,42 +1,32 @@ """Empty coveragerecords before dropping it -The ``coverage_records`` relationships on Identifier, DataSource and Collection are -removed in this release, which is what finally stops the application reading the -``coveragerecords`` table. Those relationships were also the only thing keeping parent -deletes working: ``coveragerecords`` has plain foreign keys to ``identifiers``, -``datasources`` and ``collections`` with no ``ON DELETE`` clause, so SQLAlchemy had to -load the children and cascade (or null their FK) by hand. With the relationships gone -nothing does that any more, and any surviving row would make deleting its parent fail -with a foreign-key violation. - -The rows are dead data -- the CoverageProvider machinery that wrote them was retired a -release ago -- so we empty the table rather than add ``ON DELETE`` clauses to a table +This release removes the ``coverage_records`` relationships on Identifier, DataSource +and Collection, which is what finally stops the application reading this table. Those +relationships were also the only thing cleaning up children on a parent delete -- +``coveragerecords`` has plain foreign keys with no ``ON DELETE`` clause -- so with them +gone any surviving row would make deleting its parent fail on a foreign key. The rows +are dead data, so we empty the table rather than add ``ON DELETE`` clauses to a table that is dropped in the next release. TRUNCATE rather than DELETE: the table can hold tens of millions of rows, and TRUNCATE -reclaims them in constant time without generating row-level WAL. Nothing references +reclaims them in constant time without row-level WAL. Nothing references ``coveragerecords``, so no CASCADE is needed. -The ACCESS EXCLUSIVE lock TRUNCATE takes is *not* guaranteed to be uncontended: N-1 -servers still map the ``coverage_records`` relationships this release removes, so they -take an ACCESS SHARE lock on the table whenever they delete an Identifier, DataSource -or Collection -- that is the very behaviour this release exists to stop. If such a -delete is in flight the TRUNCATE waits, and a waiting ACCESS EXCLUSIVE request queues -ahead of every later reader, so the whole table stalls behind it. The wait should be -short, but ``lock_timeout`` bounds it: the migration fails fast and is retried rather -than blocking instance startup. - -The timeout is reset immediately afterwards. ``SET LOCAL`` lasts to the end of the -*transaction*, not the end of this revision, and ``alembic/env.py`` runs every pending -revision inside a single ``context.begin_transaction()`` (it does not set -``transaction_per_migration``). Without the reset, any revision applied after this one -in the same ``alembic upgrade`` -- the stacked drop in the next release, or anything -added before this one ships -- would silently inherit a 5s lock timeout it never asked -for, and could roll the whole upgrade back while waiting on a busy table. - -``equivalentscoveragerecords`` deliberately gets no such treatment: it has no ORM -relationships pointing at it, and its one foreign key already declares -``ON DELETE CASCADE``, so the database cleans it up on its own. +``lock_timeout`` bounds the wait for TRUNCATE's ACCESS EXCLUSIVE lock. That lock is not +guaranteed to be uncontended: N-1 servers still map the relationships this release +removes, so they read the table when deleting an Identifier, DataSource or Collection. +Without the timeout a TRUNCATE waiting on such a delete would queue ahead of every +later reader and stall them all indefinitely. With it, the statement gives up after 5s +and the migration fails -- nothing retries it, so the deploy has to be re-run, which is +the better failure: loud and bounded rather than a spreading stall. + +The timeout is reset immediately afterwards because ``SET LOCAL`` lasts to the end of +the *transaction*, not the end of this revision, and ``alembic/env.py`` runs every +pending revision in a single ``context.begin_transaction()``. Without the reset, a +later revision in the same upgrade would inherit a 5s timeout it never asked for. + +``equivalentscoveragerecords`` needs no such treatment: it has no ORM relationships +pointing at it, and its one foreign key already declares ``ON DELETE CASCADE``. Revision ID: 58ebd34c5092 Revises: 5b1f4f3c7979 diff --git a/src/palace/manager/sqlalchemy/model/coverage.py b/src/palace/manager/sqlalchemy/model/coverage.py index 84b04994fd..eb02302ac1 100644 --- a/src/palace/manager/sqlalchemy/model/coverage.py +++ b/src/palace/manager/sqlalchemy/model/coverage.py @@ -31,11 +31,7 @@ class BaseCoverageRecord: - """Holds the ``coverage_status`` enum shared by the two dormant coverage models. - - Everything else on this mixin went with the queries it supported; it is - removed along with those models and their tables in the follow-up PR. - """ + """Holds the ``coverage_status`` enum shared by the two dormant coverage models.""" SUCCESS = "success" TRANSIENT_FAILURE = "transient failure" @@ -315,30 +311,13 @@ def recording(self) -> Generator[Self]: class CoverageRecord(Base, BaseCoverageRecord): """A record of a Identifier being used as input into some process. - Dormant model retained only so the ``coveragerecords`` table stays in the - schema for one more release. The CoverageProvider machinery that read and - wrote these records has been retired, and the ``coverage_records`` - relationships on Identifier, DataSource and Collection have been removed, so - nothing in the current code reads or writes this table. - - Removing those relationships is what actually stops the reads: a mapped - relationship is loaded by SQLAlchemy whenever its parent is deleted (to - cascade the delete, or to null the child's foreign key), so while they - existed every ``session.delete(collection)`` still SELECTed from this table. - - The model is kept for one more release because the schema of a freshly - initialized database is built with ``create_all`` from these models, not by - replaying migrations -- dropping the class would remove the table from new - installs immediately, while N-1 app servers still expect it. The model and - the table are removed together in a follow-up PR that ships after this - release. - - Only the columns remain. The query and write helpers (``lookup``, ``add_for``, - ``bulk_add`` and friends) are gone: nothing called them, and with the parent - relationships removed a row they wrote could no longer be cleaned up when its - Identifier, DataSource or Collection is deleted -- it would just make that - delete fail on a foreign key. Keeping the class a bare table definition makes - it impossible to write such a row. + 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" diff --git a/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py b/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py index 0d6e9da70a..9bfee20618 100644 --- a/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py +++ b/tests/migration/test_20260916_58ebd34c5092_empty_coveragerecords.py @@ -1,5 +1,11 @@ from __future__ import annotations +import importlib.util +from pathlib import Path +from types import ModuleType + +from alembic.operations import Operations +from alembic.runtime.migration import MigrationContext as RuntimeMigrationContext from pytest_alembic import MigrationContext from sqlalchemy import text from sqlalchemy.engine import Engine @@ -14,6 +20,22 @@ def _coveragerecord_count(engine: Engine) -> int: ).scalar_one() +def _load_revision() -> ModuleType: + """Import this revision as a standalone module. + + Revision files are not importable as a package, so load it by path. The + module is a private copy, so rebinding its ``op`` below cannot affect the + real alembic proxy used elsewhere. + """ + versions = Path(__file__).parents[2] / "alembic" / "versions" + (path,) = versions.glob(f"*_{REVISION}_*.py") + spec = importlib.util.spec_from_file_location(f"revision_{REVISION}", path) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + def test_empties_coveragerecords( alembic_runner: MigrationContext, alembic_engine: Engine, @@ -43,3 +65,34 @@ def test_empties_coveragerecords( text("INSERT INTO coveragerecords (operation) VALUES ('import')") ) assert _coveragerecord_count(alembic_engine) == 1 + + +def test_upgrade_does_not_leak_lock_timeout(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. + + 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. + """ + revision = _load_revision() + + with alembic_engine.begin() as connection: + before = connection.execute(text("SHOW lock_timeout")).scalar_one() + + # setattr, not plain assignment: the module is typed as ModuleType, so + # mypy does not know about the ``op`` its revision file imports. + setattr( + revision, "op", Operations(RuntimeMigrationContext.configure(connection)) + ) + revision.upgrade() + + after = connection.execute(text("SHOW lock_timeout")).scalar_one() + + assert after == before