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

Regex did not match CREATE TABLE in all cases #37991

Merged
merged 1 commit into from Dec 23, 2019
Merged

Regex did not match CREATE TABLE in all cases #37991

merged 1 commit into from Dec 23, 2019

Conversation

UlrichThomasGabor
Copy link
Contributor

The regular expression did not match CREATE TABLE statements printed out by AWS Aurora MySQL 5.6 instances, because they lack the required space at that position. Now it is optional.

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @UlrichThomasGabor thanks for the PR, can you add a test for this behavior?

@UlrichThomasGabor
Copy link
Contributor Author

Hi @eileencodes, I am not sure how. I can provide the output of an SHOW CREATE TABLE query which is affected, if this helps.

@eileencodes
Copy link
Member

There's a table options test here: https://github.com/rails/rails/blob/master/activerecord/test/cases/adapters/mysql2/table_options_test.rb. That's probably a good place to add a test for this.

@UlrichThomasGabor
Copy link
Contributor Author

To make sure I understand you correctly, you want a test stubbing the create_table_info method to return an affected CREATE query string, which is then transformed into the ruby schema syntax to be checked for correctness?

@eugeneius
Copy link
Member

This looks like the same issue that was previously discussed in #37706 and #37726.

Is it possible to make a local installation of MySQL respond to SHOW CREATE TABLE in the same format? It doesn't seem to be specific to Amazon Aurora if MySQL on Digital Ocean is also affected.

@UlrichThomasGabor
Copy link
Contributor Author

@eugeneius Yes, these look like the same issue.

From this site I understand that the SHOW CREATE TABLE replies with the create statement the table was created with. Although, I do not know where the default ENGINE=innodb and other options should get lost on the way while creation (or later). But I also think this is not important. In my opinion, it could be, for whatever reason, that the create statement does not include table options and the function should be able to deal with it.

I do not know of options to modify or configure the output of SHOW CREATE TABLE.

@eugeneius
Copy link
Member

It seems like the NO_TABLE_OPTIONS SQL mode is probably the culprit here:

https://dev.mysql.com/doc/refman/5.6/en/sql-mode.html#sqlmode_no_table_options

Do not print MySQL-specific table options (such as ENGINE) in the output of SHOW CREATE TABLE.

A test for this change could temporarily set that SQL mode on the connection.

@UlrichThomasGabor
Copy link
Contributor Author

@eugeneius Yes, this option has an effect on the output of SHOW CREATE TABLE (at least on the mysql server I tried it just now). I will add a test later.

@UlrichThomasGabor
Copy link
Contributor Author

TheNO_TABLE_OPTIONS sql mode does not exist in current versions of mysql anymore, therefore the test would only work with old mysql instances… I don't think this is useful.

Stubbing create_table_info also does not work, because it's private.

Other ideas anyone?

@UlrichThomasGabor
Copy link
Contributor Author

Since nobody seems to have a better idea, I just added a comment hinting towards the sql mode.

@kamipo
Copy link
Member

kamipo commented Dec 22, 2019

Since We support MySQL >= 5.5.8, it should have a regression test even if that is no longger issue on latest MySQL version.

@UlrichThomasGabor
Copy link
Contributor Author

@kamipo One needs an old MySQL version to set the old SQL mode. If you tell me how to add a test, which requires a specific (range of) MySQL version(s), I am happy to add it.

@kamipo
Copy link
Member

kamipo commented Dec 22, 2019

  def test_no_table_options_sql_mode
    skip "As of MySQL 5.7.22, NO_TABLE_OPTIONS is deprecated. It will be removed in a future version of MySQL." if ActiveRecord::Base.connection.database_version >= "5.7.22"

    # ... please write a test below
  end

@kamipo
Copy link
Member

kamipo commented Dec 23, 2019

Can you squash commits into one?

The regular expression did not match CREATE TABLE statements printed out by AWS Aurora MySQL 5.6 instances, because they lack the required space at that position.
@UlrichThomasGabor
Copy link
Contributor Author

@kamipo Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants