MySQL create_table loses ENGINE default info if options doesn't provide it.

[albertoalmagro]

Steps to reproduce

In ActiveRecord::ConnectionAdapters::AbstractMysqlAdapter the default value "ENGINE=InnoDB" for options is provided. This value is propagated unless the create_table method receives another options String, in that case the ENGINE reference is lost.

How to reproduce

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails"
  gem "arel", github: "rails/arel"
  gem "mysql2"
end

require "active_record"
require "minitest/autorun"
require "logger"

LOG_FILE_PATH = "mysql_create_table_engine.log"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "mysql2", database: "test", username: "root", password: "")
ActiveRecord::Base.logger = Logger.new(LOG_FILE_PATH)

class CreateUsers < ActiveRecord::Migration[5.2]
  def change
    create_table :users, options: "DEFAULT CHARSET=utf8" do |t|
      t.string :name
      t.string :email
    end
  end
end

class CreateProducts < ActiveRecord::Migration[5.2]
  def change
    create_table :products do |t|
      t.string :name
      t.integer :price
    end
  end
end

class BugTest < Minitest::Test
  def test_create_table_loses_engine_info
    create_users_migration = CreateUsers.new
    create_users_migration.change

    logged_info = File.read(LOG_FILE_PATH)
    assert !logged_info.include?("ENGINE")

    create_users_migration = CreateProducts.new
    create_users_migration.change

    logged_info = File.read(LOG_FILE_PATH)
    assert logged_info.include?("ENGINE")

    File.delete(LOG_FILE_PATH)
  end
end

Expected behavior

Default ENGINE=InnoDB is provided by default unless options string provides a different ENGINE, in which case this new engine will override the default one, as documented in Rails Guides. See last paragraph at point 3.1 in Rails Migrations Guides.

Actual behavior

The generated SQL does NOT contain default ENGINE=InnoDB information if an options string is provided.

System configuration

Rails version: Edge

Ruby version: 2.4.1

I am preparing a fix which will satisfy the expected behavior. Please confirm the issue and I will submit a PR.

[eileencodes]

@albertoalmagro that definitely sounds like a bug. If you have time and want to work on this please do open an PR. If not let us know so someone else can work on it.

[albertoalmagro]

Hi @eileencodes, thanks for your time reviewing this.

Yes, I do want to work on fixing it. I will submit a PR and link it with this issue as soon as I have it ready.

[kamipo]

The reason for explicitly passing ENGINE=InnoDB is because the default storage engine was MyISAM in MySQL 5.1. This is not absolutely necessary in MySQL 5.5 unless you intentionally change the server's default configuration.
And MySQL 5.1 had already been EOL in Dec 2013. So I'm not excited to keep the passing default for a future version of Rails.

https://dev.mysql.com/doc/refman/5.5/en/innodb-introduction.html

[albertoalmagro]

Thanks @kamipo, that is interesting. With that information I would then suggest the following:

1. Removing the default value completely for the following reasons:
   a. Providing default ENGINE=InnoDB doesn't seem to be necessary anymore, and
   b. Now we aren't passing it consistently, providing custom options has the side effect of removing ENGINE=InnoDB.
2. Update related documentation and guides.

Please tell me if that would be OK for you both.

[sgrif]

Updating the documentation seems best to me

[albertoalmagro]

@sgrif thanks. Please note I proposed to do both actions simultaneously. Of course only updating documentation could also be an option by itself, but I somehow feel uncomfortable with the idea of leaving options handling as it is now.

[sgrif]

I'm in favor of removing the default, but only if implemented so that existing migrations are not affected. However, this has issues with schema.rb potentially being inconsistent unless we are already always dumping the engine option there (I don't recall if we are or not, but I know that dumping MySQL specific stuff is a point of contention for some folks).

Either way, I think updating the docs is something that we can do as a separate item, as there's no downside

[albertoalmagro]

Thanks @sgrif. Yes I will definitely update the documentation.

I have just double checked what you mention about schema.rb with a schema generated by a Rails production app using MySQL so that we can be sure. I didn't find any reference to the Engine or to stuff only related to MySQL.

This change would affect to ActiveRecord's MySQL adapters, which generate the SQL to create the table. This generated SQL code is itself specific to MySQL. I assume MySQL specific code would remain contained to the places it currently is.

[albertoalmagro]

I was analyzing the adapter code and I realized this change would also affect MariaDB, which also uses this default value on create_table.

From MariaDB 10.2 default engine is InnoDB, but MariaDB version 10.1, which will be supported until October 2020, uses the engine XtraDB. See here.

Having this into account, I would keep default ENGINE option unless another ENGINE is provided, otherwise MariaDB would start behaving inconsistently.

[kamipo]

XtraDB is Percona's patched InnoDB, that is used as ENGINE=InnoDB.
If you use MariaDB 5.5 or higher, the default storage engine is also InnoDB.
So the default options is also not absolutely necessary in MariaDB 5.5 unless you intentionally change the server's default configuration.

[albertoalmagro]

PR is ready for review, please check #30994

[albertoalmagro]

@kamipo according to this documentation MariaDB 5.5 comes with both engines and XtraDB is the default one, not InnoDB: https://mariadb.com/kb/en/library/changes-improvements-in-mariadb-55/

[kamipo]

XtraDB is used as ENGINE=InnoDB.