Someone in their eternal wisdom added NO_ENGINE_SUBSTITUTION to the TRADITIONAL SQL mode in MySQL 5.5. I can't fathom how a totally mysql specific behaviour one way or another is more traditional than the other but this makes it impossible to use MyISAM with MySQL 5.5 (it's a pita anyways as you need to add default-storage-engine=myisam to your my.cnf as I learned from a mysql bug report...). I have timed drush si -y minimal to be about 6.5s with InnoDB (per file, on tmpfs) and 2.8s with MyISAM(on tmpfs too). Knowing the hundreds of Drupal installs created while running tests this is no triffle matter for developers. It does not affect any users in any ways, what so ever it only cuts the time needed for tests in half on MySQL 5.5. I have copied the meaning of TRADITIONAL http://dev.mysql.com/doc/refman/5.1/en/server-sql-mode.html#sqlmode_trad... from here into the patch. It's the same in 5.0 and 5.5 -- just 5.5 also has this accursed mode.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

bfroehle’s picture

Issue tags: +Needs backport to D7

And MySQL just turned you down. I guess that leaves us with the patch in #1.

Crell’s picture

Status: Needs review » Needs work

*facepalm*

If we are explicitly specifying the modes, we should state why we're doing it that way rather than just using TRADITIONAL so that some well-meaning developer doesn't revert it for us later for code simplification.

chx’s picture

Status: Needs work » Needs review
FileSize
1.1 KB

Sure

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, MySQL :)

chx’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

webchick asked this to test this on every mysql version supported. That won't happen.

webchick’s picture

Status: Closed (won't fix) » Needs review

I just love being completely misrepresented.

I said "this could use some testing." It passes on MySQL 5.5. It passes on whatever the testbot is using. I'd love for a couple of people running MAMP or similar to click around a bit with this patch applied and make sure it doesn't break anything, since this is changing DatabaseConnection_mysql, which affects almost everyone.

chx’s picture

Status: Needs review » Closed (won't fix)
~/mysql-server/sql$ grep -r TRADITIONAL .
./sql_update.cc:    Generate an error (in TRADITIONAL mode) or warning
./sp.cc:    "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER',"
./sys_vars.cc:  if (sql_mode & MODE_TRADITIONAL)
./sys_vars.cc:  "ALLOW_INVALID_DATES", "ERROR_FOR_DIVISION_BY_ZERO", "TRADITIONAL",
./event_db_repository.cc:    "'ERROR_FOR_DIVISION_BY_ZERO','TRADITIONAL','NO_AUTO_CREATE_USER',"
./sql_class.h:#define MODE_TRADITIONAL                (MODE_ERROR_FOR_DIVISION_BY_ZERO*2)
./sql_class.h:#define MODE_NO_AUTO_CREATE_USER        (MODE_TRADITIONAL*2)
./log_event.h:    MODE_TRADITIONAL==0x10000000

Here's sys_vars.cc

  if (sql_mode & MODE_TRADITIONAL)
    sql_mode|= (MODE_STRICT_TRANS_TABLES | MODE_STRICT_ALL_TABLES |
                MODE_NO_ZERO_IN_DATE | MODE_NO_ZERO_DATE |
                MODE_ERROR_FOR_DIVISION_BY_ZERO | MODE_NO_AUTO_CREATE_USER |
                MODE_NO_ENGINE_SUBSTITUTION);
  return sql_mode;

This is the same as the docs say. What else can I say? Merely clicking around won't ever reveal any problems with this. Either we believe MySQL docs or won't fix this. I am seriously sick of the commit process right now.

chx’s picture

Status: Closed (won't fix) » Reviewed & tested by the community

Sorry didnt mean to crosspost.

chx’s picture

BTW this issue is a great demonstration (together with the update helper issue recently) of everything wrong with the core process right now. Originally this was committed on what grounds? Through testing of the MySQL SQL modes? Nope. It was commited because the docs said so, the maintainers believed the docs and now on the same grounds we won't commit a fix.

Edit: put your hands up if you fully grok what the current mode line does! I can attest that I don't. I can read the MySQL manual but that's it.

chx’s picture

Re-read #344575: Force MySQL to run in ANSI compatability mode. and tell me where the discussions of the modes that TRADITIONAL occured. (Hint: nowhere. We were talking ANSI. TRADITIONAL just sneaked in.)

chx’s picture

I have to admit making a serious mistake by re-entering core development at this point. Won't happen again for some time: I downgraded to MySQL 5.1, removed the my issues block from my dashboard and I will return to spending my time on relation for another month. We will see whether core re-emerges from development hell by then.

claar’s picture

Status: Reviewed & tested by the community » Needs review

Wow, I'm personally sorry to see you take a break from core dev, chx. You'll be missed and the project will be hindered by it -- I look forward to your return.

That said, I must assume that your frustrations run deeper than this one issue. Because what I see here from webchick is completely reasonable -- #344575: Force MySQL to run in ANSI compatability mode. was committed during active D7 development. It's irrelevant that it was committed on little evidence, since that's often where the bar is set at that point in the dev cycle. Well, reasonable for D7, at least.

Anyway, I was going to put this issue back at "needs review", but I have checked the patch against the mysql code posted in #8 and the docs and am convinced that this patch is solid -- I believe it's more than RBTC for D8, and personally I'm comfortable with it for D7 (for what my +1 is worth).

claar’s picture

Status: Needs review » Reviewed & tested by the community

Grr.

chx’s picture

No, it's completely unreasonable to say "we do not trust documentation" and then presuming there is easy testing that would reveal bugs in this. edit: that the bot woulndt reveal. There reason I won't fixed is that if we do not trust the MySQL documentation then the only way is to write complicated tests for each of the modes and run it on the hundred plus minor versions we support on every OS.

gadams’s picture

subscribing

Crell’s picture

For the record, I'm +1 on the patch in #4.

catch’s picture

#4: traditional_broken_55.patch queued for re-testing.

xjm’s picture

Tagging issues not yet using summary template.

marcingy’s picture

Have run this on windows 7 with mysql 5.5.8 on php 5.3.5 against the database and node creation tests and got a 100% pass rate

Dries’s picture

Version: 8.x-dev » 7.x-dev

Committed to 8.x.

It is not clear to me if there may be side-effects for existing Drupal 7 sites. I'm just moving it to 7.x for now.

chx’s picture

Well, existing sites, no, unless they rely on trying to create a table with Drupal and getting an error instead :) I do not think that's normal behaviour.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Well, creating content types and fields creates tables. As does installing modules.

Since asking for testing back in #7 (April), it looks like only marcingy actually did it, or at least did it and reported back to the issue (on Windows no less. Nice!). I played around some with my local MAMP-based setup and didn't hit any issues, and since the code sprints at DrupalCon, which were primarily focused on D8, happened without incident, this is probably good to go.

Committed and pushed to 7.x.

Automatically closed -- issue fixed for 2 weeks with no activity.