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
Gracefully fallback on version migrations for sqlite < 3.7.11 #24685
Gracefully fallback on version migrations for sqlite < 3.7.11 #24685
Conversation
a56ed2b
to
95851d7
Compare
@vipulnsward Your code is much nicer than that I'm working on. |
"INSERT INTO #{sm_table} (version) VALUES ('#{version}');" | ||
}.join "\n\n" | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitting the insert methods means the caller is responsible for figuring out which to use. The caller is concerned with inserting versions, not with which insertion technique to use. Let's combine a single insert_versions(versions)
for them to call:
def dump_schema_information
insert_versions_sql ActiveRecord::SchemaMigration.order('version').pluck(:version)
end
def insert_versions_sql(versions)
sm_table = ActiveRecord::Migrator.schema_migrations_table_name
if supports_multi_insert?
…
else
…
end
end
def assume_migrated_upto_version(version, migrations_paths)
…
execute insert_versions_sql(versions)
end
95851d7
to
961ac79
Compare
@@ -29,6 +30,23 @@ def test_dump_schema_information_outputs_lexically_ordered_versions | |||
ActiveRecord::SchemaMigration.delete_all | |||
end | |||
|
|||
if current_adapter?(:SQLite3Adapter) | |||
%w{3.7.8 3.7.11 3.7.12}.each do |version_string| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added check for different versions <, <=, >
961ac79
to
85345ef
Compare
@@ -1026,7 +1047,7 @@ def assume_migrated_upto_version(version, migrations_paths) | |||
if (duplicate = inserting.detect {|v| inserting.count(v) > 1}) | |||
raise "Duplicate migration #{duplicate}. Please renumber your migrations to resolve the conflict." | |||
end | |||
execute "INSERT INTO #{sm_table} (version) VALUES #{inserting.map {|v| "('#{v}')"}.join(', ') }" | |||
insert_versions(versions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to execute
this SQL…
85345ef
to
b95155b
Compare
This breaks for sqlite versions < 3.7.11, which is especially the case on Ubuntu 12.04 LTS, that has SQLite version 3.7.9 as default. So we check for support for multi insert, before performing single query inserts, else fallback to older version of running multiple queries. [Vipul A M & Yasuo Honda]
b95155b
to
6e09828
Compare
Done with the changes. |
…sert Gracefully fallback on version migrations for sqlite < 3.7.11
@jeremy thank you for the patient review! |
Thank you, @vipulnsward! 👏 |
@vipulnsward Thank you for working on this! ❤️ ❤️ ❤️ |
👏 |
Follow up to rails#24685. `insert_versions_sql` is not public API.
It appears the fallback doesn't actually work and doesn't store all versions. See the output of this Travis run: https://travis-ci.org/publify/publify_core/builds/180931099, in particular the part where I list the migrations and the versions in the migrations table:
|
42dd233 changed INSERT INTO versions to run in 1 single query.
This breaks for sqlite versions < 3.7.11, which is especially the case on Ubuntu 12.04 LTS, that has SQLite version 3.7.9 as default.
So we check for support for multi insert, before performing single query inserts, else fallback to older version of running multiple queries.