Rollback to LOWER comparison instead of (I)LIKE because of bug and poor Postgres performance #339

Merged
merged 1 commit into from Dec 7, 2012

Conversation

Projects
None yet
8 participants
@cris
Contributor

cris commented Nov 27, 2012

Hi, Ben.

I suggest to revert changes with LIKE back to LOWER, because this solution doesn't add any benefits and introduce subtle bug.

  1. (I)LIKE is used without any quotiong. This leads to login/email collision. For example, user1 has email: user1@gmail.com and user2 has email: user_@gmail.com. If user1 is registered before user2, user2 isn't able to login. Because query:
SELECT  "users".* FROM "users"  WHERE ("users".email ILIKE 'user_@gmail.com') LIMIT 1;

allways returns user1 record. Take a look on issue 321, they already obtain this email collision: binarylogic/authlogic/issues/#321

  1. For MySQL case-insensitive is usualy default, because Rails creates database with collation "utf8_general_ci", and with such collation search by string fields is case insensitive. So, MySQL doesn't benefit from LIKE, because it doesn't make sense for it to use such option. Plane find_by_#{field} does all in case-insensitive manner.

  2. For Postgres ILIKE only brings new issue instead of solving something.
    First of all, ILIKE can't use indexes. You can clearly see this via EXPLAIN:

EXPLAIN SELECT  "users".* FROM "users"  WHERE ("users".email ILIKE 'user_@gmail.com') LIMIT 1;
                            QUERY PLAN                             
-------------------------------------------------------------------
 Limit  (cost=0.00..846.95 rows=1 width=1903)
   ->  Seq Scan on users  (cost=0.00..32184.19 rows=38 width=1903)
         Filter: ((email)::text ~~* 'user_@gmail.com'::text)
(3 rows)

What is worse, now I can't use Postgres ability to create index on function. For previous version of Authlogic I've created index on expression: LOWER(email) and it provides me with required functionality.

explain SELECT  "users".* FROM "users"  WHERE (LOWER("users".email) = 'user_@gmail.com') LIMIT 1;
                                          QUERY PLAN                                          
----------------------------------------------------------------------------------------------
 Limit  (cost=0.00..8.48 rows=1 width=1903)
   ->  Index Scan using index_users_lower_email on users  (cost=0.00..8.48 rows=1 width=1903)
         Index Cond: (lower((email)::text) = 'user_@gmail.com'::text)

After update to the latest version of AuthLogic I lost such ability.

Please, apply this pull-request to handle all caveats.

@bri3d

This comment has been minimized.

Show comment Hide comment
@bri3d

bri3d Dec 6, 2012

+1. The ILIKE quoting issue is fairly serious, and switching back to LOWER seems like a good way to both fix it and improve performance on Postgres at the same time.

It looks like the original switch to LIKE was done for "performance" reasons on MySQL, but wasn't thought through completely as it actually degraded performance on all versions of Postgres.

(for reference here's the original LIKE commit)

cris/authlogic@ca37d8a

bri3d commented Dec 6, 2012

+1. The ILIKE quoting issue is fairly serious, and switching back to LOWER seems like a good way to both fix it and improve performance on Postgres at the same time.

It looks like the original switch to LIKE was done for "performance" reasons on MySQL, but wasn't thought through completely as it actually degraded performance on all versions of Postgres.

(for reference here's the original LIKE commit)

cris/authlogic@ca37d8a

@jstin

This comment has been minimized.

Show comment Hide comment
@jstin

jstin Dec 6, 2012

+1 this is a much better solution.

jstin commented Dec 6, 2012

+1 this is a much better solution.

binarylogic added a commit that referenced this pull request Dec 7, 2012

Merge pull request #339 from cris/back-to-lower
Rollback to LOWER comparison instead of (I)LIKE because of bug and poor Postgres performance

@binarylogic binarylogic merged commit d8c61db into binarylogic:master Dec 7, 2012

@maxjustus

This comment has been minimized.

Show comment Hide comment
@maxjustus

maxjustus Feb 20, 2013

FWIW this change destroys performance on MySQL, since the use of LOWER prevents MySQL from being able to employ indices at all.

FWIW this change destroys performance on MySQL, since the use of LOWER prevents MySQL from being able to employ indices at all.

@mperham

This comment has been minimized.

Show comment Hide comment
@mperham

mperham Feb 20, 2013

Well, this caused a production outage for us. When you use LOWER on a column in MySQL, you can't index that constraint. Every user lookup turned into a full table scan.

Well, this caused a production outage for us. When you use LOWER on a column in MySQL, you can't index that constraint. Every user lookup turned into a full table scan.

This comment has been minimized.

Show comment Hide comment
@bbhoss

bbhoss Feb 20, 2013

The solution is probably to detect Postgres and use LOWER there, for mysql and others use LIKE.

The solution is probably to detect Postgres and use LOWER there, for mysql and others use LIKE.

This comment has been minimized.

Show comment Hide comment
@cris

cris Feb 21, 2013

Contributor

@mperham, you don't need LOWER or ILIKE anyway. Please, read this article: http://railsware.com/blog/2012/12/07/speeding-up-authlogic-login-for-postgres-and-mysql/

ILIKE isn't needed for MySQL at all. Correct way - is to disable case_sensitive search and it will use plain equal-comparison which is by default case-insensitive in MySQL(if you use utf8_general_ci).
This should fix an issue:

class User < ActiveRecord::Base
  # ...
  acts_as_authentic do |c|
    c.validates_uniqueness_of_email_field_options case_sensitive: true
    c.validates_uniqueness_of_login_field_options case_sensitive: true
    # ... other settings
  end
  # ...
end
Contributor

cris replied Feb 21, 2013

@mperham, you don't need LOWER or ILIKE anyway. Please, read this article: http://railsware.com/blog/2012/12/07/speeding-up-authlogic-login-for-postgres-and-mysql/

ILIKE isn't needed for MySQL at all. Correct way - is to disable case_sensitive search and it will use plain equal-comparison which is by default case-insensitive in MySQL(if you use utf8_general_ci).
This should fix an issue:

class User < ActiveRecord::Base
  # ...
  acts_as_authentic do |c|
    c.validates_uniqueness_of_email_field_options case_sensitive: true
    c.validates_uniqueness_of_login_field_options case_sensitive: true
    # ... other settings
  end
  # ...
end

This comment has been minimized.

Show comment Hide comment
@mperham

mperham Feb 21, 2013

@cris How was I supposed to know to make that change when upgrading to 3.2? There's no changelog or any sort of upgrade notes.

@cris How was I supposed to know to make that change when upgrading to 3.2? There's no changelog or any sort of upgrade notes.

This comment has been minimized.

Show comment Hide comment
@tiegz

tiegz Mar 28, 2013

Collaborator

@mperham just opened a pull request that I think might fix it for you. It follows Rails implementation and only uses LOWER if it's a case-sensitive column. Thoughts?

#356

Collaborator

tiegz replied Mar 28, 2013

@mperham just opened a pull request that I think might fix it for you. It follows Rails implementation and only uses LOWER if it's a case-sensitive column. Thoughts?

#356

This comment has been minimized.

Show comment Hide comment
@tiegz

tiegz Mar 28, 2013

Collaborator

Also, good call about the changelog. Perhaps we should introduce one? :\

Collaborator

tiegz replied Mar 28, 2013

Also, good call about the changelog. Perhaps we should introduce one? :\

This comment has been minimized.

Show comment Hide comment
@mperham

mperham Mar 28, 2013

@tiegz I just added the config that @cris mentioned and it solved the performance problem. It does scare me that the default config has this terrible performance problem and all customers on mysql are expected to find and change this config. It would be nice to have it just work.

@tiegz I just added the config that @cris mentioned and it solved the performance problem. It does scare me that the default config has this terrible performance problem and all customers on mysql are expected to find and change this config. It would be nice to have it just work.

This comment has been minimized.

Show comment Hide comment
@mperham

mperham Mar 28, 2013

@tiegz and your PR looks great to me. 👏

@tiegz and your PR looks great to me. 👏

pixeltrix added a commit to alphagov/e-petitions that referenced this pull request Apr 28, 2015

Don't enforce case-sensitivity for admin usernames
As of Rails 4.2 enforcing case-sensitivity for usernames is broken in
Authlogic due to the commit rails/rails@a38e957. The app change was for
security reasons since at the time Authlogic was using LIKE in the
query generation. However this was fixed in binarylogic/authlogic#339.

- binarylogic/authlogic#444
- binarylogic/authlogic#453

We should leave it as case-insensitive as this is better UX
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment