[YheChen]

Proposed Changes

Adds an index on each of the 22 columns that rails active_record_doctor:unindexed_foreign_keys reports on master. 17 are foreign keys, and 5 are _id columns without a constraint: groupings_tags.grouping_id, notes(noteable_type, noteable_id), test_runs.autotest_test_id, lti_deployments.lms_course_id and assignment_properties.remote_autotest_settings_id. Without an index, a lookup by one of these columns, and the foreign key check Postgres runs when a parent row is deleted, read the whole table.

• One migration, 20261030000000_add_indexes_to_unindexed_foreign_keys.rb, with an add_index per column. notes gets one index on (noteable_type, noteable_id).
• db/structure.sql and the model annotations, as regenerated by the migration.
• After migrating, the detector reports nothing.

Timings

I timed each column before and after on synthetic data in Postgres 17, with a warm cache:

• Lookup: SELECT * FROM table WHERE column = value, median of 15 values.
• Delete check: the foreign key check on the table when a parent row is deleted, from EXPLAIN ANALYZE, median of 5.

Sized like one 1,000-student CSC108 term. I estimated each table's size from how MarkUs creates its rows, assuming 40 TAs, 12 individual assignments, 3 marks spreadsheets and 4 test runs per student per assignment. Most flagged tables hold 100 rows or fewer.

Column	Rows	Lookup before	Lookup after
test_runs.autotest_test_id	60,000	1.98 ms	0.03 ms
annotations.result_id	14,000	0.56 ms	0.05 ms
memberships.grouping_id	15,000	0.47 ms	0.05 ms
annotations.annotation_text_id	14,000	0.38 ms	0.04 ms
groupings.group_id	12,000	0.35 ms	0.09 ms
grades.grade_entry_student_id	24,000	0.30 ms	0.09 ms
annotation_texts.creator_id	8,700	0.29 ms	0.12 ms
grade_entry_students.assessment_id	3,000	0.14 ms	0.13 ms

• Delete checks on these tables went from 0.15–0.61 ms to 0.04–0.19 ms.
• The other 12 columns are on tables of 100 rows or fewer. There, Postgres still reads the whole table, and nothing changes.
• The migration took 42 ms.
• Some code paths repeat these lookups once per row. For example, TestRun#autotest_test_id_uniqueness filters test_runs by autotest_test_id for every test run MarkUs creates. So at the end of a term, a 1,000-group batch run spends about 2 s on these scans, versus about 0.03 s with the index.

1,000,000 rows in every flagged table.

• Lookups went from 9–32 ms to 0.14–0.60 ms (median 52×).
• Delete checks went from 8–34 ms to 0.04–0.20 ms (median 177×).
• The migration took 3.6 s.

Full-table scans grow with the number of rows, while index lookups stay about the same, so an instance that holds several courses gains more than a single course does.

Screenshots of your changes (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)	X
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
📖 Documentation update (change that updates documentation)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

• I have updated the project Changelog (this is required for all changes).
• I have verified that the pre-commit.ci checks have passed.
• I have verified that the CI tests have passed.
• I have reviewed the test coverage changes reported by Coveralls.
• I have requested a review from a project maintainer.

Questions and Comments

• There are no new tests, because the change only adds indexes and doesn't change behaviour. The specs for the affected models pass.
• All 22 columns are indexed so that the detector reports nothing. That includes the 12 on tables too small for Postgres to use the index at one course's size.
• The timing script isn't part of this PR. I can add it, or share the full per-column results.
• Possible follow-up: run active_record_doctor:unindexed_foreign_keys in CI, so new foreign keys get an index from the start.

[coveralls]

Coverage Report for CI Build 41132411413

Coverage remained the same at 91.063%

Details

• Coverage remained the same as the base build.
• Patch coverage: No coverable lines changed in this PR.
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	54139
Covered Lines:	50348
Line Coverage:	93.0%
Relevant Branches:	2569
Covered Branches:	1292
Branch Coverage:	50.29%
Branches in Coverage %:	Yes
Coverage Strength:	127.26 hits per line

---

💛 - Coveralls

[david-yz-liu]

Nice work, @YheChen! I especially appreciated the information on the timing evaluation; I don't think it's necessary to include that script in this repository, however.