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

read-only for lc_messages breaks compatible with phpmyadmin #38231

Closed
Defined2014 opened this issue Sep 28, 2022 · 4 comments · Fixed by #38247
Closed

read-only for lc_messages breaks compatible with phpmyadmin #38231

Defined2014 opened this issue Sep 28, 2022 · 4 comments · Fixed by #38247
Assignees
Labels
severity/moderate sig/sql-infra SIG: SQL Infra type/bug This issue is a bug.

Comments

@Defined2014
Copy link
Contributor

Bug Report

see details from #33708 (comment)

@morgo
Copy link
Contributor

morgo commented Sep 28, 2022

There are a few ways you could fix this:

  1. Change it back to a noop
  2. Protect it by noop-functions
  3. Instead of read-only, write a validation func so that it only errors when trying to set it to something other than en_US.
  4. Same as (3) but instead of an error return a warning and refuse changes to other than en_US.

I don't love noops (they are confusing to debug), but I think (4) is a safe enough tradeoff for this variable.

@williamdes @kvtb what do you think?

@williamdes
Copy link

I would vote for Change it back to a noop so no apps should be broken by that behavior

@djshow832
Copy link
Contributor

I agree with Change it back to a noop. Protecting it by noop-functions also needs the user to set tidb_enable_noop_functions to true. Considering the user upgrades TiDB, it breaks compatibility.

@Defined2014
Copy link
Contributor Author

Agree with Change it back to a noop, so maybe we could revert #33708

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity/moderate sig/sql-infra SIG: SQL Infra type/bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants