From 4cd01b5afcf3a9f69fef5a70a51646f3b95b110b Mon Sep 17 00:00:00 2001 From: Lucas Delvoye Date: Tue, 8 Sep 2026 14:44:18 -0700 Subject: [PATCH 1/3] chore(organizations): remove the member teams new_id dual-write The pk swap has landed, so the machinery that kept the shadow column equal to the id is dead weight; inserts now let the identity generate the id. The column is only marked pending here, since dropping it for real needs its own deploy. --- migrations_lockfile.txt | 2 +- src/sentry/backup/comparators.py | 6 - ...anizationmemberteam_drop_new_id_pending.py | 45 ++++++ src/sentry/models/organizationmemberteam.py | 76 +-------- .../models/test_organizationmemberteam.py | 146 +----------------- 5 files changed, 50 insertions(+), 225 deletions(-) create mode 100644 src/sentry/migrations/1165_organizationmemberteam_drop_new_id_pending.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index c2ef864cff38..fb24790b937a 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -35,7 +35,7 @@ replays: 0001_squashed_0007_organizationmember_replay_access seer: 0033_seer_run_pr_iteration -sentry: 1164_organizationmemberteam_swap_new_id_primary_key +sentry: 1165_organizationmemberteam_drop_new_id_pending social_auth: 0001_squashed_0003_social_auth_json_field diff --git a/src/sentry/backup/comparators.py b/src/sentry/backup/comparators.py index f9490a0d4d40..f66e2e0c30ee 100644 --- a/src/sentry/backup/comparators.py +++ b/src/sentry/backup/comparators.py @@ -892,12 +892,6 @@ def get_default_comparators() -> dict[str, list[JSONScrubbingComparator]]: DateUpdatedComparator("date_updated", "date_added"), HashObfuscatingComparator("token"), ], - "sentry.organizationmemberteam": [ - # `new_id` mirrors the primary key, which is reassigned on import, so - # comparing its value across an export cycle is meaningless. The - # `new_id == id` invariant is enforced on write instead. - IgnoredComparator("new_id"), - ], "sentry.projectkey": [ HashObfuscatingComparator("public_key", "secret_key"), SecretHexComparator(16, "public_key", "secret_key"), diff --git a/src/sentry/migrations/1165_organizationmemberteam_drop_new_id_pending.py b/src/sentry/migrations/1165_organizationmemberteam_drop_new_id_pending.py new file mode 100644 index 000000000000..3791b8e73674 --- /dev/null +++ b/src/sentry/migrations/1165_organizationmemberteam_drop_new_id_pending.py @@ -0,0 +1,45 @@ +# Generated by Django 5.2.16 on 2026-09-08 21:08 + +import sentry.db.models.fields.bounded +from django.db import migrations + +from sentry.new_migrations.migrations import CheckedMigration +from sentry.new_migrations.monkey.fields import SafeRemoveField +from sentry.new_migrations.monkey.state import DeletionAction + + +class Migration(CheckedMigration): + # This flag is used to mark that a migration shouldn't be automatically run in production. + # This should only be used for operations where it's safe to run the migration after your + # code has deployed. So this should not be used for most operations that alter the schema + # of a table. + # Here are some things that make sense to mark as post deployment: + # - Large data migrations. Typically we want these to be run manually so that they can be + # monitored and not block the deploy for a long period of time while they run. + # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to + # run this outside deployments so that we don't block them. Note that while adding an index + # is a schema change, it's completely safe to run the operation after the code has deployed. + # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment + + is_post_deployment = False + + dependencies = [ + ("sentry", "1164_organizationmemberteam_swap_new_id_primary_key"), + ] + + operations = [ + # `SafeRemoveField` refuses a column that is neither nullable nor has a `db_default`. + migrations.AlterField( + model_name="organizationmemberteam", + name="new_id", + field=sentry.db.models.fields.bounded.BoundedIntegerField(null=True), + ), + # State-only removal: drops the field from Django's model state but keeps the column + # in the database, so app servers still running the old code can continue to SELECT + # it during the rolling deploy. The column itself goes in a follow-up migration. + SafeRemoveField( + model_name="organizationmemberteam", + name="new_id", + deletion_action=DeletionAction.MOVE_TO_PENDING, + ), + ] diff --git a/src/sentry/models/organizationmemberteam.py b/src/sentry/models/organizationmemberteam.py index 538a13d21a5e..42bcd76d76b5 100644 --- a/src/sentry/models/organizationmemberteam.py +++ b/src/sentry/models/organizationmemberteam.py @@ -1,77 +1,22 @@ from __future__ import annotations -from collections.abc import Iterable -from typing import Any, ClassVar +from typing import Any, ClassVar, Self -from django.db import connections, models, router +from django.db import models from sentry import features, roles from sentry.backup.scopes import RelocationScope from sentry.db.models import ( BoundedBigAutoField, - BoundedIntegerField, FlexibleForeignKey, Model, cell_silo_model, sane_repr, ) from sentry.db.models.manager.base import BaseManager -from sentry.db.models.manager.base_query_set import BaseQuerySet from sentry.roles import team_roles from sentry.roles.manager import TeamRole -MAX_RESERVED_IDS = 100_000 - - -def _reserve_ids(model: type[Model], count: int, using: str) -> list[int]: - """Claim `count` values from the model's primary key sequence ahead of insert. - - Sequences are per-database, so `using` must be where the rows are written — - drawing from another hands out ids that are already taken. - """ - if not 1 <= count <= MAX_RESERVED_IDS: - raise ValueError(f"Cannot reserve {count} ids, expected 1 to {MAX_RESERVED_IDS}.") - - with connections[using].cursor() as cursor: - cursor.execute( - "SELECT nextval(%s) FROM generate_series(1,%s);", - [f"{model._meta.db_table}_id_seq", count], - ) - return [row_id for (row_id,) in cursor.fetchall()] - - -class OrganizationMemberTeamQuerySet(BaseQuerySet["OrganizationMemberTeam"]): - """Keeps `new_id` equal to `id` on bulk inserts. - - This lives on the queryset rather than the manager because not every bulk insert - goes through `objects`: adding a team via a member's `teams` accessor, or any - `.using(...)` call, reaches the queryset directly. - """ - - def bulk_create( - self, objs: Iterable[OrganizationMemberTeam], *args: Any, **kwds: Any - ) -> list[OrganizationMemberTeam]: - rows = list[OrganizationMemberTeam](objs) - if not rows: - return super().bulk_create(rows, *args, **kwds) - - # `self.db` would give the read database here. - using = self._db # type: ignore[attr-defined] - if using is None: - using = router.db_for_write(self.model, **self._hints) # type: ignore[attr-defined] - - # Claim the pks up front so `new_id` can be written in the same INSERT. - rows_with_ids = zip(rows, _reserve_ids(self.model, len(rows), using)) - for row, row_id in rows_with_ids: - row.id = row_id - row.new_id = row_id - return super().bulk_create(rows, *args, **kwds) - - -OrganizationMemberTeamManager = BaseManager.from_queryset( - OrganizationMemberTeamQuerySet, "OrganizationMemberTeamManager" -) - @cell_silo_model class OrganizationMemberTeam(Model): @@ -79,13 +24,11 @@ class OrganizationMemberTeam(Model): Identifies relationships between organization members and the teams they are on. """ - objects: ClassVar[BaseManager[OrganizationMemberTeam]] = OrganizationMemberTeamManager() + objects: ClassVar[BaseManager[Self]] = BaseManager() __relocation_scope__ = RelocationScope.Organization id = BoundedBigAutoField(primary_key=True) - # Narrow leftover of the id widening, still written on every insert until it is dropped. - new_id = BoundedIntegerField() team = FlexibleForeignKey("sentry.Team") organizationmember = FlexibleForeignKey("sentry.OrganizationMember") # an inactive membership simply removes the team from the default list @@ -100,19 +43,6 @@ class Meta: __repr__ = sane_repr("team_id", "organizationmember_id") - def save(self, **kwds: Any) -> None: - if self.id is None: - # Claim the pk up front so `new_id` can be written in the same INSERT. - using = kwds.get("using") - if using is None: - using = router.db_for_write(type(self), instance=self) - self.id = _reserve_ids(type(self), 1, using)[0] - self.new_id = self.id - # A freshly claimed pk cannot already exist, so skip the UPDATE probe - # Django would otherwise run before inserting. - kwds["force_insert"] = True - super().save(**kwds) - def get_audit_log_data(self) -> dict[str, Any]: return { "team_slug": self.team.slug, diff --git a/tests/sentry/models/test_organizationmemberteam.py b/tests/sentry/models/test_organizationmemberteam.py index c93504e5ecd9..136d4b122358 100644 --- a/tests/sentry/models/test_organizationmemberteam.py +++ b/tests/sentry/models/test_organizationmemberteam.py @@ -1,16 +1,5 @@ -from unittest import mock - -import pytest -from django.db import connections, router -from django.test.utils import CaptureQueriesContext - from sentry.hybridcloud.models.outbox import CellOutbox, outbox_context -from sentry.models.organizationmember import OrganizationMember -from sentry.models.organizationmemberteam import ( - MAX_RESERVED_IDS, - OrganizationMemberTeam, - _reserve_ids, -) +from sentry.models.organizationmemberteam import OrganizationMemberTeam from sentry.roles import team_roles from sentry.testutils.cases import TestCase from sentry.testutils.helpers import with_feature @@ -68,136 +57,3 @@ def test_bulk_membership_write_produces_no_outbox(self) -> None: OrganizationMemberTeam.objects.filter(id__in=[o.id for o in omts]).delete() assert CellOutbox.objects.count() == 0 - - -class OrganizationMemberTeamShadowIdTest(TestCase): - def setUp(self) -> None: - self.organization = self.create_organization() - self.team = self.create_team(organization=self.organization) - - def new_member(self) -> OrganizationMember: - return self.create_member(organization=self.organization, user=self.create_user()) - - def omt_updates(self, queries: CaptureQueriesContext) -> list[str]: - return [ - query["sql"] - for query in queries.captured_queries - if query["sql"].lstrip().upper().startswith("UPDATE") - and "sentry_organizationmember_teams" in query["sql"] - ] - - def test_create_populates_new_id(self) -> None: - omt = self.create_team_membership(team=self.team, member=self.new_member()) - - omt.refresh_from_db() - assert omt.new_id == omt.id - - # Built directly: no fixture exercises bulk_create, which is the path under test. - def test_bulk_create_populates_new_id(self) -> None: - members = [self.new_member() for _ in range(3)] - - omts = OrganizationMemberTeam.objects.bulk_create( - [ - OrganizationMemberTeam(organizationmember=member, team=self.team) - for member in members - ] - ) - - assert len(omts) == 3 - for omt in omts: - omt.refresh_from_db() - assert omt.new_id == omt.id - - # Built directly: no fixture exercises a bare save(), which is the path under test. - def test_bare_save_populates_new_id(self) -> None: - omt = OrganizationMemberTeam(organizationmember=self.new_member(), team=self.team) - - omt.save() - - omt.refresh_from_db() - assert omt.new_id == omt.id - - def test_update_preserves_new_id(self) -> None: - omt = self.create_team_membership(team=self.team, member=self.new_member()) - - omt.role = "admin" - omt.save() - - omt.refresh_from_db() - assert omt.new_id == omt.id - - def test_create_issues_no_follow_up_update(self) -> None: - member = self.new_member() - using = router.db_for_write(OrganizationMemberTeam) - - with CaptureQueriesContext(connections[using]) as queries: - self.create_team_membership(team=self.team, member=member) - - assert self.omt_updates(queries) == [] - - def test_bare_save_issues_no_follow_up_update(self) -> None: - omt = OrganizationMemberTeam(organizationmember=self.new_member(), team=self.team) - using = router.db_for_write(OrganizationMemberTeam) - - with CaptureQueriesContext(connections[using]) as queries: - omt.save() - - assert self.omt_updates(queries) == [] - - def test_bulk_create_with_no_objects(self) -> None: - assert list(OrganizationMemberTeam.objects.bulk_create([])) == [] - - def test_reserve_ids_rejects_a_count_below_one(self) -> None: - using = router.db_for_write(OrganizationMemberTeam) - - with pytest.raises(ValueError): - _reserve_ids(OrganizationMemberTeam, 0, using) - - with pytest.raises(ValueError): - _reserve_ids(OrganizationMemberTeam, -1, using) - - # The ids have to stay unspent: the sequence never hands a value back. - def test_reserve_ids_rejects_a_count_above_the_ceiling(self) -> None: - using = router.db_for_write(OrganizationMemberTeam) - - with CaptureQueriesContext(connections[using]) as queries: - with pytest.raises(ValueError): - _reserve_ids(OrganizationMemberTeam, MAX_RESERVED_IDS + 1, using) - - assert [query for query in queries.captured_queries if "nextval" in query["sql"]] == [] - - # The router is pointed elsewhere so the caller's database is the only thing that - # can produce a working sequence. - def test_bulk_create_reserves_ids_on_the_target_database(self) -> None: - member = self.new_member() - using = router.db_for_write(OrganizationMemberTeam) - - with mock.patch.object(router, "db_for_write", return_value="secondary"): - with CaptureQueriesContext(connections[using]) as queries: - OrganizationMemberTeam.objects.using(using).bulk_create( - [OrganizationMemberTeam(organizationmember=member, team=self.team)] - ) - - assert [query for query in queries.captured_queries if "nextval" in query["sql"]] - - # The m2m accessor inserts through-rows via `QuerySet.bulk_create`, bypassing - # anything defined on the manager. - def test_m2m_add_populates_new_id(self) -> None: - member = self.new_member() - - member.teams.add(self.team) - - omt = OrganizationMemberTeam.objects.get(organizationmember=member, team=self.team) - assert omt.new_id == omt.id - - # `Manager.using(...)` returns a queryset, so this too skips the manager. - def test_queryset_bulk_create_populates_new_id(self) -> None: - member = self.new_member() - using = router.db_for_write(OrganizationMemberTeam) - - OrganizationMemberTeam.objects.using(using).bulk_create( - [OrganizationMemberTeam(organizationmember=member, team=self.team)] - ) - - omt = OrganizationMemberTeam.objects.get(organizationmember=member, team=self.team) - assert omt.new_id == omt.id From 21a384aa5b003d5d64bc23756d422b14b6bdb55f Mon Sep 17 00:00:00 2001 From: Lucas Delvoye Date: Tue, 8 Sep 2026 14:44:28 -0700 Subject: [PATCH 2/3] test(organizations): drop the member teams pk swap migration test The swap is deployed and its migration is frozen, so the test can no longer fail in a way anyone would act on. --- ...ationmemberteam_swap_new_id_primary_key.py | 188 ------------------ 1 file changed, 188 deletions(-) delete mode 100644 tests/sentry/migrations/test_1164_organizationmemberteam_swap_new_id_primary_key.py diff --git a/tests/sentry/migrations/test_1164_organizationmemberteam_swap_new_id_primary_key.py b/tests/sentry/migrations/test_1164_organizationmemberteam_swap_new_id_primary_key.py deleted file mode 100644 index 9dc3097f26cd..000000000000 --- a/tests/sentry/migrations/test_1164_organizationmemberteam_swap_new_id_primary_key.py +++ /dev/null @@ -1,188 +0,0 @@ -from django.db import connection - -from sentry.testutils.cases import TestMigrations - -# Past int4, so a row carrying it can only have come through the wide column. -WIDE_ID = 2_147_483_648 - - -def fetch_columns(): - with connection.cursor() as cursor: - cursor.execute( - """ - SELECT column_name, data_type, column_default - FROM information_schema.columns - WHERE table_name = 'sentry_organizationmember_teams' - AND column_name IN ('id', 'new_id') - """ - ) - rows = cursor.fetchall() - return {name: (data_type, default) for name, data_type, default in rows} - - -def fetch_primary_key(): - with connection.cursor() as cursor: - cursor.execute( - """ - SELECT pk.constraint_name, pk_columns.column_name - FROM information_schema.table_constraints pk - JOIN information_schema.key_column_usage pk_columns - ON pk_columns.constraint_name = pk.constraint_name - WHERE pk.table_name = 'sentry_organizationmember_teams' - AND pk.constraint_type = 'PRIMARY KEY' - """ - ) - return cursor.fetchall() - - -def fetch_sequence_type(): - with connection.cursor() as cursor: - cursor.execute( - "SELECT seqtypid::regtype::text FROM pg_sequence " - "WHERE seqrelid = 'sentry_organizationmember_teams_id_seq'::regclass" - ) - (sequence_type,) = cursor.fetchone() - return sequence_type - - -def fetch_id_sequence_name(): - with connection.cursor() as cursor: - cursor.execute("SELECT pg_get_serial_sequence('sentry_organizationmember_teams', 'id')") - (sequence_name,) = cursor.fetchone() - return sequence_name - - -def id_is_identity(): - with connection.cursor() as cursor: - cursor.execute( - """ - SELECT is_identity FROM information_schema.columns - WHERE table_name = 'sentry_organizationmember_teams' AND column_name = 'id' - """ - ) - (is_identity,) = cursor.fetchone() - return is_identity == "YES" - - -def next_sequence_value(): - with connection.cursor() as cursor: - cursor.execute( - "SELECT CASE WHEN is_called THEN last_value + 1 ELSE last_value END " - "FROM sentry_organizationmember_teams_id_seq" - ) - (next_id,) = cursor.fetchone() - return next_id - - -def assert_identity_id(): - # Rolling 1164 back always restores an identity, so this shape needs no reshaping. - assert id_is_identity() - - -def force_sequence_backed_id(): - # Production's shape, which predates Django emitting identity columns. - next_id = next_sequence_value() - with connection.cursor() as cursor: - cursor.execute("ALTER TABLE sentry_organizationmember_teams ALTER COLUMN id DROP IDENTITY") - cursor.execute( - "CREATE SEQUENCE sentry_organizationmember_teams_id_seq AS integer " - f"START WITH {next_id} OWNED BY sentry_organizationmember_teams.id" - ) - cursor.execute( - "ALTER TABLE sentry_organizationmember_teams ALTER COLUMN id " - "SET DEFAULT nextval('sentry_organizationmember_teams_id_seq')" - ) - - -class SwapOrganizationMemberTeamNewIdPrimaryKeyTest(TestMigrations): - app = "sentry" - migrate_from = "1163_drop_organizationmapping_require_email_verification" - migrate_to = "1164_organizationmemberteam_swap_new_id_primary_key" - - def prepare_id_shape(self): - assert_identity_id() - - def setup_initial_state(self): - self.organization = self.create_organization() - self.team = self.create_team(organization=self.organization) - self.member = self.create_member(organization=self.organization, user=self.create_user()) - self.other_member = self.create_member( - organization=self.organization, user=self.create_user() - ) - - def setup_before_migration(self, apps): - self.prepare_id_shape() - ((self.original_pk_name, _),) = fetch_primary_key() - - OrganizationMemberTeam = apps.get_model("sentry", "OrganizationMemberTeam") - - # The historical model has none of the dual-write hooks and new_id is NOT NULL, so the - # realistic row is created with a placeholder and squared up afterwards. - self.matched = OrganizationMemberTeam.objects.create( - team_id=self.team.id, organizationmember_id=self.member.id, new_id=-1 - ) - OrganizationMemberTeam.objects.filter(id=self.matched.id).update(new_id=self.matched.id) - - # Production never writes a mismatch, but it is what proves the surviving id column is - # the wide one rather than the two happening to agree. - self.mismatched = OrganizationMemberTeam.objects.create( - team_id=self.team.id, - organizationmember_id=self.other_member.id, - new_id=WIDE_ID, - ) - - # One test method: setUp runs the whole migrate-down, seed, migrate-up cycle per method. - def test_new_id_became_the_primary_key(self): - columns = fetch_columns() - assert columns["id"][0] == "bigint" - assert columns["new_id"][0] == "integer" - assert columns["new_id"][1] is None - - assert id_is_identity() - assert fetch_primary_key() == [(self.original_pk_name, "id")] - - # _reserve_ids selects from this name, so an identity named after the pre-rename - # column would break every insert. - assert fetch_id_sequence_name() == "public.sentry_organizationmember_teams_id_seq" - - # A narrow sequence would cap inserts at 2^31 whatever the column can hold. - assert fetch_sequence_type() == "bigint" - - OrganizationMemberTeam = self.apps.get_model("sentry", "OrganizationMemberTeam") - - swapped = OrganizationMemberTeam.objects.get(new_id=self.matched.id) - assert swapped.id == self.matched.id - - swapped = OrganizationMemberTeam.objects.get(new_id=self.mismatched.id) - assert swapped.id == WIDE_ID - - # No id given, so this only works if the identity survived the swap. A restarted - # sequence would hand back an id that is already taken. - third_member = self.create_member(organization=self.organization, user=self.create_user()) - inserted = OrganizationMemberTeam.objects.create( - team_id=self.team.id, organizationmember_id=third_member.id, new_id=0 - ) - assert inserted.id > self.matched.id - - # What _reserve_ids does: claim from the sequence, then insert that id explicitly. - # GENERATED ALWAYS would reject this; BY DEFAULT must not. - fourth_member = self.create_member(organization=self.organization, user=self.create_user()) - with connection.cursor() as cursor: - cursor.execute("SELECT nextval('sentry_organizationmember_teams_id_seq')") - (claimed_id,) = cursor.fetchone() - explicit = OrganizationMemberTeam.objects.create( - id=claimed_id, - team_id=self.team.id, - organizationmember_id=fourth_member.id, - new_id=claimed_id, - ) - assert explicit.id == claimed_id - - -class SwapSequenceBackedOrganizationMemberTeamNewIdPrimaryKeyTest( - SwapOrganizationMemberTeamNewIdPrimaryKeyTest -): - """The same swap against production's shape, where id is sequence-backed, not an identity.""" - - def prepare_id_shape(self): - force_sequence_backed_id() From 40f6e1f9e12eb74efb6da3b55bdd3bfdfda39895 Mon Sep 17 00:00:00 2001 From: Lucas Delvoye Date: Wed, 9 Sep 2026 09:11:56 -0700 Subject: [PATCH 3/3] chore(organizations): renumber the new_id pending migration to 1167 Master landed 1165 and 1166 while this was open. --- migrations_lockfile.txt | 2 +- ...ng.py => 1167_organizationmemberteam_drop_new_id_pending.py} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename src/sentry/migrations/{1165_organizationmemberteam_drop_new_id_pending.py => 1167_organizationmemberteam_drop_new_id_pending.py} (96%) diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index 9b6afe8efd1a..a7ac994475c5 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -35,7 +35,7 @@ replays: 0001_squashed_0007_organizationmember_replay_access seer: 0033_seer_run_pr_iteration -sentry: 1166_externalissue_provider_assignee_updated_at +sentry: 1167_organizationmemberteam_drop_new_id_pending social_auth: 0001_squashed_0003_social_auth_json_field diff --git a/src/sentry/migrations/1165_organizationmemberteam_drop_new_id_pending.py b/src/sentry/migrations/1167_organizationmemberteam_drop_new_id_pending.py similarity index 96% rename from src/sentry/migrations/1165_organizationmemberteam_drop_new_id_pending.py rename to src/sentry/migrations/1167_organizationmemberteam_drop_new_id_pending.py index 3791b8e73674..633834626d2b 100644 --- a/src/sentry/migrations/1165_organizationmemberteam_drop_new_id_pending.py +++ b/src/sentry/migrations/1167_organizationmemberteam_drop_new_id_pending.py @@ -24,7 +24,7 @@ class Migration(CheckedMigration): is_post_deployment = False dependencies = [ - ("sentry", "1164_organizationmemberteam_swap_new_id_primary_key"), + ("sentry", "1166_externalissue_provider_assignee_updated_at"), ] operations = [