Update YesSQL 3.4.0

[hishamco]

https://github.com/sebastienros/yessql/releases/tag/v3.4.0

Fix #14036

[hishamco]

Should fix #14338 right?

/cc @MikeAlhayek

[hishamco]

Seems the build complaining about using unicode, I will fix it ASAP

[sebastienros]

I'd rather see a PR that updates YesSql and fixes the index issues in migrations, targeting release/1.7.
Then the fixes will flow back to main. Otherwise we need to do the changes in both branches and fix merge conflicts.

[hishamco]

Now I'm fixing the failed unit tests, then I will retarget to release/1.7.1

[sebastienros]

@hishamco this should also fix the issue where we had problem with unicode chars in the query editor.

[sebastienros]

But we need to ensure it has fixed it, it would be bad to ship a patch release that has more bugs :/

[hishamco]

@hishamco this should also fix the issue where we had problem with unicode chars in the query editor.

I will check this, but I'm surprised why the tests failed and forced me to use nvarchar by default?!!

[MikeAlhayek]

@hishamco To fix issue #14338

Change

OrchardCore/src/OrchardCore.Modules/OrchardCore.Alias/Migrations.cs

Lines 35 to 42 in 9a76299

	SchemaBuilder.AlterIndexTable<AliasPartIndex>(table => table
	.CreateIndex("IDX_AliasPartIndex_DocumentId",
	"DocumentId",
	"Alias",
	"ContentItemId",
	"Published",
	"Latest")
	);

to this

// The index in MySQL can accommodate up to 768 characters or 3072 bytes.
// DocumentId (26) + Alias (714) + ContentItemId (26) + Published (1) + Latest (1) = 768
SchemaBuilder.AlterIndexTable<AliasPartIndex>(table => table
    .CreateIndex("IDX_AliasPartIndex_DocumentId",
        "DocumentId",
        "Alias(714)",
        "ContentItemId",
        "Published",
        "Latest")
);

This should also fix #14036

[sebastienros]

We need to check all indexes

[hishamco]

@MikeAlhayek I prefer to fix the indexes in another PR to avoid mixing things. @sebastienros related to the issue #13440 I think the issue needs to be fixed in the SQL Parser itself to support using N for nvarchar type

[sebastienros]

related to the issue #13440 I think the issue needs to be fixed in the SQL Parser itself to support using N for nvarchar type

You already mentioned that multiple times, and I answered it already, and will repeat it, I don't think so, and I think I fixed it in YesSql. To prove me wrong you should be able to repro the issue even with the new YesSql version.

[sebastienros]

@MikeAlhayek I prefer to fix the indexes in another PR to avoid mixing things.

Updating YesSql IS to fix the index issue. I don't think it add much value to do one without the other, but whatever works for everyone.

[jtkech]

@MikeAlhayek

The inner IDX_AliasPartIndex_DocumentId can be kept as is, as long as we limit the size in the related AliasPartIndex by using the MaxAliasLength that we reduced. As mentioned in #14338 (comment) but just as an example for what we already have done.

Yes we could think about no longer reduce MaxAliasLength and apply what you suggested but for now maybe better to only do that for inner indexes that still reference columns that are too long.

In #14338 (comment) I was referring about the OC.ContentFields.Indexing.SQL.Migrations where some max sizes are too high, but where we now could only apply the new ColumnName(maxSize) syntax instead.

Just checking the TextFieldIndex, yes it is already too long, we would need to decrease for example the MaxContentTypeSize, MaxContentPartSize and MaxContentFieldSize.

Anyway would only work for fresh installations, so for now the column length of the related index tables need to be changed manually.

So for now, for example this one should be updated (and some others in the same file)

OrchardCore/src/OrchardCore.Modules/OrchardCore.ContentFields/Indexing/SQL/Migrations.cs

Lines 45 to 53 in 93d1993

	SchemaBuilder.AlterIndexTable<TextFieldIndex>(table => table
	.CreateIndex("IDX_TextFieldIndex_DocumentId_ContentType",
	"DocumentId",
	"ContentType",
	"ContentPart",
	"ContentField",
	"Published",
	"Latest")
	);

Ultimately we may need to update any migration step that introduces an IDX_ inner index, this to give a better chance for people migrating from a previous version.

[MikeAlhayek]

@jtkech adding this

SchemaBuilder.AlterIndexTable<AliasPartIndex>(table => table
    .CreateIndex("IDX_AliasPartIndex_DocumentId",
        "DocumentId",
        "Alias(714)",
        "ContentItemId",
        "Published",
        "Latest")
);

should fix the issue for MySQL without having to reduce the size of the column length.

But are right, we should also reduce the size here

OrchardCore/src/OrchardCore.Modules/OrchardCore.ContentFields/Indexing/SQL/Migrations.cs

Lines 45 to 53 in 93d1993

	SchemaBuilder.AlterIndexTable<TextFieldIndex>(table => table
	.CreateIndex("IDX_TextFieldIndex_DocumentId_ContentType",
	"DocumentId",
	"ContentType",
	"ContentPart",
	"ContentField",
	"Published",
	"Latest")
	);

to something like

26 + 246 + 246 + 246 + 1 + 1 = 766

SchemaBuilder.AlterIndexTable<TextFieldIndex>(table => table
    .CreateIndex("IDX_TextFieldIndex_DocumentId_ContentType",
        "DocumentId",
        "ContentType(246)",
        "ContentPart(246)",
        "ContentField(246)",
        "Published",
        "Latest")
);

[jtkech]

should fix the issue for MySQL without having to reduce the size of the column length.

As I can see there is no current issue for the IDX_AliasPartIndex_DocumentId.

What I meant is that we already reduced the size of the Alias column (see below). In your 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) is sufficient, anyway we kept a margin of 4 bytes.

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; }

[sebastienros]

should fix the issue for MySQL without having to reduce the size of the column length.

we don't need to restrict the size of the Alias column anymore, at least not for MySQL. We just need to update the index definition.

So this doesn't make sense anymore:

// 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;

We should just have public const int MaxAliasLength = 123455; or whatever limit we want to apply here.

[jtkech]

Yes I understand, this is why I said

Yes we could think about no longer reduce MaxAliasLength and apply what you suggested ...

So if we go this way we may want to revert back the size we reduced for Alias and also for other indexes for the same reason. Then use the new Column(n) syntax with the right values and comments.

[MikeAlhayek]

@sebastienros I am merging this one and starting a new PR to fix the actual issues. We'll chary pick this one and the new one into 1.7.1. Can't modify this one since it is a fork not a branch we can maintain

[sebastienros]

That was supposed to target release/1.7

The flow is that we release these fixes for 1.7 and then we'll merge release/1.7 to main. We can't do it the other way around, which means now we have to cherry pick every change and do it again.

[sebastienros]

Sorry, my bad, this one doesn't need to be in release/1.7

[sebastienros]

Or it does, if we want the indexes to be fixed in 1.7, because it has the index length support :/