Add MySQL 8.4 unit tests #16440

dbussink · 2024-07-22T19:25:46Z

Description

In order to support MySQL 8.4 in the future, we need to run CI unit tests against it.

Part of #16466

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

vitess-bot · 2024-07-22T19:26:43Z

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.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

MySQL 8.4 adds restrict_fk_on_non_standard_key for which the details can be found here: https://dev.mysql.com/doc/refman/8.4/en/server-system-variables.html#sysvar_restrict_fk_on_non_standard_key It prevents the use of non-unique keys or partial keys as foreign keys. For regular MySQL replication though, it does the following: Implication for MySQL Replication. When a foreign key is created on a nonstandard key on the primary because restrict_fk_on_non_standard_key is OFF, the statement succeeds on the replica regardless of any setting on the replica for this variable. Given how this works fir regular replication, we should mirror this behavior for vreplication and always allow this. So we set it up so we can do this with similar logic as to how we disable foreign key checks and we reset it back afterwards. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

In vitessio#7020 the logic was changed to always map YEAR to binary. But that is wrong, since this breaks the text protocol. Instead, we need to make sure that when we parse the binary data for prepared statements, we treat YEAR there as binary. That ensures we can parse it the way it's encoded in practice in the protocol (and not how it is documented). Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

codecov · 2024-07-23T10:21:48Z

Codecov Report

Attention: Patch coverage is 62.77372% with 51 lines in your changes missing coverage. Please review.

Project coverage is 68.65%. Comparing base (3186413) to head (e005120).
Report is 5 commits behind head on main.

Files	Patch %	Lines
...vttablet/tabletmanager/vreplication/vreplicator.go	47.36%	20 Missing ⚠️
go/mysql/flavor_filepos.go	50.00%	8 Missing ⚠️
go/vt/vtgate/evalengine/compiler_asm.go	80.00%	5 Missing ⚠️
go/vt/vtgate/evalengine/eval_numeric.go	0.00%	4 Missing ⚠️
go/vt/vtgate/evalengine/fn_string.go	87.50%	3 Missing ⚠️
go/mysql/capabilities/capability.go	50.00%	2 Missing ⚠️
go/mysql/flavor.go	50.00%	2 Missing ⚠️
go/mysql/flavor_mysqlgr.go	0.00%	2 Missing ⚠️
go/vt/binlog/binlogplayer/dbclient.go	0.00%	2 Missing ⚠️
go/vt/binlog/binlogplayer/fake_dbclient.go	0.00%	2 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16440      +/-   ##
==========================================
- Coverage   68.66%   68.65%   -0.02%     
==========================================
  Files        1548     1549       +1     
  Lines      199124   199323     +199     
==========================================
+ Hits       136731   136838     +107     
- Misses      62393    62485      +92

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

There's some small changes for newer MySQL versions around typing information for certain functions etc. This adopts those changes for newer versions. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

go/mysql/capabilities/capability.go

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

dbussink · 2024-07-23T13:32:08Z

Didn't mark this specific one as needing release notes etc, since we only want to add those once we've completed the 8.4 stuff.

frouioui

We should make sure the MySQL version in CI is the same as the one we ship in our docker images as the default MySQL version.
EDIT: please ignore 😄

dbussink · 2024-07-23T13:47:50Z

We should make sure the MySQL version in CI is the same as the one we ship in our docker images as the default MySQL version.

I don't understand this comment? This only adds a CI job for MySQL 8.4 and doesn't do anything for Docker etc. yet.

frouioui · 2024-07-23T15:59:36Z

.github/workflows/unit_test_mysql84.yml

+name: Unit Test (mysql84)
+on: [push, pull_request]
+concurrency:
+ group: format('{0}-{1}', ${{ github.ref }}, 'Unit Test (mysql84)')
+ cancel-in-progress: true


FYI, we need to mark this workflow as required once the PR is merged.

deepthi

Nice work!

deepthi · 2024-07-24T14:33:00Z

go/mysql/flavor_mysqlgr.go

@@ -42,8 +42,8 @@ type mysqlGRFlavor struct {
 }

 // newMysqlGRFlavor creates a new mysqlGR flavor.
-func newMysqlGRFlavor() flavor {
- return &mysqlGRFlavor{}
+func newMysqlGRFlavor(serverVersion string) flavor {


We should probably delete this file? WDYT?

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

github-actions bot added this to the v21.0.0 milestone Jul 22, 2024

Add MySQL 8.4 unit tests

2460c52

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

dbussink force-pushed the mysql84-unit-tests branch 3 times, most recently from ee7282d to 9279fb3 Compare July 23, 2024 07:29

dbussink force-pushed the mysql84-unit-tests branch from 9279fb3 to 0110468 Compare July 23, 2024 10:02

dbussink added 3 commits July 23, 2024 13:09

Fix evalengine behavior for newer MySQL versions

842405e

There's some small changes for newer MySQL versions around typing information for certain functions etc. This adopts those changes for newer versions. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Fix FilePos replication for 8.4

9f6d788

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Rerun codegen

00e4c3a

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

GrahamCampbell suggested changes Jul 23, 2024

View reviewed changes

go/mysql/capabilities/capability.go Outdated Show resolved Hide resolved

dbussink force-pushed the mysql84-unit-tests branch from dfa4e32 to dcfeef5 Compare July 23, 2024 12:26

Fix capability checks

0f4b635

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

dbussink force-pushed the mysql84-unit-tests branch from dcfeef5 to 0f4b635 Compare July 23, 2024 12:29

Remove MAKETIME case

e005120

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

dbussink marked this pull request as ready for review July 23, 2024 13:32

dbussink requested review from GuptaManan100, frouioui, rohit-nayak-ps, deepthi, mattlord, harshit-gangal, shlomi-noach, systay and vmg as code owners July 23, 2024 13:32

frouioui reviewed Jul 23, 2024

View reviewed changes

frouioui approved these changes Jul 23, 2024

View reviewed changes

vmg approved these changes Jul 24, 2024

View reviewed changes

dbussink merged commit be8f9f4 into vitessio:main Jul 24, 2024
134 of 135 checks passed

dbussink deleted the mysql84-unit-tests branch July 24, 2024 08:54

deepthi reviewed Jul 24, 2024

View reviewed changes

venkatraju pushed a commit to slackhq/vitess that referenced this pull request Aug 29, 2024

Add MySQL 8.4 unit tests (vitessio#16440)

abfb670

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MySQL 8.4 unit tests #16440

Add MySQL 8.4 unit tests #16440

dbussink commented Jul 22, 2024 •

edited

Loading

vitess-bot bot commented Jul 22, 2024

codecov bot commented Jul 23, 2024 •

edited

Loading

dbussink commented Jul 23, 2024

frouioui left a comment •

edited

Loading

dbussink commented Jul 23, 2024

frouioui Jul 23, 2024

deepthi left a comment

deepthi Jul 24, 2024

Add MySQL 8.4 unit tests #16440

Add MySQL 8.4 unit tests #16440

Conversation

dbussink commented Jul 22, 2024 • edited Loading

Description

Checklist

vitess-bot bot commented Jul 22, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jul 23, 2024 • edited Loading

Codecov Report

dbussink commented Jul 23, 2024

frouioui left a comment • edited Loading

Choose a reason for hiding this comment

dbussink commented Jul 23, 2024

frouioui Jul 23, 2024

Choose a reason for hiding this comment

deepthi left a comment

Choose a reason for hiding this comment

deepthi Jul 24, 2024

Choose a reason for hiding this comment

dbussink commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading

frouioui left a comment •

edited

Loading