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

Adding a duplicate constraint with a new name overwrites the existing constraint rather than coexisting #8254

Closed
glennmatthews opened this issue Aug 12, 2024 · 8 comments
Labels
bug Something isn't working correctness We don't return the same result as MySQL customer issue

Comments

@glennmatthews
Copy link

Dolt version: 1.41.3

In Dolt, adding a duplicate constraint to a table with a new name overwrites the existing constraint on that table rather than coexisting with it. I discovered this because it appears to be a different behavior than exhibited by MySQL, and as such causes a Django migration (in our app, resulting from replacing an old-style Django unique_together constraint with a new-style UniqueConstraint), which works in MySQL, to error out in Dolt, because it tries to remove the original constraint after adding the duplicate constraint, but the original constraint no longer exists.

MySQL

mysql> CREATE TABLE `foobar` (`id` char(32) NOT NULL PRIMARY KEY, `col1` varchar(255) NOT NULL, `col2` varchar(255) NOT NULL);
Query OK, 0 rows affected (0.03 sec)

mysql> SELECT kc.`constraint_name`, kc.`column_name`, kc.`referenced_table_name`, kc.`referenced_column_name` FROM information_schema.key_column_usage AS kc WHERE kc.table_schema = DATABASE() AND kc.table_name = 'foobar' ORDER BY kc.`ordinal_position`;
+-----------------+-------------+-----------------------+------------------------+
| CONSTRAINT_NAME | COLUMN_NAME | REFERENCED_TABLE_NAME | REFERENCED_COLUMN_NAME |
+-----------------+-------------+-----------------------+------------------------+
| PRIMARY         | id          | NULL                  | NULL                   |
+-----------------+-------------+-----------------------+------------------------+
1 row in set (0.00 sec)

mysql> ALTER TABLE `foobar` ADD CONSTRAINT `constraint_1` UNIQUE (`col1`, `col2`);
Query OK, 0 rows affected (0.02 sec)
Records: 0  Duplicates: 0  Warnings: 0

mysql> SELECT kc.`constraint_name`, kc.`column_name`, kc.`referenced_table_name`, kc.`referenced_column_name` FROM information_schema.key_column_usage AS kc WHERE kc.table_schema = DATABASE() AND kc.table_name = 'foobar' ORDER BY kc.`ordinal_position`;
+-----------------+-------------+-----------------------+------------------------+
| CONSTRAINT_NAME | COLUMN_NAME | REFERENCED_TABLE_NAME | REFERENCED_COLUMN_NAME |
+-----------------+-------------+-----------------------+------------------------+
| constraint_1    | col1        | NULL                  | NULL                   |
| PRIMARY         | id          | NULL                  | NULL                   |
| constraint_1    | col2        | NULL                  | NULL                   |
+-----------------+-------------+-----------------------+------------------------+
3 rows in set (0.01 sec)

mysql> ALTER TABLE `foobar` ADD CONSTRAINT `constraint_2` UNIQUE (`col1`, `col2`);
Query OK, 0 rows affected, 1 warning (0.01 sec)
Records: 0  Duplicates: 0  Warnings: 1

mysql> SELECT kc.`constraint_name`, kc.`column_name`, kc.`referenced_table_name`, kc.`referenced_column_name` FROM information_schema.key_column_usage AS kc WHERE kc.table_schema = DATABASE() AND kc.table_name = 'foobar' ORDER BY kc.`ordinal_position`;
+-----------------+-------------+-----------------------+------------------------+
| CONSTRAINT_NAME | COLUMN_NAME | REFERENCED_TABLE_NAME | REFERENCED_COLUMN_NAME |
+-----------------+-------------+-----------------------+------------------------+
| constraint_1    | col1        | NULL                  | NULL                   |
| constraint_2    | col1        | NULL                  | NULL                   |
| PRIMARY         | id          | NULL                  | NULL                   |
| constraint_1    | col2        | NULL                  | NULL                   |
| constraint_2    | col2        | NULL                  | NULL                   |
+-----------------+-------------+-----------------------+------------------------+
5 rows in set (0.00 sec)

Dolt

MySQL > CREATE TABLE `foobar` (`id` char(32) NOT NULL PRIMARY KEY, `col1` varchar(255) NOT NULL, `col2` varchar(255) NOT NULL);
Query OK, 0 rows affected (0.030 sec)

MySQL > SELECT kc.`constraint_name`, kc.`column_name`, kc.`referenced_table_name`, kc.`referenced_column_name` FROM information_schema.key_column_usage AS kc WHERE kc.table_schema = DATABASE() AND kc.table_name = 'foobar' ORDER BY kc.`ordinal_position`;
+-----------------+-------------+-----------------------+------------------------+
| CONSTRAINT_NAME | COLUMN_NAME | REFERENCED_TABLE_NAME | REFERENCED_COLUMN_NAME |
+-----------------+-------------+-----------------------+------------------------+
| PRIMARY         | id          | NULL                  | NULL                   |
+-----------------+-------------+-----------------------+------------------------+
1 row in set (0.071 sec)

MySQL > ALTER TABLE `foobar` ADD CONSTRAINT `constraint_1` UNIQUE (`col1`, `col2`);
Query OK, 0 rows affected (0.016 sec)

MySQL > SELECT kc.`constraint_name`, kc.`column_name`, kc.`referenced_table_name`, kc.`referenced_column_name` FROM information_schema.key_column_usage AS kc WHERE kc.table_schema = DATABASE() AND kc.table_name = 'foobar' ORDER BY kc.`ordinal_position`;
+-----------------+-------------+-----------------------+------------------------+
| CONSTRAINT_NAME | COLUMN_NAME | REFERENCED_TABLE_NAME | REFERENCED_COLUMN_NAME |
+-----------------+-------------+-----------------------+------------------------+
| PRIMARY         | id          | NULL                  | NULL                   |
| constraint_1    | col1        | NULL                  | NULL                   |
| constraint_1    | col2        | NULL                  | NULL                   |
+-----------------+-------------+-----------------------+------------------------+
3 rows in set (0.079 sec)

MySQL > ALTER TABLE `foobar` ADD CONSTRAINT `constraint_2` UNIQUE (`col1`, `col2`);
Query OK, 0 rows affected (0.034 sec)

MySQL > SELECT kc.`constraint_name`, kc.`column_name`, kc.`referenced_table_name`, kc.`referenced_column_name` FROM information_schema.key_column_usage AS kc WHERE kc.table_schema = DATABASE() AND kc.table_name = 'foobar' ORDER BY kc.`ordinal_position`;
+-----------------+-------------+-----------------------+------------------------+
| CONSTRAINT_NAME | COLUMN_NAME | REFERENCED_TABLE_NAME | REFERENCED_COLUMN_NAME |
+-----------------+-------------+-----------------------+------------------------+
| PRIMARY         | id          | NULL                  | NULL                   |
| constraint_2    | col1        | NULL                  | NULL                   |
| constraint_2    | col2        | NULL                  | NULL                   |
+-----------------+-------------+-----------------------+------------------------+
3 rows in set (0.070 sec)
@fulghum
Copy link
Contributor

fulghum commented Aug 13, 2024

Thanks for reporting this one and for taking the time to provide such easy repro steps 🙏

We'll dig into this and see what's going on.

@fulghum fulghum added bug Something isn't working correctness We don't return the same result as MySQL labels Aug 13, 2024
@fulghum
Copy link
Contributor

fulghum commented Aug 13, 2024

I debugged through this one and I see what's happening now. The way Dolt works with secondary indexes, like these unique constraints create, is that the index is almost always looked up by the columns in the index, not by the index's name. In this case, because the columns are the same for the existing index and the new index, the attempt to create constraint_2 finds the existing unique index and updates it. The current API does have methods to look up an index directly by its name, but what's used more often are the methods to lookup an index by the columns it covers, such as in this create index case. One of the benefits of this is that indexes can be renamed on branches and then merged more easily, but it does create some limitations like this.

This seems fixable from what I can see so far, although it'll probably take some updates across the code base to untangle things. I'm happy to keep digging into this if it's causing issues for you and your team. At the same time, I also wanted to better understand the need for having two unique indexes over the same set of columns, since the index data would be redundant anyway. I'm wondering if there's something specific you and your team are trying to achieve and if there's a faster way we could help you get there with another approach.

Let me know what you think when you get a chance and I'll go ahead and keep poking on this a little more to better understand the scope.

@glennmatthews
Copy link
Author

Thanks for the update!

As best as I understand the issue from our side, we don't actually need the two unique indexes long-term, it's a temporary artifact of how Django generates the migration to replace one style of constraint (with a name autogenerated by Django) with a newer style (with an explicitly specified name). It appears to default to adding the new constraint before dropping the old one:

  1. migrations.AddConstraint(..., name='new_name')
  2. migrations.AlterUniqueTogether(..., unique_together=set())

or in other words:

  1. ALTER TABLE ... ADD CONSTRAINT 'new_name' UNIQUE ...
  2. ALTER TABLE ... DROP INDEX 'old_name'

Furthermore, as part of the AlterUniqueTogether step, before dropping the 'old_name' index, Django automatically runs a SELECT ... FROM information_schema.key_column_usage ... to look for it, and errors out there because the index by that name no longer exists at this time.

I'll do a little more digging - maybe we could work around this by manually editing the migration to reverse the order of the two steps above, but the above order works fine as-is with Postgres and MySQL.

Thanks again!

@fulghum
Copy link
Contributor

fulghum commented Aug 14, 2024

That's really helpful context, thank you for sharing! That's a totally valid use case and one we want to support for compatibility with MySQL and MySQL-based tools like Django. It's also helpful to know that you all need this for a migration – that's helpful because if we can't immediately get merges with duplicated indexes working totally seamlessly, it sounds like that won't affect you.

I did a quick audit of the uses of the index lookup APIs yesterday and have some ideas on how to change them to better support multiple indexes over the same set of columns. I'll keep digging in today and see what I can come up with.

@fulghum
Copy link
Contributor

fulghum commented Aug 15, 2024

I've got a change in my workspace that allows the duplicate index to be created and I've tested the delete case as well. I spent some time digging around to see if this would cause problems with other features (like merging), and confirmed that we already have logic in place to block trying to merge a duplicate index. I think this change should unblock the Django migration for you.

I need to clean up the tests and a little bit and move them into a better position in our test suite, then I'll get a PR opened and get this out for review.

@itdependsnetworks
Copy link

Safe to say this one can be closed?

@fulghum
Copy link
Contributor

fulghum commented Aug 16, 2024

We just released Dolt 1.42.12 with the fix for this issue. Let us know if you all hit any other problems.

@glennmatthews
Copy link
Author

Thank you for the speedy fix! Will verify it locally as soon as the release hits GitHub :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctness We don't return the same result as MySQL customer issue
Projects
None yet
Development

No branches or pull requests

4 participants