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

VReplication: Use MariaDB Compat JSON Functions #12420

Merged

Conversation

mattlord
Copy link
Contributor

@mattlord mattlord commented Feb 22, 2023

Description

This is needed to better support importing from MariaDB to Vitess/MySQL as we leverage MySQL's JSON support more and more in VReplication.

While we do not fully support MariaDB in Vitess v16 and later (where VDiff2 is GA and the default), we DO want to support easy and reliable imports from MariaDB to Vitess and MySQL. This is also a pretty easy and straightforward fix — as col1->>'$.field1' is equivalent to JSON_UNQUOTE(JSON_EXTRACT(col1, '$.field1')) — and doing so allows us to fully support VDiff2 with MariaDB imports, even when running the VDiffs against the reverse workflows after SwitchTraffic is done (which are then executed on the source MariaDB instance).

Note VDiffs are not typically run against the _reverse workflows. Without attempting those, you would get a couple of vttablet log messages per minute on the source primary tablet(s) that the attempt to find VDiffs to retry failed due to the unsupported syntax used. And even if you did attempt one, this would only prevent a VDiff that had encountered an initial error for any reason from being retried (it would be permanently failed). So this is a relatively minor fix, but one certainly worth doing.

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Documentation was added or is not required

This is needed to better support importing from
MariaDB into Vitess/MySQL.

Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord added Component: VReplication Forwardport to: main This will forward port the PR to the main branch Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Feb 22, 2023
@mattlord mattlord marked this pull request as ready for review February 22, 2023 05:29
@mattlord mattlord removed the request for review from rsajwani February 22, 2023 05:29
@GuptaManan100 GuptaManan100 mentioned this pull request Feb 22, 2023
51 tasks
@mattlord
Copy link
Contributor Author

The OnlineDDL VReplication test failures are a known unrelated issue that is being worked on.

@dbussink
Copy link
Contributor

The OnlineDDL VReplication test failures are a known unrelated issue that is being worked on.

@mattlord Is this something we could relatively easily test in the MariaDB migration job that we have? That it also tests a vdiff both ways?

Copy link
Member

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mattlord
Copy link
Contributor Author

mattlord commented Feb 22, 2023

The OnlineDDL VReplication test failures are a known unrelated issue that is being worked on.

@mattlord Is this something we could relatively easily test in the MariaDB migration job that we have? That it also tests a vdiff both ways?

I double checked... we do already do this today:

vdiff1(t, "product.p2c_reverse", "")

What wouldn't have worked on the reverse workflow is the auto retry on error (because the query used to find VDiffs that should be retried failed due to the invalid syntax). But you could still manually [re]start the workflow:

Or of course you could run another VDiff, which is what you always had to do with VDiff1 if you encountered an error.

So again, I think in practice this is a pretty minor issue but again, one that I think is still definitely worth doing.

If we do at some point decide that it's worth adding a VDiff2 auto retry test specifically with MariaDB, then we could effectively duplicate this test: https://github.com/vitessio/vitess/blob/main/go/test/endtoend/vreplication/vdiff2_test.go#L102

And use MariaDB for the keyspaces.

Copy link
Contributor

@rohit-nayak-ps rohit-nayak-ps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mattlord mattlord merged commit fa2c0a6 into vitessio:release-16.0 Feb 22, 2023
@mattlord mattlord deleted the vrepl_mariadb_compat_jsonq branch February 22, 2023 16:30
@hmaurer hmaurer mentioned this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VReplication Forwardport to: main This will forward port the PR to the main branch Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants