Skip to content
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

[stable23] Use explicit cast to make use of index #3497

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Dec 28, 2021

This ensures that indexes of oc_share are being used on MySQL when fetching the shares of a user during mountpoint setup, where otherwise a full table scan would happen.

@nickvergessen I'm wondering if there is a specific reason that the MySQLExpressionBuilder does not implement any actual casting when using castColumn(). Is that something that we should rather do in the server then maybe?

EXPLAIN before

+------+-------------+-------+--------+----------------------------------------+----------------------------+---------+-------------------------+------+-------------------------------------------------+
| id   | select_type | table | type   | possible_keys                          | key                        | key_len | ref                     | rows | Extra                                           |
+------+-------------+-------+--------+----------------------------------------+----------------------------+---------+-------------------------+------+-------------------------------------------------+
|    1 | SIMPLE      | db    | const  | PRIMARY                                | PRIMARY                    | 4       | const                   | 1    | Using index; Using temporary; Using filesort    |
|    1 | SIMPLE      | ds    | ref    | PRIMARY,deck_stacks_board_id_index     | deck_stacks_board_id_index | 8       | const                   | 1    | Using index                                     |
|    1 | SIMPLE      | dc    | ref    | PRIMARY,deck_cards_stack_id_index      | deck_cards_stack_id_index  | 8       | nextcloud.ds.id         | 1    | Using where; Using index                        |
|    1 | SIMPLE      | s     | ALL    | item_share_type_index,share_with_index | NULL                       | NULL    | NULL                    | 1290 | Using where; Using join buffer (flat, BNL join) |
|    1 | SIMPLE      | f     | eq_ref | PRIMARY,fs_id_storage_size             | PRIMARY                    | 8       | nextcloud.s.file_source | 1    | Using where                                     |
|    1 | SIMPLE      | st    | eq_ref | PRIMARY                                | PRIMARY                    | 8       | nextcloud.f.storage     | 1    | Using where                                     |
+------+-------------+-------+--------+----------------------------------------+----------------------------+---------+-------------------------+------+-------------------------------------------------+

EXPLAIN after

+------+-------------+-------+--------+----------------------------------------+----------------------------+---------+-------------------------+------+----------------------------------------------+
| id   | select_type | table | type   | possible_keys                          | key                        | key_len | ref                     | rows | Extra                                        |
+------+-------------+-------+--------+----------------------------------------+----------------------------+---------+-------------------------+------+----------------------------------------------+
|    1 | SIMPLE      | db    | const  | PRIMARY                                | PRIMARY                    | 4       | const                   | 1    | Using index; Using temporary; Using filesort |
|    1 | SIMPLE      | ds    | ref    | PRIMARY,deck_stacks_board_id_index     | deck_stacks_board_id_index | 8       | const                   | 1    | Using index                                  |
|    1 | SIMPLE      | dc    | ref    | deck_cards_stack_id_index              | deck_cards_stack_id_index  | 8       | nextcloud.ds.id         | 1    | Using where; Using index                     |
|    1 | SIMPLE      | s     | ref    | item_share_type_index,share_with_index | share_with_index           | 1023    | func                    | 36   | Using index condition; Using where           |
|    1 | SIMPLE      | f     | eq_ref | PRIMARY,fs_id_storage_size             | PRIMARY                    | 8       | nextcloud.s.file_source | 1    | Using where                                  |
|    1 | SIMPLE      | st    | eq_ref | PRIMARY                                | PRIMARY                    | 8       | nextcloud.f.storage     | 1    | Using where                                  |
+------+-------------+-------+--------+----------------------------------------+----------------------------+---------+-------------------------+------+----------------------------------------------+

@nickvergessen
Copy link
Member

I'm wondering if there is a specific reason that the MySQLExpressionBuilder does not implement any actual casting when using castColumn(). Is that something that we should rather do in the server then maybe?

Looks like it makes sense. It was not added so far as currently it was only needed for oracle and postgres when dealing with TEXT and having to convert that to CHAR for comparisons.

@juliusknorr juliusknorr force-pushed the bugfix/noid/share-query-opt branch 2 times, most recently from a1c1793 to eb1944d Compare December 30, 2021 10:39
@juliusknorr juliusknorr requested a review from Raudius April 12, 2022 08:07
@juliusknorr juliusknorr force-pushed the bugfix/noid/share-query-opt branch 2 times, most recently from 997cb28 to 47a655d Compare April 12, 2022 08:11
@juliusknorr juliusknorr changed the title Use explicit cast to make use of index [stable23] Use explicit cast to make use of index Apr 12, 2022
@juliusknorr juliusknorr changed the base branch from master to stable23 April 12, 2022 08:12
@juliusknorr
Copy link
Member Author

Changed the base to 23 since on 24 we have this fixed in the server.

@juliusknorr
Copy link
Member Author

/backport to stable22

@juliusknorr
Copy link
Member Author

/backport to stable21

@juliusknorr juliusknorr force-pushed the bugfix/noid/share-query-opt branch from 47a655d to c04e5c1 Compare April 12, 2022 08:13
$cardIdExpression = $qb->createFunction('CAST(dc.id as CHAR)');
} else {
$cardIdExpression = $qb->expr()->castColumn('dc.id', IQueryBuilder::PARAM_STR);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The snippet is used twice in the code. I think it would be a good idea to create a temp function for that as that function will be removed in the coming version of nextcloud.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For newer server releases this will be taken care of in the query builder abstraction with nextcloud/server#30471, so I'd say we stick to those now for the bugfix only releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants