-
Notifications
You must be signed in to change notification settings - Fork 182
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
Add database indexes for the session_stats tables #493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
The postgresql docs say
Ensure that the
constraint_exclusion
configuration parameter is not disabled inpostgresql.conf
. If it is, queries will not be optimized as desired.
I believe we don't change the default setting for constraint_exclusion
( https://github.com/mozilla/hubs-ops/blob/master/plans/postgresql/habitat/config/postgresql.conf ), and the default is what we want.
|
||
def up do | ||
for year <- 2018..@max_year, | ||
month <- 0..11 do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this written as 0..11
instead of 1..12
just to be consistent with the CreateSessionStatsTable
migration?
https://github.com/mozilla/reticulum/blob/ca443c520f5a2cb9f0e5da1367b72bddedc8a38a/priv/repo/migrations/20180408231444_create_session_stats_table.exs#L31-L32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah it was just a copy-paste. Could have simplified it, but I think this works.
defmodule Ret.Repo.Migrations.AddSessionStatsIndex do | ||
use Ecto.Migration | ||
|
||
@max_year 2030 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2030? Show some faith
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah! That's like 30 decades away in tech years.
Thanks for the review. Good find on the constraint_exclusion config. Definitely something to verify if this doesn't pan out. |
The session_stats table appears to be one of the main bottlenecks for server performance due to the large number of sessions we now handle. The table was created with this in mind, where we partitioned the table in to monthly date ranges, however even this partitioning is not sufficient to mitigate the performance impact it has. This impact is particularly prevalent when we experience a flood of activity, such as when the Hubs Discord Bot needs to reestablish connections for all its bridged rooms.
The majority of the work behind this PR went into testing the performance characteristics of this table. I did so using a number of utility functions to populate a large number of records and test the performance various operations with and without the index. Importantly, while the index does have an impact on the upper limit of performance, it significantly speeds up update and query operations by about 10x, going from ~5.5 instructions per second (ips) for row updates on a fully loaded partition vs 56 ips on an indexed partition. This is compared to 77 ips on an empty, un-indexed partition.
The utility functions I used are in this gist. They were implemented as mix tasks, which can be run with
mix session_stats
followed by one of the commands implemented in the gist. Some of the functions use the Benchee library to benchmark the performance of those operations. https://gist.github.com/brianpeiris/e2bd4935497ddf60393a8549d17a0402