Improve use of keys when rebuilding repository summary table
AbandonedPublic
Actions

Authored by epriestley on Sep 4 2018, 9:45 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13195: 2018 Week 36 Bonus Content

Summary

See PHI842. Ref T13195. If we discover commits have been removed from the repository, we rebuild the entire summary table.

This currently uses a query with two MAX(...) fields on different columns, and an importStatus & flag which can't be satisfied with a key.

Instead:

Denormalize unreachability into a separate, keyable column.
Add appropriate keys.
Execute the query as two halves with UNION ALL.

Also, remove an ancient rebuild_summaries.php script which has a copy of this logic and isn't referenced anywhere. This should become bin/repository <something> if we have some need for it. It currently gets the wrong result (it isn't aware of unreachable commits).

Test Plan

Changed the discovery engine to force the summary to always rebuild.
Ran bin/repository update ... --trace to see the rebuild happen.
Verified rebuild comes out properly.
Used EXPLAIN on the new query and saw a better (entirely key-based) query plan.

Diff Detail

Repository

rP Phabricator

Branch

unreach1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 20740
Build 28199: Run Core Tests
Build 28198: arc lint + arc unit

Event Timeline

epriestley created this revision.Sep 4 2018, 9:45 PM

Owners added a subscriber: Restricted Owners Package.Sep 4 2018, 9:45 PM

Harbormaster completed remote builds in B20739: Diff 46931.Sep 4 2018, 9:46 PM

epriestley requested review of this revision.Sep 4 2018, 9:46 PM

I'm not thrilled about this change since it requires us to do a bunch of ALTER TABLE to commit to improve a very edge-case query which only takes ~1 second to run even in the bad case. The outcome is probably better, sort of? And rebuild_summaries.php should get nuked no matter what. But I'm not sure I'll actually land this.

Actually remove the "rebuild_summaries.php" script.
(But, I'm going to just do that part and not the rest of it for now.)

epriestley planned changes to this revision.Sep 4 2018, 11:57 PM

Harbormaster completed remote builds in B20740: Diff 46932.Sep 4 2018, 11:58 PM

epriestley mentioned this in D19643: Remove outdated "rebuild_summaries.php" script.Sep 5 2018, 8:49 PM

At least for now, let's just do D19643 (remove the outdated script). It sounds like the rest of this runs very rarely and probably isn't worth all these schema changes to optimize. We can keep an eye out for other queries which would benefit, or other motivations.

epriestley mentioned this in rP7967fab5ccc6: Remove outdated "rebuild_summaries.php" script.Sep 6 2018, 3:18 PM

• GoogleLegacy added a subscriber: • GoogleLegacy.Sep 10 2018, 8:41 AM