perf(CommonPlayersStrategy): add index to reduce suggestions bottleneck #3132
Conversation
| 'deleted_at', | ||
| 'game_id', | ||
| ], 'idx_player_games_suggestions'); // custom name needed because the auto-generated one is too long | ||
| }); |
There was a problem hiding this comment.
This looks like the player_games_completion_sample_idx, but with the additional deleted_at column. Do we need both?
There was a problem hiding this comment.
There's currently 0 deleted player_games records.
Maybe you could remove the deleted_at check form the query and leverage the existing index? A little deleted data isn't going to matter for the outcome of random selection.
There was a problem hiding this comment.
I believe it's unfortunately still needed:
The screenshot above is with the index removing that column. Eloquent deciding to automagically include the column is what necessitates it being part of the compound index.
There was a problem hiding this comment.
That works - fixed in latest.
There was a problem hiding this comment.
Yes, I think so. As soon as I delete it, I see these monster queries appear:
There was a problem hiding this comment.
If we're not leveraging the old index, there's no reason to remove the deleted_at clause.
There was a problem hiding this comment.
Sorry, I'm happy to implement feedback, but I'm getting confused. What specifically would you like me to do?
My interpretation is you'd like me to add back deleted_at to the index as well as the queries, but we don't have any deleted records.
There was a problem hiding this comment.
I was hoping you'd be able to use the existing player_games_completion_sample_idx index, but it sounds like that's not being picked up.
KEY `player_games_completion_sample_idx` (`game_id`,`achievements_unlocked`,`achievements_total`,`user_id`,`id`),
I see now that that index has id in it - why would you ever want id in an index? Maybe we can drop it and replace it with yours.
If you're going to have to add a new index, do whatever you feel is necessary with it - like filtering out the deleted items.
There was a problem hiding this comment.
player_games_completion_sample_idx is a covering index, but id is probably redundant.
Based on what I'm seeing in my local, we don't even need that index anymore and a lot of the code in the strategy can be simplified.
I've updated the new migration and the strategy in latest.
Adds an index to
player_gameswhich dramatically improves the performance ofCommonPlayersStrategy.Before
After
