-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(db): Remove unused unique constrained from Group #103144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This uniq index and was added in #27064 for CDC and is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Uniqueness Constraint: Foundation of Counter System
Removing the unique_together constraint on (project, short_id) breaks the stuck counter detection and fixing logic in event_manager.py. The _is_stuck_counter_error function relies on detecting UniqueViolation errors on this constraint to automatically fix stuck project counters. Without this constraint, duplicate short_id values within a project can exist undetected, breaking the qualified short ID system which assumes uniqueness.
src/sentry/models/group.py#L693-L694
sentry/src/sentry/models/group.py
Lines 693 to 694 in 9b330cf
| models.Index(fields=("project", "status", "priority", "last_seen", "id")), | |
| ] |
|
|
||
| dependencies = [ | ||
| ("sentry", "1003_group_history_prev_history_safe_removal"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AlterUniqueTogether( | ||
| name="group", | ||
| unique_together=set(), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Removing the (project, short_id) unique constraint disables critical "stuck counter" error detection, leading to silent duplicate group creation.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The removal of the unique constraint (project, short_id) from the Group model will disable the existing error handling mechanism for "stuck counter" conditions. Specifically, the _is_stuck_counter_error function, which relies on catching psycopg2.errors.UniqueViolation for the sentry_groupedmessage_project_id_short_id constraint, will no longer trigger. This means that if the Counter mechanism for generating short_id values gets out of sync with existing group short_ids (a condition that has occurred in production, as evidenced by the comment "< 20 ever that we know of" in _handle_stuck_project_counter), duplicate groups with identical (project_id, short_id) values will be silently created instead of being detected and recovered.
💡 Suggested Fix
Either re-evaluate the necessity of removing the (project, short_id) unique constraint, or update the _is_stuck_counter_error and _handle_stuck_project_counter functions to use an alternative, robust mechanism for detecting and recovering from short_id synchronization issues without relying on a database unique constraint.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
src/sentry/migrations/1004_remove_group_project_short_id_unique_constraint.py#L1-L32
Potential issue: The removal of the unique constraint `(project, short_id)` from the
`Group` model will disable the existing error handling mechanism for "stuck counter"
conditions. Specifically, the `_is_stuck_counter_error` function, which relies on
catching `psycopg2.errors.UniqueViolation` for the
`sentry_groupedmessage_project_id_short_id` constraint, will no longer trigger. This
means that if the `Counter` mechanism for generating `short_id` values gets out of sync
with existing group `short_id`s (a condition that has occurred in production, as
evidenced by the comment "< 20 ever that we know of" in
`_handle_stuck_project_counter`), duplicate groups with identical `(project_id,
short_id)` values will be silently created instead of being detected and recovered.
Did we get this right? 👍 / 👎 to inform future reviews.
|
This PR has a migration; here is the generated SQL for for --
-- Alter unique_together for group (0 constraint(s))
--
ALTER TABLE "sentry_groupedmessage" DROP CONSTRAINT "sentry_groupedmessage_project_id_short_id_29374dd7_uniq"; |
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.