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

Fixes "Failed to create name index on wp2static_core_options." errors #742

Merged
merged 2 commits into from
Jan 14, 2021

Conversation

TheLQ
Copy link
Contributor

@TheLQ TheLQ commented Jan 13, 2021

When using utf8mb4_unicode_ci collation, VARCHAR(254) is bigger than the MySQL Index Prefix Limit, so creating the name index fails with "SQL Error (1071): Specified key was too long; max key length is 767 bytes". VARCHAR(191) avoids that.

To be honest I'm not entirely sure why my limit is so low since I'm on InnoDB, But since the name column shouldn't contain keys that long anyway, this is a reasonable fix

Tested successfully with Wordpress 5.6.0, MariaDB 10.1.47

Additional report: https://staticword.press/t/failed-to-create-name-index-on-wpwh-wp2static-core-options/331

When using utf8mb4_unicode_ci collation, VARCHAR(254) is bigger than the MySQL Index Prefix Limit, so fails with "SQL Error (1071): Specified key was too long; max key length is 767 bytes". VARCHAR(191) avoids that.
@john-shaffer john-shaffer mentioned this pull request Jan 13, 2021
@john-shaffer
Copy link
Contributor

john-shaffer commented Jan 13, 2021

@TheLQ Thanks greatly for tracking this down.

On my servers (on https://app.staticweb.io if you want to investigate -- they all have phpmyadmin set up under /phpmyadmin/): wp_posts has an index on post_name with varchar(200) and utf8mb4_unicode_ci collation. wp_wp2static_core_options has the same collation for me. Why can't we also use varchar(200) as wp_posts does?

@john-shaffer
Copy link
Contributor

john-shaffer commented Jan 13, 2021

I see that the actual index on post_name is varchar(191), so are they only indexing the first 191 chars of a 200 char field? This would confirm that we can expect 191 to be widely supported on WP hosts. We should definitely use varchar(191) for both the column and index as in this patch and not do as WP does.

@TheLQ
Copy link
Contributor Author

TheLQ commented Jan 13, 2021

Agreed that's best for compatibility.

Interestingly, more testing shows that

CREATE DATABASE `test` /*!40100 COLLATE 'utf8mb4_unicode_ci' */;
USE test;
CREATE TABLE `test_table` (
  `id` MEDIUMINT(9) NOT NULL AUTO_INCREMENT,
  `name` VARCHAR(249) NOT NULL COLLATE 'utf8mb4_unicode_ci',
  PRIMARY KEY (`id`) USING BTREE,
  UNIQUE INDEX `name` (`name`) USING BTREE
)
COLLATE='utf8mb4_unicode_ci'
ENGINE=InnoDB
;
  • MariaDB 10.1.47 (default in Debian Stretch) - Fails with above error
  • MariaDB 10.3.27 (default in Debian Buster) - Works
  • MySQL 5.7.22 - Works (!!)
  • MySQL 8.0.22 - Works

So this is a legacy thing, but probably best practice to follow unless you really need that long of a key

@john-shaffer
Copy link
Contributor

john-shaffer commented Jan 14, 2021

I added a patch to fix the addons table not being created, which was the cause of a lot of error reports. I've tested the changes on eredas's site (https://staticword.press/t/qsandbox-and-wp2static-zip-and-s3-addon/351) and everything works great.

@leonstafford
Copy link
Contributor

Amazing work, @TheLQ & @john-shaffer!

Merged in to develop and will be in next release I publish

@leonstafford
Copy link
Contributor

Thoughts on how to prevent this from recurring

  • later in CI with specific MySQL/MariaDB versions
  • custom rule in code quality tools to check column lengths when used in indexes
  • handle abstracted in application code (if can't DoTheThing) fail properly

Last one sounds most practical, but the CI/build coverage would be useful, too.

Thoughts around getting ensureIndex() exercised during activation or before user gets too far along?

Last option: we just remember length to use for now and I'm overthinking things?

@leonstafford
Copy link
Contributor

placeholder builds of core and S3 addon
eredaswp2staticcore.zip
with this patched

eredass3addonforwp2static.zip

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

Successfully merging this pull request may close these issues.

3 participants