maria-developers team mailing list archive

[Pavel Ivanov <pivanof@xxxxxxxxxx>]

On Sun, Nov 2, 2014 at 3:27 PM, Michael Widenius <monty@xxxxxxxxxxx> wrote:
>>> This is the same order that keys are presented in SHOW CREATE TABLE
>
> Pavel> Why is it safe to assume that keys have the same order on master and on slave?
>
> Primary and Unique keys should always be ordered the same way on
> master and slave. This is handled by the 'sort_keys()' function that
> you can find at: sql/sql_table.cc, line 2770.

This function seem to sort only by "type" of the key. If several
unique keys have the same "type" according to its classification then
they will be in the "original order" as the comment in the function
says. And the original order may be different.

> The only case when this is not the case, is if you add an unique key
> on the slave that was not on the master and there is conflicts when
> you insert things because of this key.
>
> However, if you do this, I would say that you are on your own.
>
>>> To help engine writes we are constantly adding new test in the storage
>>> engine test suite, to make it easier for storage engine vendors to
>>> test that their storage engine follows the interface.
>>> We have now added a test to check for the correct behaviour for the
>>> case of INSERT ... ON DUPLICATE KEY.
>
> Pavel> This is very interesting. Can you say which test it is and what
> Pavel> exactly does it test? Is it present in 10.0.14 or it's only at the
> Pavel> latest 10.0 branch?
>
> I added a test for this to:
> mysql-test/t/insert.test
>
> --echo #
> --echo # MDEV-5168: Ensure that we can disable duplicate key warnings
> --echo # from INSERT IGNORE
> --echo #

This test doesn't check which duplicate key is asserted to execute ON
DUPLICATE KEY action.

> Elena added another test for this in
> mysql-test/suite/storage_engine/insert_with_keys.test
>
> See line 142, that start with:
> --let $create_definition = a $int_indexed_col UNIQUE KEY, b $int_indexed_col UNIQUE KEY, c $int_col

This test seem to check that UPDATE is executed on the correct
duplicate key, but it's not executed during regular mtr run. How and
when it's executed in MariaDB?

>>> Do you have or use a storage engine that doesn't provide the expected value
>>> for the conflicting key ?
>
> Pavel> I don't have a specific example of storage engine that detects
> Pavel> duplicate key in different order, but I'm not convinced and don't have
> Pavel> hard proof that such engine cannot exist and that InnoDB itself is not
> Pavel> susceptible to this problem. I can easily imagine though that even
> Pavel> given the particular order of keys in table definition the key
> Pavel> reported as having duplicate can depend on the way storage engine
> Pavel> stores those keys.
>
> As key order is defined on the upper level, above InnoDB, this should
> be 100 % safe (at least for InnoDB).
>
> For other store engines, this is safe for any engine that test for
> duplicate key in key order.
>
> If we ever find a case where it's not safe with any storage engine
> that we deliver with MariaDB, we will fix the storage engine.
>
> It's ok if they do thing differently so that they can detect duplicate
> key for other keys faster.
>
> However, it has been required from all storage engines from start that
> if one calls 'handler->info()' to request which key was duplicated,
> they must return the first key.
>
> If not, they are breaking the storage engine interface (both for MySQL
> and MariaDB) as there is no way the upper level can guarantee that
> things will work identically between storage engines.
>
> Pavel> BTW, can you point to MySQL documentation and/or comment in the code
> Pavel> that indicates this requirement to storage engine implementation?
>
> Most of the storage engine interface is not 100 % documented. This is
> one of the case.
> However, I wrote the code related to errkey and this is how it was
> supposed to work.
> In general if something is not documented, then it's up to the people
> that created the interface or are actively working on it to define how
> it should work.
>
> What is important is that all storage engine should work the same
> way. If one returns something different compared to any of the
> existing engines then it's a bug.
>
> In this case the implicit documentation is how MyISAM, Aria and InnoDB
> works. All of them returns the first conflicting key.

I'm sorry to say, but you've explained exactly the reason why this
behavior is not guaranteed and can be changed at any time. Yes, I
understand that when you wrote the interface you had some particular
requirements in mind. But ultimately it's up to people actively
working on the code (who I believe are counted by hundreds now) to
decide whether to maintain the particular behavior or change it. And
as nobody knows the original requirements and the reasons why they
can't be changed, and when the behavior is not tested anywhere and
doesn't have any documentation even in the weakest form of comments or
asserts, the decision is usually very easy -- change at will, nobody
should ever depend on implementation details. Even if you promise to
fix storage engines that change this behavior, by the time you notice
the change you'll likely won't be able to fix it (as e.g. in case of
https://mariadb.atlassian.net/browse/MDEV-6916) or someone will
already experience a data corruption.

I'd strongly recommend to revert this change.


Thank you,
Pavel