-
Notifications
You must be signed in to change notification settings - Fork 37
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
Inverse column order on syncv3_nid_room_state_idx index #187
Conversation
5989df2
to
895d986
Compare
895d986
to
02d07e2
Compare
The tested query was |
Excellent finding, thank you for persisting. We should sanity check all the queries that use this index though. I wonder why the new index performs better, i.e. what work have we skipped? I can't spot much difference in the query planner output (similar numbers of heap fetches?) |
I forgot to say that those numbers are from a fully cold DB (right after a reboot) so are worst case scenario. The difference seems negligible if running the query a second time. However it does happen to me IRL and pretty often, after 30mn with no sync this query is cold enough to reliably take ~2+s to run. It's not clear to me either why the difference is that dramatic. |
Finding SELECT statements which aren't just scanning for ids/nids; potential queries affected:
Checking which queries make use of that index today.. |
First query is the one which this PR intentionally modifies:
Second query is affected too:
but is hilariously never used, only in tests. Third query is
Fourth query is not affected. Fifth query is affected:
Last query is not affected. |
The actual number of table lookups (heap fetches) are the same with both indexes. The value comes from not having to faff about with as much of the index. With an index of In the future when we jump snapshots ahead for gappy state, we'll make more use of I've applied this index manually for now and will sit for a bit to see how metrics behave, and specifically to see if EXPLAIN ANALYZE starts using the new index instead of the old one. |
Sadly, only 1 of the queries has started using the new index
I see no impact on DB load with this new index. Inclined to close this. |
On a more realistic query where the
This never uses the new query, preferring:
This does use the new index:
Unsure how to interpret this.. |
I'm going to close this for now. The testing back in July didn't make this an obvious improvement. We can look into this some more if/when performance becomes an issue. |
The order matters because columns that are evaluated with equality need to come before the ones that deal with inequality.
From PG doc:
Here
event_nid
is used with<=
operator whileroom_id
andis_state
are used with=
, soevent_nid
should be last.some
EXPLAIN ANALYZE
reflecting that:Signed-off-by: Mathieu Velten mathieuv@matrix.org
Pull Request Checklist