Edgewall Software
Modify

Opened 12 years ago

Closed 11 years ago

Last modified 6 years ago

#10993 closed defect (fixed)

MySQL: Upgrade should enforce new tables with InnoDB

Reported by: anatoly techtonik <techtonik@…> Owned by: Jun Omae
Priority: normal Milestone: 1.0.2
Component: database backend Version: 0.12-stable
Severity: major Keywords: mysql
Cc: Branch:
Release Notes:

Verify expected storage engine, charset and collation when initenv and db upgrades if MySQL

API Changes:
Internal Changes:

Description

During upgrade process, when Trac creates tables for MySQL, it should create a test temporary table and check if its type is MyISAM. If it is MyISAM, then Trac should enforce InnoDB engine on all newly created tables during upgrade process.

MyISAM table types breaks transactions, which becomes a source of magical bugs like #8067.

Attachments (0)

Change History (11)

comment:1 by Christian Boos, 12 years ago

Keywords: mysql added
Milestone: next-major-releases

Good idea, PatchWelcome (make that an IEnvironmentSetupParticipant.environment_needs_upgrade check, in the MySQLConnector).

comment:2 by Jun Omae, 11 years ago

Milestone: next-major-releases1.0.2
Owner: set to Jun Omae
Status: newassigned

Proposed changes can be found in log:jomae.git:ticket10993.

The following are verified on init_db and environment_needs_upgrade of MySQLConnector:

  • MyISAM engine of table isn't used
  • utf8_bin or utf8mb4_bin is used as collation of table
  • default_storage_engine variable isn't MyISAM
  • character_set_database and collation_database variables are one of (('utf8', 'utf8_bin'), ('utf8mb4', 'utf8mb4_bin'))

comment:3 by Peter Suter, 11 years ago

Looks good to me.

Are you concerned about temporary tables? Since 5.6.3 someone could sneakily set default_tmp_storage_engine. (Or other weird things, like use a CSV engine?)

comment:8:ticket:10661 proposed to:

to do a "safety check" (creating a (temp) table, verify the encoding; try to insert a few rows in a transaction (with upper/lower case conflicts); rollback and check if all rows are gone).

But I think your proposed changes are simpler and sufficient for the most likely problem of old MySQL with MyISAM default.

in reply to:  3 comment:4 by Jun Omae, 11 years ago

Replying to psuter:

Are you concerned about temporary tables? Since 5.6.3 someone could sneakily set default_tmp_storage_engine. (Or other weird things, like use a CSV engine?)

Okay. Updated log:jomae.git:ticket10993 branch.

I've added the verifications for default_tmp_storage_engine and the engine is neither EXAMPLE, ARCHIVE, CSV nor ISAM that they don't support transactions in obvious.

comment:5 by Jun Omae, 11 years ago

Release Notes: modified (diff)

Pushed in [12148] and merge to trunk in [12149].

comment:6 by Jun Omae, 11 years ago

Resolution: fixed
Status: assignedclosed

comment:7 by Peter Suter, 11 years ago

Nice.

I closed #8089 and #10661 as duplicates.

comment:8 by Ryan J Ollos, 6 years ago

Edited MySqlDb@77.

This issue might be revisited in the future, for now I'm just summarizing the discussion in gdiscussion:trac-users:MO_KSSQMdnU.

The requirement to set default_storage_engine and default_tmp_storage_engine in my.cnf could be removed if:

  • The storage engine was specified in a trac.ini variable such as [trac] mysql_storage_engine or [mysql] storage_engine
  • MySQLConnector.to_sql was modified to include the ENGINE table option in TABLE CREATE statements
  • The modules in the upgrade package were modified to use the ENGINE table option either directly or indirectly by use of a helper function (such as MySQLConnector.to_sql)

comment:9 by Jun Omae, 6 years ago

I noticed we could use default_storage_engine and default_tmp_storage_engine in session variables, rather than my.cnf.

SET @@session.default_storage_engine = 'InnoDB';

PoC: [f877c7e1d/jomae.git] (jomae.git@t10993_storage_option)

Last edited 6 years ago by Jun Omae (previous) (diff)

comment:10 by Ryan J Ollos, 6 years ago

Setting storage engine for session looks like a good idea. Should we make a new ticket and target to a 1.3.x milestone?

comment:11 by Jun Omae, 6 years ago

Created #13080 targeted to next-dev-1.3.x.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.