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

for columns with uppercase letters, generated indexes don't hit #5128

Closed
kevinburkeshyp opened this issue Apr 2, 2015 · 13 comments
Closed

Comments

@kevinburkeshyp
Copy link

Not sure if this is fixed in the latest version or what. The following attributes in a Sails model

    userId: {
      index: true,
      type:  'text',
      size:  50
    }

generates the following index:

CREATE INDEX "indexname" on "tablename" ("userId");

However, the queries generated from WL/sails-postgresql call LOWER("tablename"."userId"), so they completely miss the index for queries which find by ID:

                                               QUERY PLAN
---------------------------------------------------------------------------------------------------------
 Limit  (cost=0.00..15.00 rows=1 width=289)
         ->  Seq Scan on tablename  (cost=0.00..15.00 rows=1 width=289)
               Filter: ((lower("userId") = 'usr_123'::text))

I know there's some work going on to add case sensitivity, but it's not on yet, & this could trip people up if they are using the automagicalmigrate setting.

@particlebanana
Copy link
Contributor

Yeah I added a case sensitive flag so you can use the adapter without all the lower nonsense. You can turn it on like so in your connection config:

postgresql_connection: {
  adapter: 'sails-postgresql',
  url: process.env.DATABASE_URL,
  wlNext: {
    caseSensitive: true
  }
}

@kevinburkeshyp
Copy link
Author

Yeah... unfortunately when you have ~40 tables that have existing indexes on the lowercase column names, switching to case sensitivity isn't trivial. Would be nice to be able to add case sensitivity table-by-table, I added #2804 to discuss further.

@particlebanana
Copy link
Contributor

Table-by-table would be interesting but harder to configure. Whats the issue with case-sensitivity? You have a lowercase column, say username, and want to query using the index right?

select * from account where username = 'foo'

which translates to the following waterline query:

Account.find({
  username: 'Foo'.toLowerCase()
}).exec(....)

@particlebanana
Copy link
Contributor

Or what does the sql query need to look like?

@kevinburkeshyp
Copy link
Author

I guess either the queries and the indexes need to use lower() or neither
does. Not interested in case sensitivity so much as I am hitting the index.
In an ideal world I wouldn't have to call lower() on anything, though,
indexes or queries.

-kevin

On Apr 1, 2015, at 18:30, Cody Stoltman notifications@github.com wrote:

Or what does the sql query need to look like?


Reply to this email directly or view it on GitHub
https://github.com/balderdashy/sails-postgresql/issues/142#issuecomment-88682404
.

@particlebanana
Copy link
Contributor

Yeah in order to hit indexes on strings you have to add that flag above (which is why I added it actually). That flag bypasses the normal behavior of WL which is to wrap everything in a case-insensitve wrapper. But then now this makes all your queries case sensitive. Although all postgresql queries are case sensitive normally.

In order to keep waterline from branching api's we made it do case insensitive queries. MySql was the first adapter and unfortunately defined the api for the others so here we are. I guess we could wrap a layer around the query builder so that if a field has an index make it case-sensitive but that just seems like a headache and would be confusing for people as to why certain fields are case sensitive and others are not.

Maybe the best solution is to add a flag on the model definition so a single attribute is case-sensitive and it's explicitly defined. God knows how we would implement that in MySql though.

@tjwebb
Copy link
Contributor

tjwebb commented Apr 2, 2015

However, the queries generated from WL/sails-postgresql call LOWER("tablename"."userId"), so they completely miss the index for queries which find by ID:

Postgres objects are case insensitive. Are you sure this index isn't working? As far as columns are concerned in pg, userId == userid == UserID

@kevinburkeshyp
Copy link
Author

Yes, positive. If the column is named "userId" (capital I) and you type

select * from users where userId='usr123'

The default behavior of Postgres is to search for a column named "userid"
(lowercase). In fact it will error because no such column exists. You need
to put double quotes around the column name to have it search for and match
an uppercase column; this might be where the confusion comes from.

It's pretty easy to double check this, add an index to a table on an
uppercased column, put some rows in it, run

  EXPLAIN SELECT * FROM LOWER("tablename"."userId")

and verify that the result says Seq Scan. Then add an index on
lower("userId"), run the same query, and verify that you get "Index Scan"
back out.

You can verify the query that Sails is generating by enabling query logging
on your local instance; happy to help if you need it.

Thank you for not asking "why are you using capital letter column names",
which would also solve this problem, a mass column/model rename is not
practical at this point.

-kevin

On Apr 2, 2015, at 03:00, Travis Webb notifications@github.com wrote:

However, the queries generated from WL/sails-postgresql call
LOWER("tablename"."userId"), so they completely miss the index for queries
which find by ID:

Postgres objects are case insensitive. Are you sure this index isn't
working?


Reply to this email directly or view it on GitHub
https://github.com/balderdashy/sails-postgresql/issues/142#issuecomment-88854187
.

@kevinburkeshyp
Copy link
Author

(The confusion may come from, you can create a column and spell it create table foo (userId text); Postgres will create a lowercase column name - you need to double-quote the column name on creation as well)

@particlebanana
Copy link
Contributor

Ah so the issue is only on column name? Will it hit the index if we do the following query?

select * from users where "userId" = LOWER('usr123');

If it's just an issue with column name we can probably remove that completely.

@kevinburkeshyp
Copy link
Author

Hmm, it's possible.. you're suggesting moving the LOWER() to the query parameter instead of the column name? That should hit the index. I'm not sure if it would have the same behavior, at a minimum it would be good to have tests around it.

@SamProtas
Copy link

I'm not sure if this is related, but anything that uses waterline-sequel will miss any string index unless you're using

wlNext: { caseSensitive: true }

in your connection definition.

This is the current expected behavior as illustrated by:
https://github.com/balderdashy/waterline-sequel/blob/56370d074076ba090796973053d71222ec5ff25f/test/queries/simpleSelectWhere.js#L33

By calling calling the LOWER() function on the column that the where query is compared against, the index is thrown out. Confirmed in sails-postgresql and sails-mysql.

@sailsbot
Copy link

Thanks for posting, @kevinburkeshyp. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

  • review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
  • create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

@johnabrams7 johnabrams7 transferred this issue from balderdashy/sails-postgresql Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants