fix remove unique constraint for collection_series. #1124

mukeshk · 2019-09-21T01:34:55Z

mystamps-bot · 2019-09-21T01:38:59Z

	1 Warning
⚠️	danger check: pull request description doesn’t contain a link to original issue. Consider adding a comment in the following format: `Addressed to #XXX` where `XXX` is an issue number

Generated by 🚫 Danger

mukeshk · 2019-09-21T04:47:36Z

On my local pc,

alter table mystamps.collections_series drop index uc_collections_series_collection_id_series_id

Error on rename of '.\mystamps#sql-c38_6' to '.\mystamps\collections_series' (errno: 150 - Foreign key constraint is incorrectly formed)

mukeshk · 2019-09-21T04:59:07Z

mukeshk · 2019-09-21T07:33:31Z

Alter table mystamps.collections_series
drop index uc_collections_series_collection_id_series_id,
drop foreign key fk_collections_series_collection_id,
drop foreign key fk_collections_series_series_id

MysqlError #1025
Error on rename of '.\mystamps\collections_series' to '.\mystamps#sql2-c38-6' (errno: 152 - Cannot delete a parent row)

php-coder · 2019-09-21T11:33:00Z

...sources/liquibase/version/0.4.1/2019-09-18--rollback_collection_unique_series_constraint.xml

+
+	<changeSet author="mukeshk" id="drop-unique-constraint-from-collections_series-table">
+		<comment>Remove unique constraint (collection_id,series_id) from collection_series</comment>
+		<dropForeignKeyConstraint baseTableName="collections_series"  constraintName="fk_collections_series_series_id" />


It's counter intuitive that we remove and re-create foreign keys. Is it required? Would dropUniqueConstraint work without it?

Would dropUniqueConstraint work without it?

I've just tested it locally (with H2) and it works. So, it's not clear so far why you've added it as it's unnecessary.

php-coder · 2019-09-21T11:34:17Z

...sources/liquibase/version/0.4.1/2019-09-18--rollback_collection_unique_series_constraint.xml

+	xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
+		http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.0.xsd">
+
+	<changeSet author="mukeshk" id="drop-unique-constraint-from-collections_series-table">


Typically "id" come first and also "context" attribute is missing here.

php-coder · 2019-09-21T11:37:59Z

Ok, I'll look on this failure later.

php-coder · 2019-09-21T15:03:22Z

...sources/liquibase/version/0.4.1/2019-09-18--rollback_collection_unique_series_constraint.xml

+	<changeSet author="mukeshk" id="drop-unique-constraint-from-collections_series-table">
+		<comment>Remove unique constraint (collection_id,series_id) from collection_series</comment>
+		<dropForeignKeyConstraint baseTableName="collections_series"  constraintName="fk_collections_series_series_id" />
+		<dropUniqueConstraint constraintName="uc_collections_series_collection_id_series_id"  tableName="collections_series"/>	


This line has trailing space BTW.

php-coder · 2019-09-21T15:03:34Z

...sources/liquibase/version/0.4.1/2019-09-18--rollback_collection_unique_series_constraint.xml

+		<comment>Remove unique constraint (collection_id,series_id) from collection_series</comment>
+		<dropForeignKeyConstraint baseTableName="collections_series"  constraintName="fk_collections_series_series_id" />
+		<dropUniqueConstraint constraintName="uc_collections_series_collection_id_series_id"  tableName="collections_series"/>	
+		<addForeignKeyConstraint baseTableName="collections_series"  baseColumnNames="series_id" 


This line has trailing space BTW.

php-coder · 2019-09-21T15:06:43Z

...sources/liquibase/version/0.4.1/2019-09-18--rollback_collection_unique_series_constraint.xml

+		http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.0.xsd">
+
+	<changeSet author="mukeshk" id="drop-unique-constraint-from-collections_series-table">
+		<comment>Remove unique constraint (collection_id,series_id) from collection_series</comment>


I'd rather remove this comment as it looks obvious to me.

mukeshk · 2019-09-21T15:13:15Z

Yes. The drop unique constraint worked on H2 db. But Travis failed for MySQL I will push again so that you will notice in Travis. Thanks

…

On Sat 21 Sep, 2019, 8:39 PM Slava Semushin, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/resources/liquibase/version/0.4.1/2019-09-18--rollback_collection_unique_series_constraint.xml <#1124 (comment)>: > @@ -0,0 +1,17 @@ +<?xml version="1.0" encoding="UTF-8"?> +<databaseChangeLog + xmlns="http://www.liquibase.org/xml/ns/dbchangelog" + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog + http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.0.xsd"> + + <changeSet author="mukeshk" id="drop-unique-constraint-from-collections_series-table"> + <comment>Remove unique constraint (collection_id,series_id) from collection_series</comment> + <dropForeignKeyConstraint baseTableName="collections_series" constraintName="fk_collections_series_series_id" /> Would dropUniqueConstraint work without it? I've just tested it locally (with H2) and it works. So, it's not clear so far why you've added it as it's unnecessary. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1124?email_source=notifications&email_token=AAJPE6EHOK2C7TCOZLDXDYLQKY2MNA5CNFSM4IY4PST2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFPXMGQ#discussion_r326863208>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJPE6C2LD3L5A2JR57GYLTQKY2MNANCNFSM4IY4PSTQ> .

php-coder · 2019-09-21T15:17:04Z

Yes. The drop unique constraint worked on H2 db. But Travis failed for MySQL

I'll look on this but anyway it should be noted in the comments. Mostly anything that we solve in a unexpected way because of the obstacles, should be expressed in the comments.

I will push again so that you will notice in Travis.

JFYI: I don't get notified on your builds. So the only way to see is when you push to this PR and I look on the result manualy.

php-coder · 2019-09-21T15:18:07Z

...sources/liquibase/version/0.4.1/2019-09-18--rollback_collection_unique_series_constraint.xml

+		http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.0.xsd">
+
+	<changeSet author="mukeshk" id="drop-unique-constraint-from-collections_series-table">
+		<dropUniqueConstraint constraintName="uc_collections_series_collection_id_series_id"  tableName="collections_series"/>	


There are 2 spaces before "tableName" attribute :)

php-coder · 2019-09-21T15:25:49Z

I'm a little confused why it uses DROP KEY instead of DROP INDEX:

Caused by: liquibase.exception.DatabaseException: Error on rename of './mystamps/#sql-e87_6' to './mystamps/collections_series' (errno: 150 - Foreign key constraint is incorrectly formed) [Failed SQL: ALTER TABLE mystamps.collections_series DROP KEY uc_collections_series_collection_id_series_id]

I wouldn't surprise if that's a liquibase bug, for instance.

php-coder · 2019-09-21T15:33:18Z

I'm a little confused why it uses DROP KEY instead of DROP INDEX:

Both are failing in this case :(

mysql> alter table collections_series2 drop key uc_collections_series_collection_id_series_id;
ERROR 1025 (HY000): Error on rename of './mystamps/#sql-488_b7' to './mystamps/collections_series2' (errno: 150 - Foreign key constraint is incorrectly formed)
mysql> alter table collections_series2 drop index uc_collections_series_collection_id_series_id;
ERROR 1025 (HY000): Error on rename of './mystamps/#sql-488_b7' to './mystamps/collections_series2' (errno: 150 - Foreign key constraint is incorrectly formed)

php-coder · 2019-09-21T17:33:26Z

I found an advise (see https://dba.stackexchange.com/questions/190153/mysql-create-table-error-1005-errno-150-foreign-key-constraint-is-incorrectly/201950#201950) to use SHOW ENGINE INNODB STATUS. Here is the related output:

------------------------
LATEST FOREIGN KEY ERROR
------------------------
2019-09-21 20:14:56 0x7fbeb021d700 Error in foreign key constraint of table mystamps/collections_series2:
there is no index in the table which would contain
the columns as the first columns, or the data types in the
table do not match the ones in the referenced table
or one of the ON ... SET NULL columns is declared NOT NULL. Constraint:
,
  CONSTRAINT `collections_series2_ibfk_1` FOREIGN KEY (`collection_id`) REFERENCES `collections` (`id`)

After closer look on the table definition I see the following:

mysql> show create table collections_series2;
...
  UNIQUE KEY `uc_collections_series_collection_id_series_id` (`collection_id`,`series_id`),
  KEY `fk_collections_series_series_id` (`series_id`),
  CONSTRAINT `collections_series2_ibfk_1` FOREIGN KEY (`collection_id`) REFERENCES `collections` (`id`),
  CONSTRAINT `collections_series2_ibfk_2` FOREIGN KEY (`series_id`) REFERENCES `series`
...

Note that we have KEY fk_collections_series_series_id (series_id) that creates an index for series_id field but there is no similar key for collection_id field. Why? I suppose this is because MySQL uses compound index instead (uc_collections_series_collection_id_series_id). And when we try to remove it, we get refused because removal leads to the state when this field won't have an index at all. And we have to have an index because this field has a foreign key. So, let's try to add this index manually:

mysql> alter table collections_series2 add key fk_collections_series_collection_id(collection_id);
Query OK, 0 rows affected (0.01 sec)
Records: 0  Duplicates: 0  Warnings: 0
mysql> alter table collections_series2 drop key uc_collections_series_collection_id_series_id;
Query OK, 0 rows affected (0.02 sec)
Records: 0  Duplicates: 0  Warnings: 0
mysql> show create table collections_series2;
...
  KEY `fk_collections_series_series_id` (`series_id`),
  KEY `fk_collections_series_collection_id` (`collection_id`),
  CONSTRAINT `collections_series2_ibfk_1` FOREIGN KEY (`collection_id`) REFERENCES `collections` (`id`),
  CONSTRAINT `collections_series2_ibfk_2` FOREIGN KEY (`series_id`) REFERENCES `series` (`id`)
...

So, it seems like we should add this key then before dropping an unique constraint.

I'll check how it works in H2 later to confirm whether it's right way to go.

php-coder · 2019-09-22T10:46:03Z

I'll check how it works in H2 later to confirm whether it's right way to go.

It looks like H2 has 2 indices already and it was MySQL trick to optimize number of indices. So, I suggest to: before dropping a unique index, first create an index for collection_id column. It will be a separate changeset I think. If this operation fails on H2, then we should execute it only on MySQL (with dbms="mysql" attribute).

php-coder

See my comments.

mukeshk · 2019-09-29T17:56:16Z

It seems I messed up with my branch. it created a detached head.

git status
HEAD detached from 08d659f6
nothing to commit, working tree clean

git rebase master origin/gh986_rollback_collection_series_unique_constraint -i
Successfully rebased and updated detached HEAD.

git status
On branch gh986_rollback_collection_series_unique_constraint
Your branch is up to date with 'origin/gh986_rollback_collection_series_unique_constraint'.

git rebase master origin/gh986_rollback_collection_series_unique_constraint -i
Successfully rebased and updated detached HEAD.

git checkout master
Switched to branch 'master'
Your branch is ahead of 'origin/master' by 4 commits.
(use "git push" to publish your local commits)

Any idea what i can do to fix this? sorry for the trouble.

php-coder · 2019-09-29T18:02:06Z

Your branch is ahead of 'origin/master' by 4 commits.

If you only need to reset master to be even with origin/master, use git reset --hard origin/master within master branch. This removes 4 commits. If you need them, copy them with git cherry-pick to a branch where you need them.

php-coder · 2019-09-30T12:18:56Z

...esources/liquibase/version/0.4.1/2019-09-29--create_index_series_id_on_collection_series.xml

+	xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
+		http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.0.xsd">
+
+	<changeSet author="mukeshk" id="create-index-seriesid-on-collections-series-table">


Let's put 2 change sets to a single file.

php-coder · 2019-10-03T11:54:38Z

No, there shouldn't be my commits, only yours :)

php-coder · 2019-10-04T13:46:56Z

BTW for a new PR -- don't forget to move a file with migration to 0.4.2 folder as 0.4.1 milestone is closed.

mukeshk requested a review from php-coder as a code owner September 21, 2019 01:34

mukeshk force-pushed the gh986_rollback_collection_series_unique_constraint branch 2 times, most recently from 45258c6 to 8de6358 Compare September 21, 2019 05:30

php-coder reviewed Sep 21, 2019

View reviewed changes

mukeshk force-pushed the gh986_rollback_collection_series_unique_constraint branch from 8de6358 to 43ae664 Compare September 21, 2019 15:15

php-coder reviewed Sep 21, 2019

View reviewed changes

php-coder requested changes Sep 25, 2019

View reviewed changes

php-coder reviewed Sep 30, 2019

View reviewed changes

mukeshk closed this Oct 3, 2019

mukeshk force-pushed the gh986_rollback_collection_series_unique_constraint branch from 0e1b9c2 to f3c2518 Compare October 3, 2019 11:58

php-coder added the resolution/invalid label Oct 6, 2019

mukeshk deleted the gh986_rollback_collection_series_unique_constraint branch November 10, 2019 07:39

fix remove unique constraint for collection_series. #1124

fix remove unique constraint for collection_series. #1124

Uh oh!

Conversation

mukeshk commented Sep 21, 2019 • edited by php-coder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mystamps-bot commented Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mukeshk commented Sep 21, 2019

Uh oh!

mukeshk commented Sep 21, 2019

Uh oh!

mukeshk commented Sep 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented Sep 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukeshk commented Sep 21, 2019 via email

Uh oh!

php-coder commented Sep 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented Sep 21, 2019

Uh oh!

php-coder commented Sep 21, 2019

Uh oh!

php-coder commented Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

php-coder commented Sep 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

php-coder left a comment

Choose a reason for hiding this comment

Uh oh!

mukeshk commented Sep 29, 2019

Uh oh!

php-coder commented Sep 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented Oct 3, 2019

Uh oh!

php-coder commented Oct 4, 2019

Uh oh!

Uh oh!

mukeshk commented Sep 21, 2019 •

edited by php-coder

Loading

mystamps-bot commented Sep 21, 2019 •

edited

Loading

php-coder commented Sep 21, 2019 •

edited

Loading

php-coder commented Sep 22, 2019 •

edited

Loading