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

SQLite adapter allows indexes on non-existent columns #27782

Closed
ersinakinci opened this issue Jan 23, 2017 · 3 comments
Closed

SQLite adapter allows indexes on non-existent columns #27782

ersinakinci opened this issue Jan 23, 2017 · 3 comments

Comments

@ersinakinci
Copy link

ersinakinci commented Jan 23, 2017

Steps to reproduce

  1. Create a migration on a Rails project that's using SQlite.
  2. Add an index in the migration on a table column that doesn't exist. Example:
class Reports < ActiveRecord::Migration
  def change
    create_table :reports do |t|
      t.integer :year, null: false
      t.integer :metric, null: false

      t.timestamps null: false

    end
    add_index :reports, :yearz_doesnt_exist
  end
end

(Or place add_index within the create_table block as t.add_index :yearz_doesnt_exist.)

  1. Run the migration.

Expected behavior

The migration should fail and a new index shouldn't be created.

Actual behavior

The migration succeeds and a new index is created. Moreover, if you log STDOUT, you'll see the SQL changes:

Compewter:project user$ rake log db:migrate
D, [2017-01-23T13:08:13.558669 #40736] DEBUG -- :   ActiveRecord::SchemaMigration Load (0.1ms)  SELECT "schema_migrations".* FROM "schema_migrations"
I, [2017-01-23T13:08:13.562632 #40736]  INFO -- : Migrating to CreateReports (20170123194647)
D, [2017-01-23T13:08:13.562991 #40736] DEBUG -- :    (0.1ms)  begin transaction
== 20170123194647 CreateApplicantFunnelReports: migrating =====================
-- create_table(:reports)
D, [2017-01-23T13:08:13.564033 #40736] DEBUG -- :    (0.4ms)  CREATE TABLE "reports" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "year" integer NOT NULL, "metric" integer NOT NULL, "created_at" datetime NOT NULL, "updated_at" datetime NOT NULL)
   -> 0.0010s
-- add_index(:reports, :yearz_doesnt_exist)
D, [2017-01-23T13:08:13.564242 #40736] DEBUG -- :    (0.1ms)  select sqlite_version(*)
D, [2017-01-23T13:08:13.564561 #40736] DEBUG -- :    (0.1ms)  CREATE  INDEX "index_reports_on_yearz_doesnt_exist" ON "reports" ("yearz_doesnt_exist")
   -> 0.0005s
== 20170123194647 CreateReports: migrated (0.0015s) ============

D, [2017-01-23T13:08:13.568450 #40736] DEBUG -- :   SQL (0.2ms)  INSERT INTO "schema_migrations" ("version") VALUES (?)  [["version", "20170123194647"]]
D, [2017-01-23T13:08:13.570113 #40736] DEBUG -- :    (1.4ms)  commit transaction
D, [2017-01-23T13:08:13.573312 #40736] DEBUG -- :   ActiveRecord::SchemaMigration Load (0.1ms)  SELECT "schema_migrations".* FROM "schema_migrations"
D, [2017-01-23T13:08:13.575355 #40736] DEBUG -- :    (0.1ms)              SELECT sql
            FROM sqlite_master
            WHERE name='index_reports_on_yearz_doesnt_exist' AND type='index'
            UNION ALL
            SELECT sql
            FROM sqlite_temp_master
            WHERE name='index_reports_on_yearz_doesnt_exist' AND type='index'

After some experimentation, I found the problematic line: CREATE INDEX "index_reports_on_yearz_doesnt_exist" ON "reports" ("yearz_doesnt_exist") The problem lies in the double quotes around the column name. For whatever reason, in SQLite, the following statements behave differently:

CREATE INDEX index_reports_on_yearz_doesnt_exist ON reports (yearz_doesnt_exist);
CREATE INDEX index_reports_on_yearz_doesnt_exist ON reports ('yearz_doesnt_exist');
CREATE INDEX index_reports_on_yearz_doesnt_exist ON reports ("yearz_doesnt_exist");

The first two will fail but the last will succeed. I've written to the SQLite mailing list asking why this is, but until I receive a response, you may be interested to know the output of index_list and index_info:

sqlite> PRAGMA index_list(reports);
0|index_reports_on_yearz_doesnt_exist|0|c|0
sqlite> PRAGMA index_info(index_reports_on_yearz_doesnt_exist);
0|-2|

Note that the output from index_info indicates that our index is on column -2, which of course doesn't exist. Very weird.

This may well be a SQLite problem. On the other hand, using double quotes in SQL when it isn't necessary is usually frowned upon. Here's the relevant code from the abstract connector in Rails:

def add_index(table_name, column_name, options = {})
  index_name, index_type, index_columns, index_options = add_index_options(table_name, column_name, options)
  execute "CREATE #{index_type} INDEX #{quote_column_name(index_name)} ON #{quote_table_name(table_name)} (#{index_columns})#{index_options}"
end

Note that the Postgres connector seems to do this a little differently:

def quote_column_name(name) # :nodoc:
  @quoted_column_names[name] ||= PGconn.quote_ident(super).freeze
end

What probably needs to be done is that SQLite-specific code for adding indexes needs to be written. I'm happy to take a crack at it and submit a PR.

System configuration

Rails version: 4.2.7

Ruby version: 2.3.3p222

SQLite version: 3.14.0

@ersinakinci
Copy link
Author

ersinakinci commented Jan 25, 2017

From D. Richard Hipp, the creator of SQLite, in an email response on the mailing list to my inquiry:

SQLite has a (mis-)feature that double-quoted names that cannot be
resolved to a table or column name are treated as strings.  This was a
very early design decision, made long before SQLite went viral and
found itself running in everything device on the planet, and was
intended to make SQLite more compatible with MySQL, which at the time
was the most widely deployed database engine in the world.  I regret
that choice now, but I cannot undo it without breaking backwards
compatibility.

So there you have it. I still have some questions that I'm following up on (e.g., what does it mean when you CREATE INDEX on a string as opposed to a column?), but what's germane to Rails is that we should change the query such that it doesn't use quotes or uses single quotes for the column name when creating an index.

I will work on this and issue a PR within the week.

EDIT: I got some clarity about what's going on. Consider these three cases again:

CREATE INDEX index_reports_on_yearz_doesnt_exist ON reports (yearz_doesnt_exist);
CREATE INDEX index_reports_on_yearz_doesnt_exist ON reports ('yearz_doesnt_exist');
CREATE INDEX index_reports_on_yearz_doesnt_exist ON reports ("yearz_doesnt_exist");
  1. In the first case, the query fails because there's no column called yearz_doesnt_exist.
  2. In the third case, the query succeeds because, as explained above, SQLite interprets double quotes first as column names, but if the column doesn't exist then it treats it as a string. Since the column "yearz_doesnt_exist" can't be found, what's happening is that we've created a "computed index," i.e. an index on the string literal "yearz_doesnt_exist". This might seem silly (certainly, it doesn't do anything useful), but as someone on the mailing list explained to me a string literal is still a valid expression and indexes can be created on valid expressions.
  3. The second case is perplexing but beyond the scope of Rails. Unlike double quotes, single quotes are always treated as string literals by SQLite. In theory, the second case should never fail to create a computed index on the string "yearz_doesnt_exist". For some reason, however, SQLite interprets the single quote in this context as a column name and fails. A bug report has been filed with the SQLite team and it's being investigated.

The point is that the proper solution here is to not use any quotes for column names when creating indexes in SQLite. Double quotes have unreliable behavior and single quotes' behavior is reliable but also probably a bug.

@maclover7
Copy link
Contributor

@earksiinni Agree that we should change Rails to better suit SQLite. Please do send a PR!

@rails-bot
Copy link

rails-bot bot commented May 5, 2017

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot closed this as completed May 12, 2017
casperisfine pushed a commit to casperisfine/sqlite3-ruby that referenced this issue May 15, 2022
Ref: rails/rails#45101
Ref: rails/rails#27782
Ref: https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted

Sqlite has a really unfortunate behavior around double quotes. It first
tries to consider them as column names, but if they don't exist it then consider
them as string literals.

Because of this, typos can silently go unnoticed. It would be very useful
for Active Record if this behavior could be disabled.
casperisfine pushed a commit to casperisfine/sqlite3-ruby that referenced this issue May 15, 2022
Ref: rails/rails#45101
Ref: rails/rails#27782
Ref: https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted

Sqlite has a really unfortunate behavior around double quotes. It first
tries to consider them as column names, but if they don't exist it then consider
them as string literals.

Because of this, typos can silently go unnoticed. It would be very useful
for Active Record if this behavior could be disabled.
casperisfine pushed a commit to casperisfine/sqlite3-ruby that referenced this issue May 25, 2022
Ref: rails/rails#45101
Ref: rails/rails#27782
Ref: https://www.sqlite.org/quirks.html#double_quoted_string_literals_are_accepted

Sqlite has a really unfortunate behavior around double quotes. It first
tries to consider them as column names, but if they don't exist it then consider
them as string literals.

Because of this, typos can silently go unnoticed. It would be very useful
for Active Record if this behavior could be disabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants