-
Notifications
You must be signed in to change notification settings - Fork 21
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 a correct composite INDEX
for the history table
#686
Conversation
59fcfd0
to
bf7ef0d
Compare
That optimization was only added PostgreSQL 13 though, so the changed index might even be useful for older versions. |
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.
I have successfully tested this PR on a local setup with round about 300k entries in the history table. LGTM.
Times with the old schema
MariaDB [icingadb]> SELECT 1 FROM history ORDER BY event_time LIMIT 1\G
*************************** 1. row ***************************
1: 1
1 row in set (0.001 sec)
MariaDB [icingadb]> SELECT 1 FROM history ORDER BY event_time, event_type LIMIT 1\G
*************************** 1. row ***************************
1: 1
1 row in set (0.260 sec)
Performing schema migration
$ time mariadb […] icingadb < /tmp/schema-1.1.2.sql
real 0m3.122s
user 0m0.013s
sys 0m0.004s
Times with the new schema
MariaDB [icingadb]> SELECT 1 FROM history ORDER BY event_time LIMIT 1\G
*************************** 1. row ***************************
1: 1
1 row in set (0.001 sec)
MariaDB [icingadb]> SELECT 1 FROM history ORDER BY event_time, event_type LIMIT 1\G
*************************** 1. row ***************************
1: 1
1 row in set (0.001 sec)
MariaDB [icingadb]> SELECT COUNT(*) FROM history\G
*************************** 1. row ***************************
COUNT(*): 310850
1 row in set (0.169 sec)
I noticed some strange, possibly noteworthy behavior, when testing the schema upgrade on MariaDB (11.0.2). Before I started to apply the schema change, I tried to access I looked at
Killing the
So looks like there's a risk of blocking the whole database if there's some long-running query (in my case, the one that the new index is supposed to speed up). I'm not sure though if we can do anything about this apart from warning about it in the upgrade docs. |
I think it's because MySQL/MariaDB performs a MariaDB [icingadb]> EXPLAIN select * from history order by event_time, event_type limit 25;
+------+-------------+---------+------+---------------+------+---------+------+-------+----------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+------+-------------+---------+------+---------------+------+---------+------+-------+----------------+
| 1 | SIMPLE | history | ALL | NULL | NULL | NULL | NULL | 18878 | Using filesort |
+------+-------------+---------+------+---------------+------+---------+------+-------+----------------+
1 row in set (0.005 sec)
MariaDB [icingadb]> EXPLAIN select * from history order by event_time limit 25;
+------+-------------+---------+-------+---------------+------------------------+---------+------+------+-------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+------+-------------+---------+-------+---------------+------------------------+---------+------+------+-------+
| 1 | SIMPLE | history | index | NULL | idx_history_event_time | 8 | NULL | 25 | |
+------+-------------+---------+-------+---------------+------------------------+---------+------+------+-------+
1 row in set (0.001 sec |
My guess for what's going on is some interaction of shared locks: Probably the |
I also think it's a good idea to have this written somewhere. Having the processlists documented as you did would be also good. So a user can quickly identify such a scenario and fix it. |
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.
After #656 is done (otherwise this would just add merge conflicts anyways), please do the following:
- Rebase and resolve conflicts
- Add hints to the upgrading docs added in Unify check attempt data type to uint32 already used somewhere #656 that point out the potential locking problem during the upgrade (Add a correct composite
INDEX
for the history table #686 (comment), Add a correct compositeINDEX
for the history table #686 (comment))
6f92303
to
c7897b9
Compare
Just like the aforementioned PR would do here! That is not an excuse for not having merged this before, which by the way has already been approved. |
c7897b9
to
4633f26
Compare
4633f26
to
17118b4
Compare
17118b4
to
4b87c81
Compare
4b87c81
to
c3adfc7
Compare
c3adfc7
to
79d6f7e
Compare
No complaints with PostgreSQL so far, but to maintain the consistency with MySQL/MariaDB, the index for pgsql has also been adjusted. The output of the explain command also shows the differences between before and after this change.
PostgreSQL (EXPLAIN)
Before:
After:
fixes #683