Making WordPress.org

Opened 2 weeks ago

Last modified 12 days ago

#8452 new enhancement

Optimize Trac database schema

Reported by: timse201 Owned by:
Priority: normal Milestone:
Component: Trac Keywords: dev-reviewed 2nd-opinion
Cc:

Description

A review of the MySQL schema files (mysql-schema.sql and notifications-schema.sql) identified several areas where performance, storage efficiency, and data integrity can be improved.

  1. Remove Redundant Indexes
  • ticket_change: Drop KEY ticket_change_ticket_idx (ticket) because ticket is already the first column of the composite PRIMARY KEY (ticket, time, field).
  • node_change: Drop KEY node_change_repos_rev_idx (repos, rev) because repos and rev are already covered by PRIMARY KEY (repos, rev, path, change_type).
  1. Add / Optimize Indexes
  • ticket: Currently only has indexes on time and status. Common queries like status != 'closed' AND reporter = $USER cause full-table scans. We should add composite indexes for common filter combinations (e.g., (status, reporter), (component, status), (milestone, status)).
  • _notifications: Change KEY type_value (type, value) to a covering index (type, value, username) so queries can be satisfied directly from the index.
  1. Fix Foreign Key Type Mismatch
  • ticket.id is defined as int(10) unsigned.
  • Foreign key references in ticket_change.ticket, ticket_custom.ticket, and _ticket_subs.ticket are defined as signed int(11).
  1. Improve _ticket_subs Table Structure
  • The table currently lacks an explicit primary key.
  • The current UNIQUE KEY username_ticket_status (username, ticket, status) allows multiple rows for the same user and ticket if the status changes.
  • Define PRIMARY KEY (ticket, username) to enforce one status entry per user per ticket.
  1. Field Data Types
  • ticket.summary: Change from text to varchar(255). This reduces storage overhead.
  • auth_cookie.time and session.last_visit: Consider changing from int(11) to bigint(20) to prevent Year 2038 issues and align with timestamps used in other tables.

Change History (2)

#1 @johnjamesjacoby
13 days ago

  • Keywords dev-reviewed 2nd-opinion added

Thanks for digging into this! I think there's useful housekeeping here, particularly making the ticket ID types consistent and widening the two remaining timestamp columns.

A few thoughts after looking through the schemas and notification queries:

  • The two indexes in item 1 are covered by the leftmost columns of their primary keys. Removing them seems reasonable, with before/after query plans to confirm we're not losing a useful scan path. For example, the narrower (ticket) secondary index can cover count queries without scanning the full clustered rows.
  • For the reporter query, I'd start with (reporter, status): the reporter comparison is equality, while status != 'closed' is a range condition. The component and milestone indexes look like reasonable candidates too, but I'd pick the final set from actual queries and EXPLAIN results.
  • I don't think _notifications needs the proposed covering-index change. Its existing UNIQUE (username, type, value) has all columns NOT NULL, so InnoDB uses it as the clustered key. The (type, value) secondary index already includes the remaining clustered-key column, username, and should already cover the SELECT username queries in the notification patch.
  • Matching the ticket ID signedness makes sense. These schemas don't declare foreign key constraints, so this is a consistency fix rather than a fix to an existing enforced constraint.
  • One status per user/ticket seems sensible, but _ticket_subs needs the write paths checked before changing uniqueness. The notification patch reads subscriptions (status > 0) and explicit blocks (status = 0) separately. With the current reads, an explicit block wins if both rows exist, because the block pass removes the user after subscriptions are added. A migration should preserve that behavior unless we intend to change it, and the write paths need to ensure changing a status updates the existing row. Its current unique, non-null key already gives InnoDB a clustered key, even without an explicit primary key.
  • I'd leave summary as TEXT for now unless we can demonstrate a benefit and establish that 255 characters is the intended application limit. VARCHAR(255) isn't automatically a meaningful storage saving with this multibyte charset, and it introduces a new limit for both existing and future tickets.

A few additional things I noticed in the schemas that might be worth separate follow-ups:

  • ticket_change.oldvalue is TEXT, while ticket.description and ticket_change.newvalue are LONGTEXT. I'd check the description-edit history path and consider widening oldvalue to match. If the previous description is recorded there, an edit to a description over 65,535 bytes can hit the smaller history-column limit even though the ticket itself supports it. A regression test that edits a large description would be useful here.
  • attachment.filename allows 400 characters, but its primary key only includes filename(255). That means two filenames on the same attachment parent with identical first 255 characters collide even if their remaining characters differ. I'd check the filename limits enforced by Trac and storage, then either make the schema match that limit or enforce uniqueness on the full filename, subject to the supported InnoDB key-size limits.
  • Both schemas explicitly use utf8/utf8_bin, and the main schema also sets the connection charset to utf8. I'd compare these with the live tables and connection settings and plan an explicit utf8mb4 migration if they're still using three-byte UTF-8. That would support supplementary Unicode characters, including emoji. We'd need to preserve the intended case-sensitive comparisons and check composite key sizes and row formats before changing the table and connection charsets together.

I'd be happy to see this move forward in small pieces, with migration SQL for the existing databases and measured results for the index changes.

Thanks for putting the list together!

#2 @timse201
12 days ago

Thanks a lot for the thorough review and great suggestions!

I really appreciate the pragmatic approach and agree with taking this in smaller steps.

Note: See TracTickets for help on using tickets.