Add indices on various columns to increase performance #5935
Conversation
miaulalala
force-pushed
the
enhancement/additional-indices-for-mail
branch
from
e8cd9dc
to
060ee70
Compare
|
@nickvergessen I can't get Static Analysis to accept the migration. Any chance you could take a look? |
Signed-off-by: Anna Larch <anna@nextcloud.com>
miaulalala
force-pushed
the
enhancement/additional-indices-for-mail
branch
from
003729c
to
de0a122
Compare
| } | ||
|
|
||
| // Postgres doesn't allow for shortened indices, so let's skip the last index. | ||
| if ($schema->getDatabasePlatform() instanceof PostgreSQL94Platform) { |
There was a problem hiding this comment.
open a tech debt ticket. this code is prone to explode with any changes to the implementation of the db abstraction. we had the same issue when IDbConnection (or was it the builder?) was assumed to directly implement the Doctrine classes. But since we added a new layer of abstraction that was never the case -> kaboom.
suggestion for a fix: propose an OCP API change with a new accessor that gives us the db type, e.g. as string or number that you can compare to a constant.
There was a problem hiding this comment.
API change on the ISchemaWrapper class?
There was a problem hiding this comment.
|
|
||
| $table = $schema->getTable('mail_messages'); | ||
| if (!$table->hasIndex('mail_messages_id_flags')) { | ||
| $table->addIndex(['mailbox_id', 'flag_important', 'flag_deleted', 'flag_seen'], 'mail_messages_id_flags'); |
There was a problem hiding this comment.
have you done any performance profiling? the messages table is potentially large. if you have an installation with 20k users and 5k messages each, the table will contain 100M rows.
There was a problem hiding this comment.
I haven't but these indices are from c.nc.com. They must have been applied last year when we did an evaluation. It was suggested to a customer to use them and it worked fine for them and seemed to help with performance.
If you tell me how to, I will do it :)
There was a problem hiding this comment.
I don't have an exact threshold but any migration should run in a matter of seconds. The reason is that many installation use the web updater and there is a default timeout of 30s. Assuming that this app might not be the only component that needs time for a migration, we only have a fraction of those 30s to finish the job.
Anything that takes longer should be optional, if possible.
| $table->addIndex(['mailbox_id', 'flag_important', 'flag_deleted', 'flag_seen'], 'mail_messages_id_flags'); | ||
| } | ||
| if (!$table->hasIndex('mail_messages_id_flags2')) { | ||
| $table->addIndex(['mailbox_id', 'flag_deleted', 'flag_flagged'], 'mail_messages_id_flags2'); |
There was a problem hiding this comment.
same potential performance problems here. maybe this should all be optional indices?
There was a problem hiding this comment.
Sure thing, we could add an occ command for it for example?
There was a problem hiding this comment.
yes, albeit it would be hard for admins to ever discover those as we do not have app-provided setup checks. nextcloud/server#25471 is probably the more integrated approach.
|
This is scheduled to be applied with the cache reset. |
|
Redo as discussed |
This should speed up different queries, namely the threading query, considerably.