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

Don't want to overwrite sql_mode, but how is gone. #17370

Closed
kamipo opened this issue Oct 23, 2014 · 6 comments

Comments

@kamipo
Copy link
Member

commented Oct 23, 2014

By #16065 have been merged, sql_mode came to be always overwritten regardless of activerecord's strict_mode.

If sql_mode is not set properly in MySQL side, strict_mode is useful.

If sql_mode is set properly in MySQL side, there is no need to override sql_mode for each connection.

In MySQL 5.6, default my.cnf includes sql_mode=NO_ENGINE_SUBSTITUTION,STRICT_TRANS_TABLES.
(refs http://www.tocker.ca/2014/01/14/making-strict-sql_mode-the-default.html)

In MySQL 5.7.5, default sql_mode is ONLY_FULL_GROUP_BY,NO_ENGINE_SUBSTITUTION,STRICT_TRANS_TABLES.
(refs http://dev.mysql.com/doc/refman/5.7/en/sql-mode.html)

I think how want to not overwrite sql_mode that we needed.

@matthewd

This comment has been minimized.

Copy link
Member

commented Oct 23, 2014

Well, prior to that, we were already setting it when strict_mode was enabled, which is the default.

It's unfortunate that MySQL (as far as I'm aware) doesn't offer a syntax by which we can "append" to sql_mode. Any approach wherein we first check the current value will presumably mean an extra network round trip, every time we connect.

@kamipo

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2014

@matthewd Indeed, I think too that MySQL is should support a syntax by which we can append to sql_mode (as well as optimizer_switch). If not, it will not be able to achieve strict_mode efficiently without affecting existing sql_mode.

In practice, strict: false had been used for not overwrite sql_mode.
(relied on MySQL settings, regardless of whether they are set properly)

However, strict: false no longer be able to use for that by #16065.

#16065 was because fixes a test failure.
If you for fixes a test failure, why not be able to use other means?
(for example, use variables: { sql_mode: '' })

@matthewd

This comment has been minimized.

Copy link
Member

commented Oct 24, 2014

In practice, strict: false had been used for not overwrite sql_mode

Yeah.. but that was just a convenient lie: AR believed one thing was happening, while MySQL did something different.

Instead of restoring the confusion of strict: false actually meaning "maybe strict, maybe not.. we're not telling", I'd prefer to consider a different option or value, so everyone's on the same page.

strict: 'default'? please_do_not_set_the_sql_mode_because_it_is_already_fine: true?

Or maybe we can check the MySQL version, and only set sql_mode if:

  • strict: false and MySQL >= 5.6, or
  • strict: true and MySQL < 5.6.

The latter has the disadvantage that it re-introduces potential disagreement: historically, we were counting on the fact that MySQL's default sql_mode was non-strict, but also that the administrator hadn't changed it from the original default in my.cnf. This would re-introduce the latter assumption.

@kamipo

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2014

I found a good workaround than strict: false for does not affect the existing sql_mode.

production:
  adapter: mysql2
  database: foo_prod
  user: foo
  variables:
    sql_mode: :default

Probably would not be practical problem with this.

@rails-bot

This comment has been minimized.

Copy link

commented Mar 20, 2015

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 4-2-stable, 4-1-stable branches 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.

@zzak zzak removed the stale label May 26, 2015
kamipo added a commit to kamipo/rails that referenced this issue May 27, 2015
kamipo added a commit to kamipo/rails that referenced this issue May 27, 2015
@kamipo

This comment has been minimized.

Copy link
Member Author

commented May 28, 2015

#17654 was merged. Now supported for strict: :default:

production:
  adapter: mysql2
  database: foo_prod
  user: foo
  strict: :default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.