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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use bulk INSERT to insert fixtures #29504

Merged
merged 1 commit into from Jun 20, 2017

Conversation

Projects
None yet
3 participants
@kirs
Member

kirs commented Jun 20, 2017

This patch improves the performance of inserting fixtures from O(n) to O(1).
Previously it would require 50 queries to insert 50 fixtures into one table. Now it takes only one query per table.

Disabled on sqlite which doesn't support multiple inserts.

Some benchmarks:

With two tables, each with 1k records. Time to insert fixtures on MySQL:
before: 0.7s
after: 0.2s

While 1k fixtures may sound unrealistic, at Shopify scale we have hundreds of tables, each having up to 100 fixtures. In our app this patch dramatically improved the performance of test helper. This patch would be especially helpful for those apps who use fixtures(:all) (I heard it's the case for Basecamp too).

馃帀 馃帀 馃帀

Closes #26901

@rafaelfranca @matthewd

Show outdated Hide outdated ...verecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -526,8 +526,25 @@ def default_index_type?(index) # :nodoc:
index.using == :btree || super
end
def insert_fixtures(*)
without_sql_mode('NO_AUTO_VALUE_ON_ZERO') { super }

This comment has been minimized.

@kirs

kirs Jun 20, 2017

Member

In bulk insert, we rely on queries like INSERT INTO users (id, name) VALUES (DEFAULT, 'David'), (DEFAULT, 'Rafael'). This kind of query doesn't work with NO_AUTO_VALUE_ON_ZERO flag.

On MySQL adapter, we enable flag option by default. The solution is to temporary disable this flag to insert the fixtures.

@kirs

kirs Jun 20, 2017

Member

In bulk insert, we rely on queries like INSERT INTO users (id, name) VALUES (DEFAULT, 'David'), (DEFAULT, 'Rafael'). This kind of query doesn't work with NO_AUTO_VALUE_ON_ZERO flag.

On MySQL adapter, we enable flag option by default. The solution is to temporary disable this flag to insert the fixtures.

@@ -349,6 +349,12 @@ def foreign_keys(table_name)
end
end
def insert_fixtures(rows, table_name)

This comment has been minimized.

@kirs

kirs Jun 20, 2017

Member

Fallback in Sqlite that doesn't support bulk inserts.

@kirs

kirs Jun 20, 2017

Member

Fallback in Sqlite that doesn't support bulk inserts.

@rafaelfranca

This looks good to me. Could you pass rubocop on those files and add a CHANGELOG entry?

Use bulk INSERT to insert fixtures
Improves the performance from O(n) to O(1).
Previously it would require 50 queries to
insert 50 fixtures. Now it takes only one query.

Disabled on sqlite which doesn't support multiple inserts.
@kirs

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Jun 20, 2017

Member

Updated. Thanks for a quick review 鉂わ笍

Member

kirs commented Jun 20, 2017

Updated. Thanks for a quick review 鉂わ笍

@rafaelfranca rafaelfranca merged commit d37af71 into rails:master Jun 20, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate no new or fixed issues
Details

@kirs kirs deleted the kirs:fixtures-arel-bulk branch Jun 20, 2017

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jun 20, 2017

Use conventional fixture load
rails/rails#29504 implements bulk insert for fixture load.
Oracle database needs another syntax.
In the mean time this commit just falls back to conventional fixture load.

Refer rails/arel#482

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jun 20, 2017

Use conventional fixture load
rails/rails#29504 implements bulk insert for fixture load.
Oracle database needs another syntax.
In the mean time, this commit just falls back to conventional fixture load.

Refer rails/arel#482
@Envek

This comment has been minimized.

Show comment
Hide comment
@Envek

Envek Jun 25, 2017

Contributor

FYI, SQLite does support multi-row inserts since version 3.7.11 (released in 2012, see changelog), but in some CIs older versions are being used by default, so you need to upgrade SQLite first (like this: travis-ci/apt-package-whitelist#368 (comment))

Maybe it worth adding a version check in implementation for SQLite?

Contributor

Envek commented Jun 25, 2017

FYI, SQLite does support multi-row inserts since version 3.7.11 (released in 2012, see changelog), but in some CIs older versions are being used by default, so you need to upgrade SQLite first (like this: travis-ci/apt-package-whitelist#368 (comment))

Maybe it worth adding a version check in implementation for SQLite?

@kirs

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Jun 25, 2017

Member
Member

kirs commented Jun 25, 2017

@kirs

This comment has been minimized.

Show comment
Hide comment
@kirs

kirs Jun 30, 2017

Member

@Envek I just realized that there was another reason why I couldn't use SQLite for bulk insert.

As a placeholder for default values (those columns that were not specified in a fixture), we use DEFAULT keyword. For MySQL and Postgres the query looks like:

INSERT INTO people (id, name) VALUES (DEFAULT, "kir"), (DEFAULT, "alex");

I found that SQLite doesn't support DEFAULT placeholder per column value. It requires you to use either all defaults (INSERT people DEFAULT VALUES) either specify all values (INSERT people VALUES ("value", "for", "every", "column")).

Member

kirs commented Jun 30, 2017

@Envek I just realized that there was another reason why I couldn't use SQLite for bulk insert.

As a placeholder for default values (those columns that were not specified in a fixture), we use DEFAULT keyword. For MySQL and Postgres the query looks like:

INSERT INTO people (id, name) VALUES (DEFAULT, "kir"), (DEFAULT, "alex");

I found that SQLite doesn't support DEFAULT placeholder per column value. It requires you to use either all defaults (INSERT people DEFAULT VALUES) either specify all values (INSERT people VALUES ("value", "for", "every", "column")).

Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Jan 9, 2018

Remove the `without_sql_mode` method:
- This method was introduced in rails#29504 ([comment](rails#29504 (comment)))
- Our entire test suite seems to pass without having to disabling the NO_AUTO_ON_ZERO flag even with queries looking like `INSERT INTO users (id, name) VALUES (DEFAULT, 'David'), (DEFAULT, 'Rafael')` on auto incremented columns

kamipo added a commit to kamipo/rails that referenced this pull request Jan 25, 2018

Bring back ability to insert zero value on primary key for fixtures
Since #29504, mysql2 adapter lost ability to insert zero value on
primary key due to enforce `NO_AUTO_VALUE_ON_ZERO` disabled.
That is for using `DEFAULT` on auto increment column, but we can use
`NULL` instead in that case.

kamipo added a commit that referenced this pull request Jan 26, 2018

Bring back ability to insert zero value on primary key for fixtures (#鈥
鈥31795)

Since #29504, mysql2 adapter lost ability to insert zero value on
primary key due to enforce `NO_AUTO_VALUE_ON_ZERO` disabled.
That is for using `DEFAULT` on auto increment column, but we can use
`NULL` instead in that case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment