Remove lower(uid) and lower(gid) from SQL queries to use the database index

[cdamken]

@scolebrook found some good optimizations tips here:

We've just upgraded our system to use Percona XtraDB Cluster with Cluster Control from Severalnines. We're seeing lots of queries being reported that perform full table scans. I've found some queries in 7.0.4 that could be optimized.

/core/lib/private/group/database.php line 175
The WHERE LOWER(gid) should be replaced with using lower in the insert and updates for this table so that index can be used. The index on gid is being ignored because the result of a function is being compared, not the column.

Same in /core/lib/private/user/database.php line 130 in an UPDATE query.
And line 159, this one also should see displayname added as an index on the users table.
Line 180, 210, 234 also with uid.

I see this referred to in #3939. This PR was against 6 and was never merged. It was closed about 1 year ago. The approach here was to make a column to hold a lower case version of uid in the users table. It would be populated during an upgrade and changes to this table would include writing a lower case version to this column at insert/update time. Uid is a common key and is used in many tables but the ideal solution would see this be changed in all tables at once rather than store a duplicate of the data in an additional column. However that may impact 3rd party apps so a change that big should co-inside with a major release. The lower case column was a good compromise.

Could you please bring this issue up again for us. It is poor design and very inefficient. We use ldap and even though there are hundreds of thousands of these queries daily it doesn't impact us measurably. But it produces a lot of noise in our reporting system which can hide other issues that do actually impact our performance.

@MorrisJobke @iliyasbasir

00002474

[MorrisJobke]

@DeepDiver1975 I will try to have a look at this in the early stage of 8.1. Then I try to come up with a full migration and we have the full release cycle to find possible problems. Does this sound reasonable?

[MorrisJobke]

@DeepDiver1975 Please assign to the milestone if you accept this.

[DeepDiver1975]

data migration will be fun - repair step required.

In contrary to the approach taken in #3939 I'd lower-case the existing field and not introduce a new one.
We have to very carefull where the user id is referenced in (repeating myself - FKs would help).

THX

[PVince81]

"userid" is referenced in the "share_with" and "owner" columns in oc_share. Third party apps might also use such field, so might be tricky to migrate.

[DeepDiver1975]

8.2 for me - we need to discuss quite some topic in the area of user management

[bboule]

@MTRichards @cmonteroluque seems like something we need to resolve as it was opened by EE user thoughts?

[MTRichards]

@cmonteroluque can we have an engineer evaluate this, or maybe @DeepDiver1975 ? I can't say if this is important or not based on the current state of oC.

[MorrisJobke]

@cmonteroluque can we have an engineer evaluate this, or maybe @DeepDiver1975 ? I can't say if this is important or not based on the current state of oC.

The problem here is that the userid is used more often in columns that doesn't look like userid columns. For example are there now background jobs for trashbin expiry - they hold the userid as argument in the argument column of oc_jobs. The problem here is that the argument is a serialized PHP object, which makes a repair step harder.

Nothing is impossible, but I think this one will be a tough one.

[DeepDiver1975]

The user accounts are a mess. period. Needs a bigger sledgehammer #21282 - which is a potential work package for 9.1.

[MorrisJobke]

The user accounts are a mess. period. Needs a bigger sledgehammer #21282 - which is a potential work package for 9.1.

As this 🔨 was moved to 9.2 I will move this ticket here too. The blue ticket doesn't really apply here. I will degrade to purple.

[MorrisJobke]

As this 🔨 was moved to 9.2 I will move this ticket here too.

I meant #23558

[PVince81]

@DeepDiver1975 @butonic would move to backlog for now. We anyway need #21282 first.

[hodyroff]

Performance against LDAP for users got solved and for groups it is in progress. Done as far as I can see. If there are new concrete areas for improvement, please open a new ticket.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.