search

Geeklog-Core / geeklog

Geeklog - The Secure CMS.
https://www.geeklog.net
24 stars 19 forks source link

Trying to upgrade from 2.2.0 to 2.2.1 fails with InnoDB when database character set is 'utf8mb4' #1027

Closed mystralkk closed 4 years ago

mystralkk commented 4 years ago 
Fatal error: 1071: Specified key was too long; max key length is 1000 bytes in path\to\geeklog\system\databases\mysqli.class.php on line 468

SQL in question is:

$sql = "ALTER TABLE gl_topic_assignments
   DROP PRIMARY KEY,
   ADD PRIMARY KEY( `tid`, `type`, `subtype`, `id`);
";

4 * (75(tid) + 30(type) + 30(subtype) + 128( id)) = 1052

mystralkk commented 4 years ago

@eSilverStrike, is it acceptable to change the length of subtype from 30 to 15?

mystralkk commented 4 years ago

I wondered why this error doesn't happen on fresh install and now understand the reason. The topic_assignments table is defined as:

CREATE TABLE `{$_TABLES['topic_assignments']}` (
  `tid` varchar(75) NOT NULL,
  `type` varchar(30) NOT NULL,
  `subtype` varchar(15) NOT NULL DEFAULT '',
  `id` varchar(128) NOT NULL,
  `inherit` tinyint(1) NOT NULL default '1',
  `tdefault` tinyint(1) NOT NULL default '0',
  PRIMARY KEY  (`tid`,`type`,`id`)
);

Look at PRIMARY KEY and you see it consists of tid,type,id, while on upgrade it is changed to consist of tid, type, subtype, id. Which is right?

eSilverStrike commented 4 years ago

Good catch.

Subtypes don't have to be 30 I just made them that way to match the plugin type length. Still with the orignal primary key we are almost maxed out. Technically the primary key should be ADD PRIMARY KEY( tid, type, subtype, id); as we don't want duplicates of the above. Since we cannot do this should we keep it ADD PRIMARY KEY( tid, type, id); and rely on the Topics Editor to ensure that (tid, type, subtype, id) doesn't exist more than once (and keep subtype at 30)?

I don't like it but it's not like we don't do this else where since our table structures don't use foreign keys.

The new likes table also uses a similar structure but does not have a proper primary key since that would include almost all of the columns in the table. If we change the subtype length it would need to be changed on the likes table as well.

eSilverStrike commented 4 years ago

Hmm, looking at my development server the topic_assignments table which is InnoDB has the primary key of PRIMARY KEY( tid, type, subtype, id); with the subtype column of 30. I guess this depends on the version of MySQL and is the reason why I didn't notice the problem. I also didn't have an issue with it when upgrading Geeklog.net

Maybe this is the reason why:

https://dev.mysql.com/doc/refman/5.6/en/innodb-limits.html

"When the innodb_large_prefix configuration option is enabled, the index key prefix length limit is raised to 3072 bytes for InnoDB tables that use the DYNAMIC or COMPRESSED row format. "

eSilverStrike commented 4 years ago

I updated the primary key for the topic assignments table to be just ( tid, type, id) since it is not possible with our minimum requirements MySQL requirements to have (tid, type, subtype, id).

Does that sound good?

mystralkk commented 4 years ago

I don't think it is a good idea to exclude subtype field from the primary key, since it could cause inconsistency. As I first wrote, why don't we change the length of subtype from 30 to 15?

eSilverStrike commented 4 years ago

Oh yes that is fine, my mistake. When I did the calculation before with 15 I got over 1000 for some reason. I will update the code and reduce the size of the subtype and fix the primary key