I remember it being an intentional change, assigning to Monty in case he wants to reconsider.
Ugh, after examining this change in detail, it has a few more serious problems:
- It exposes an HA_* handler-internal error number (167) to the client. It should have implemented an existing or new ER_* error and sent that instead. (This would also have allowed the test to use an ER_* name instead of the constant 167.)
- It conflates "Duplicate key" and "Out of range" errors for ALL transactions in replication in order to paper over the upgrade issues with changing this error. This is definitely not expected!
- It breaks a test in insert.result by causing the row_count() function to return -1 (due to the hidden SHOW WARNINGS done by mysqltest) instead of 0. The test was just updated with the new invalid result.
For many it has been a problem that IGNORE didn't give warnings for everything it ignored. I added the warnings for duplicate key errors to correct this.
I assume you agree that the more you know the server did, the better?
I did however forget to update the ignore section and the compatibility section of the KB. This are now fixed.
As far we have seen this has not caused many conflicts. For one year, we have only got one bug report (this one).
The reason for merging Duplicate key error and Out of range errors was the best thing I could come up with to not get replication to break when using an old master and new slave or vice versa. As this is only for replication I don't think this is a
serious issue.
From a client point if view, there is not a big difference between giving a handler error than an SQL error (> 1000). There is already many cases where we use handler errors directly. Only common errors that we want to translate are giving SQL errors. When it comes to test cases, we should sometime expand to have also handler errors as key values for the test cases.
That said, I could add a new compatibility 'mysql55' mode where we ignore giving the warnings for duplicate key. (We already have MySQL323 and MYSQL40 so this would be in line with this.
To fix the 167 issue, we could add the handler errors to ./include/mysqld_ername.h.
Jeremy, do you have a strong issue with that the error number is < 1000 ?
(I would prefer to not change an error number that has been in use for > 1 year).
The change of row_count() to -1 was actually correct.
If you call row_count() after a query that was niot insert it will return -1. This has not changed.
The reason for the result change was that the test case issued a 'show warnings' just after the insert, because of the new warning. This cases row_count() to return -1 instead of 0. Sorry for not adding a per-file comment for this.
I have now pushed code into 5.5 that allow one to also use symbols for handler errors.
As a test of this, I removed the error number constant 167 from the test cases.
Documentation updated about difference in behavior.
To make things compatible with old versions, I suggest we add a new flag: 'old_mode'
This would work as 'sql_mode', but should only be used when one wants to emulate some behavior from old MySQL or MariaDB versions.
To handle this case, one could use:
set @@old_mode="NO_DUP_KEY_WARNINGS_WITH_IGNORE";
(or --old-mode from the command line)
We should over time make the old 'old' variable obsolete and instead start using this method, as this is much more flexible.
I am discussing this with Sergei today and if gives ok I will push this into 5.5.
Added OLD_MODE variable that should fix this issue.
One can now turn of the warnings by doing:
OLD_MODE= "NO_DUP_KEY_WARNINGS_WITH_IGNORE";
http://bazaar.launchpad.net/~maria-captains/maria/5.5/revision/4040