Set index length limit for MySQL

[MikeAlhayek]

Fix #14338

@sebastienros @jtkech I did some tests on MySQL and all seems to be working as expected. I also did the same test with Sqlite.

After creating a tenant on MySQL I run this command SHOW CREATE TABLE oc.textfieldindex

And it returned as you can see the 246 length on the key

CREATE TABLE `textfieldindex` (
   `Id` bigint(20) NOT NULL AUTO_INCREMENT,
   `DocumentId` bigint(20) DEFAULT NULL,
   `ContentItemId` varchar(26) DEFAULT NULL,
   `ContentItemVersionId` varchar(26) DEFAULT NULL,
   `ContentType` varchar(255) DEFAULT NULL,
   `ContentPart` varchar(255) DEFAULT NULL,
   `ContentField` varchar(255) DEFAULT NULL,
   `Published` bit(1) DEFAULT NULL,
   `Latest` bit(1) DEFAULT NULL,
   `Text` varchar(766) DEFAULT NULL,
   `BigText` longtext DEFAULT NULL,
   PRIMARY KEY (`Id`),
   KEY `IDX_FK_TextFieldIndex` (`DocumentId`),
   KEY `IDX_TextFieldIndex_DocumentId` (`DocumentId`,`ContentItemId`,`ContentItemVersionId`,`Published`,`Latest`),
   KEY `IDX_TextFieldIndex_DocumentId_ContentType` (`DocumentId`,`ContentType`(246),`ContentPart`(246),`ContentField`(246),`Published`,`Latest`),
   CONSTRAINT `FK_TextFieldIndex` FOREIGN KEY (`DocumentId`) REFERENCES `document` (`Id`) ON DELETE CASCADE ON UPDATE CASCADE
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4

[sebastienros]

Awesome. I also see that the Text field is limited to 766, was it done this way because of indexing issues too? If so we could update it, at least for new installs.

[sebastienros]

But can be done in a different PR for main.

[jtkech]

As said in #14491 (comment)

... DocumentId (26) is wrong as it is at most a bigint (size = 8 bytes = 2 chars size), the 2 booleans may not share the same bytes but one char size (4 bytes) for both is sufficient, anyway we kept a margin of 4 bytes.

Not so important to assume one "char" per boolean, but at least comment DocumentId (2) which here will allow Alias(738), and if we want to keep a margin of 4 bytes => Alias(734).

Then the following comments could be removed, and maybe revert back MaxAliasLength to its original value, from memory it was 1024, but maybe too much, I don't know, just for info.

OrchardCore/src/OrchardCore.Modules/OrchardCore.Alias/Models/AliasPart.cs

Lines 7 to 13 in f377a07

	// Maximum length that MySql can support in an index under utf8mb4 collation is 768,
	// minus 2 for the `DocumentId` integer (bigint size = 8 bytes = 2 character size),
	// minus 26 for the `ContentItemId` and 1 for the 'Published' and 'Latest' bools.
	// minus 4 to allow at least to add a new integer column.
	public const int MaxAliasLength = 735;
	public string Alias { get; set; }

Same remark for other indexes.

[MikeAlhayek]

@jtkech thanks for pointing out the seize of DocumentId I know it was long but assumed it was a char :). Anyway, I update it.

[MikeAlhayek]

Awesome. I also see that the Text field is limited to 766, was it done this way because of indexing issues too? If so we could update it, at least for new installs.

@sebastienros not sure why 766. But most likely related to the same limit. i don't know if we should increase it. I mean we could, but would would be a good length? 1024? The longer the column, may require more disk space from the server to maintained it. If you have a magic number, then I can update this and the Alias to use the same.

[MikeAlhayek]

@sebastienros I also did an audit on all the map index and updated others. I could not live with nameof() being used in the sql command, so I remove them since renaming them could cause problems.

[MikeAlhayek]

@sebastienros one thing to note if we decided to increase the size of alias and the text field columns, we have to account for existing installation when mapping data to column in the index providers. We can't try to insert more records that the columns allow for. We would have to add a new value in the ShellSettings to tell us what should we use for length.

For example, we can add a new property to the ShellSettings called MaxTextColumnLength. For new installation, we can use 1024. However, if that property does not exists, we have to take current length 735 for Alias and 766 for text field.

Maybe it is better to leave this out of this PR just so we don't complicate it since it's going to be part of 1.7.1

[jtkech]

@MikeAlhayek

Cool that you finally saw that DocumentId has not a 26 chars size but 2 (bigint = 8 bytes), which allows you to increase your limits. Then remember to remove the comments in AliasPart where we decreased MaxAliasLength, and maybe revert it to its original value.

To find out other places where we decreased other max sizes, search for collation is 768.

For example in TextFieldIndex.

---

That said what happens when an index is used but a referenced column has been truncated because the current size is greater than the limit? Particularly I'm thinking about the Alias field which is an identifier.

If this may be a problem, maybe better to also keep max sizes as is, and then use the same limits in SQL index definitions (and same comments).

[MikeAlhayek]

@jtkech yes check out my previous comment #14500 (comment)
I don't think it is a good idea to change the column sizes in this PR. We can do it in a separate PR for 1.8 since it require a bit more work so we can identify new/old installation.

[sebastienros]

That said what happens when an index is used but a referenced column has been truncated because the current size is greater than the limit? Particularly I'm thinking about the Alias field which is an identifier.

We need to differentiate "a field is truncated" from "a field's value is truncated in the index". In the index that is totally fine. It just means that there can be collisions when doing a lookup, and in that case a standard scan will be used with the colliding rows. Knowing that it might never happen since aliases are almost never close to the length of the limit, and even if that was the case the probability that the truncated values are the same is even lower. And even in this case it's not a problem, just a perf concern.

[jtkech]

Okay cool then, thanks for the confirmation.

I don't think it is a good idea to change the column sizes in this PR.

Okay, maybe at least keep their values / comments consistent, for example if a max size is lower than the new number now used to limit the inner index, at least would need to be equal.

[jtkech]

So if we keep max sizes as is, I would not have changed anything for the Alias Index.

Only changed the SQL Fields Indexes which is related to the new issue.

Which is what I suggested at the beginning ;)

[jtkech]

I don't think it is a good idea to change the column sizes in this PR. We can do it in a separate PR for 1.8 since it require a bit more work so we can identify new/old installation.

As said okay to not change the max sizes. But I don't see the problem, on an existing installation normally existing map index tables and inner sql indexes would not be altered.

[MikeAlhayek]

@jtkech yes I agree, we don't need to make any change to Alias "at least in this PR". I undid the changes that were made to Alias If you are good with this PR approve or merge it.

[jtkech]

So maybe we could keep the comments in Alias migrations that may become again useful ;)

But no problem, as you want.

Okay I could approve and merge but most of the changes are to replace all nameof() with strings, I know why we may do this, but are you sure that you didn't do any typo?

[sebastienros]

Was it supposed to target main ?

[MikeAlhayek]

Yes. I'll cherry pick the 2 PR into 1.7 tomorrow