mysql icon indicating copy to clipboard operation
mysql copied to clipboard

Add error codes from MySQL 8

Open ARitz-Cracker opened this issue 5 years ago • 9 comments

Fixes #2325 Fixes #2402 Fixes #2410

This PR makes the generate-error-constants.js tool able to get all the error codes from the MySQL 8 sources, as the txt file which defines all of them was in a different location compared to MySQL 5.

I've also included the output from the tool.

However, there appears to be one issue. The MySQL 8 sources marks a lot of the MySQL 5 error codes as "obsolete". Since MySQL 5 is going to go EOL in Oct 2023, I'm assuming that these "obsolete" codes shouldn't be marked as such, at least until then. In order to solve this, we can do one of the two things:

  1. Manually edit errors.js to include the non-obsolete definitions for the MySQL 5 error codes.
  2. Allow for generate-error-constants.js to be passed 2 directories, one pointing the the MySQL 5 sources, and the other to the MySQL 8 sources. If it detects a MySQL 8 definition which starts with OBSOLETE_, it'll fall back to the MySQL 5 definition.

Please let me know what's preferred so I can make the adjustments.

ARitz-Cracker avatar Aug 10 '20 17:08 ARitz-Cracker

Thank so much @ARitz-Cracker ! Yea, we definately need some solution to the 5 vs 8 issue, as we cannot just drop this change in as-is with changing all those code to include the word obsolete -- as code that uses this library relies on the code names there for detection of what type of error occurred vs the number, so it would be at least a semver major change for this module (not even considering if it makes sense to lead them with obsolete).

Perhaps, if all the codes are included in 5 sources still, would just having the tool simply remove the OBSOLETE_ prefix be sufficient?

As for your linked issues, I see you say this fixes those two bug reports. Can you list here which code number fixes #2325 and which code number fixes #2350 ? We don't want to close those out without being sure this fixes those issues, and the way you have the description written GitHub will automatically close them once this is merged, so want to be sure it makes sense.

dougwilson avatar Aug 10 '20 17:08 dougwilson

Hello, @dougwilson! This PR does include ER_WRONG_SRID_FOR_COLUMN which #2325 refers to. I'll remove the reference to #2350 as I can't find the string "Connection was killed" on its own anywhere in the MySQL 8 sources.

As for making the tool simply removing the OBSOLETE_ prefix, I don't think that's a good idea. The most obvious reason being that some 5.x codes are defined as "OBSOLETE_ER_UNUSED" in 8.x. like here

Going down this path, I think making the tool read both the 5.x sources and the 8.x sources is the best solution.

Also, the CI is failing due to linting errors, I'll make sure to run eslint this time. 😆

ARitz-Cracker avatar Aug 10 '20 17:08 ARitz-Cracker

By the way, you may notice that this PR still replaces some of the generated output with "unused" or "obsolete", this must have been done sometime between 5.7.29 and 5.7.31. So there are still some definitions which both versions mark as obsolete. Should I make the script remove those last remaining obsolete and unused definitions from the list?

ARitz-Cracker avatar Aug 10 '20 18:08 ARitz-Cracker

@dougwilson one more thing! Since I'm going through the trouble of modifying the error constants generator anyway, I can further extend the tool to read the MariaDB sources as well, and thus solve #1982! the MariaDB 10 sources have them defined in a similar manner as MySQL 5 with some minor tweaks. So it definitely won't be difficult to do.

ARitz-Cracker avatar Aug 10 '20 20:08 ARitz-Cracker

That would be neat. I think I tried that and ran into issues. There may be a branch in this repo with that work already if you want to take a look.

But regardless, please make sure to do the MariaDB changes as a separate PR from this one.

I have been at work since I first commented and will take a look at the new changes of this PR after work.

dougwilson avatar Aug 10 '20 21:08 dougwilson

Sounds great! I'll create a new branch which extends onto this one. Also, just made one final change to make sure that this tool will always prioritize MySQL 5 definitions over MySQL 8 so that there are no breaking changes.

ARitz-Cracker avatar Aug 10 '20 21:08 ARitz-Cracker

I've made another branch here, which contains the MariaDB codes and the MySQL 8 codes. I'll submit a PR for that one once this one is approved.

ARitz-Cracker avatar Aug 10 '20 21:08 ARitz-Cracker

I've just re-written history. I removed the changes to errors.js during my 3 wip commits, and added a final commit where I ran the tool. Things look like they work as expected now!

ARitz-Cracker avatar Aug 11 '20 02:08 ARitz-Cracker

Hello! I don't know why the AppVeyor build failed. Should I rebase this PR?

ARitz-Cracker avatar Aug 27 '20 13:08 ARitz-Cracker