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

[stable22] add a prefix index to filecache.path, attempt 2 #29324

Merged
merged 3 commits into from
Jan 5, 2022

Conversation

backportbot-nextcloud[bot]
Copy link

@backportbot-nextcloud backportbot-nextcloud bot commented Oct 19, 2021

⚠️ This backport had conflicts and is incomplete ⚠️

backport of #28541

@blizzz
Copy link
Member

blizzz commented Oct 20, 2021

are conflicts resolved?

@skjnldsv
Copy link
Member

skjnldsv commented Oct 21, 2021

are conflicts resolved?

Looks like it, the commits don't have backportbot-nextcloud's touche anymore 🤔

Apparently not

@icewind1991
Copy link
Member

resolved

* @throws Exception
* @since 23.0.0
*/
public function getDatabasePlatform();
Copy link
Member

Choose a reason for hiding this comment

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

introducing a new API in a backport ?

Copy link
Member

Choose a reason for hiding this comment

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

I've updated the @since to be correct for the backport but I don't know a clean way around it

Copy link
Member

Choose a reason for hiding this comment

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

What do we do about it now? A new API in a backport indeed looks strange, but otherwise it doesn't hurt and it seems to help with the issue of long/hanging file searches: #29377 #23835
I think people are waiting for this PR/backport.

Copy link
Member

Choose a reason for hiding this comment

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

Note that we have done backported public api's in the past like this.

I don't see a way around adding this and no real downside

Copy link
Member

Choose a reason for hiding this comment

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

@PVince81
Any objections?

@blizzz
Copy link
Member

blizzz commented Nov 10, 2021

moved to 22.2.2

The reason that `filecache.path` hasn't had an index added is the mysql limitation of ~1kb for indexeded fields,
which is to small for the `path`, however mysql supports indexing only the first N bytes of a column instead of the entire column,
allowing us to add an index even if the column is to long.

Because the index doesn't cover the entire column it can't be used in all situations where a normal index would be used, but it does cover the `path like 'folder/path/%'` queries that are used in various places.

Sqlite and Postgresql don't support prefix indexes, but they also don't have the 1kb limit and DBAL handles the differences in index creation.

Signed-off-by: Robin Appelman <robin@icewind.nl>
having the index work properly for the queries we need it for requires some additional options which dbal does not support at the momement.
to prevent making it harder to add the correct index later on we don't create the index for now on postgresql

Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
@blizzz blizzz merged commit d892610 into stable22 Jan 5, 2022
@blizzz blizzz deleted the backport/28541/stable22 branch January 5, 2022 10:05
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2022
9 tasks
This was referenced Jan 20, 2022
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.

8 participants