v18 backport: schemadiff: fix diffing of textual columns with implicit charsets

[shlomi-noach]

Description

A manual (and partial) backport of #14930. This had to be made manually because the changes in v19 rely on un-backportable changes we made to the collation environment.

This fixes an issue in v18, described in #15859 (comment)

Related Issue(s)

#15859 (comment)

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[shlomi-noach]

Seems like Cluster (schemadiff_vrepl) mysql57 CI test fails consistently. Looking.

[arthurschreiber]

Seems like Cluster (schemadiff_vrepl) mysql57 CI test fails consistently. Looking.

I saw this on my branch too. I think MySQL 5.7 and MySQL 8 had different behaviour around when and how charset and collation were part of the table definition.

[arthurschreiber]

If I remember correctly, this might be related to the default charset on MySQL 5.7 being different from 8.0.

So, on 5.7, the table will be created with utf8 as the default charset, and utf8 as the charset on the columns. Because the charset on the columns and the charset on the table matches, MySQL will skip including the charset in the SHOW CREATE TABLE output for the columns, and also skip listing it on the table itself (because it matches the system charset).

[shlomi-noach]

That makes sense. I haven't had the time to dig into this deeply yet (about to work on the point release now) but will take a look within an hour or so.

[arthurschreiber]

Seems like this is related to the alias?

Here's another failing test case:

		{
			name: "two identical tables, one with canonical charset name, one with alias",
			fromQueries: []string{
				"create table foobar (id int primary key, foo varchar(64) character set utf8 collate utf8_bin)",
			},
			toQueries: []string{
				"create table foobar (id int primary key, foo varchar(64) character set utf8mb3 collate utf8mb3_bin)",
			},
		},

[shlomi-noach]

A bit struggling with the test failure and how to address it.

[shlomi-noach]

Here's another failing test case:

Thanks for the pointer. FWIW, the best place for this kind of test is in TestCreateTableDiff, in table_test.go.

[shlomi-noach]

Here's another failing test case:

This is passing for me in this PR though.

[shlomi-noach]

OK, there's yet more code to backport, in particular getTableCharsetCollate()

[shlomi-noach]

I feel like this is the very problem we only fixed in v19 and could not backport to v18. The multitude of MySQL versions and their contextual environment.

[shlomi-noach]

If I add the fix, that break 8.0 behavior.

[arthurschreiber]

Maybe I can work around this on my side a bit by also setting the charset on the table itself so that hopefully both versions of MySQL will be happy?

[arthurschreiber]

Let's see if c875fac will help. 🙇‍♂️

[shlomi-noach]

Seems like we won't be needing this PR after all, since #15859 found a simpler solution. Closing for now.