Skip to content
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

audit-logs: Add index to RealmAuditLog for realm and event type. #29763

Conversation

laurynmm
Copy link
Collaborator

Adds an index on RealmAuditLog for the realm, event_type, and id in order to improve database queries on these audit logs.


Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

models.Index(
name="zerver_realmauditlog_realm_event_type",
fields=["realm", "event_type", "id"],
),
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth putting this one first in the list, since it's the primary index.

),
models.Index(
name="zerver_realmauditlog_realm_event_type",
fields=["realm", "event_type", "id"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id is in there for ordering purposes, and as discussed elsewhere, event_time is probably better here -- and we should audit for existing order_by clauses to make sure they're doing them by that, and not id.

@timabbott timabbott force-pushed the add-generic-realm-event-type-index-for-auditlogs branch from e375faf to 2e79e03 Compare April 19, 2024 23:44
@timabbott
Copy link
Sponsor Member

I made the changes to use event_time and tested that this index was indeed also used even when one ordered by ID, since filtering to (realm, event_type) is usually pretty precise. With those changes, merged, thanks @laurynmm!

We should probably still do the audits that @alexmv suggested, and also look at adding parallel indexes to some remote audit logs, but this will do for this PR.

@timabbott timabbott enabled auto-merge (rebase) April 19, 2024 23:46
Adds an index on RealmAuditLog for the realm, event_type, and
event_time in order to improve database queries on these audit logs.

tabbott verified using EXPLAIN ANALYZE that this also considerably
speeds up queries that order by ID rather than event_time, but
event_time is how these should be ordered given the possibility of
backfills.
@alexmv alexmv force-pushed the add-generic-realm-event-type-index-for-auditlogs branch from 2e79e03 to ad912e1 Compare April 20, 2024 00:37
@timabbott timabbott merged commit 1d58970 into zulip:main Apr 20, 2024
14 checks passed
@laurynmm laurynmm deleted the add-generic-realm-event-type-index-for-auditlogs branch April 21, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants