Skip to content

[5.6] Swap the index order of morph type and id#21693

Merged
taylorotwell merged 1 commit into
laravel:masterfrom
eddiclin:5.5
Oct 17, 2017
Merged

[5.6] Swap the index order of morph type and id#21693
taylorotwell merged 1 commit into
laravel:masterfrom
eddiclin:5.5

Conversation

@eddiclin

Copy link
Copy Markdown
Contributor

It can be utilize the index when query the type by using "WHERE type = ?",
also "WHERE type IN (?)"

It can be utilize the index when query the type by using "WHERE type = ?",
also "WHERE type IN (?)"
@eddiclin eddiclin changed the title swap the index order of morph type and id [5.5] Swap the index order of morph type and id Oct 16, 2017
@Dylan-DPC-zz

Copy link
Copy Markdown

This is a breaking change.

@eddiclin

Copy link
Copy Markdown
Contributor Author

@Dylan-DPC Please point it out, I don't know it will break what.

@Dylan-DPC-zz

Copy link
Copy Markdown

Your order of indexing is different so it is a tough one whether it is breaking or not

@eddiclin

Copy link
Copy Markdown
Contributor Author

It just speed up the sql which query by morph type. Sometimes we will query many rows with one type or some types, sql server can speed up it by this index.

@taylorotwell taylorotwell requested a review from themsaid October 16, 2017 16:48
@themsaid

Copy link
Copy Markdown
Member

Problem here is that the new index name will be difference since the order changed, so if I have a migration with a down() method removing the key by old name, this migration will now error on rollback since the name has changed, it's an edge case but trying to make sure no breaking changes.

Also won't this make it do a slower lookup by ID? Your changes will make the following queries faster:

  • by type
  • by type and ID

But will make this slower

  • by ID only

@antonkomarev

Copy link
Copy Markdown
Contributor

Search by ID only is pretty rare case for morph tables. Because it's useless to find all entities with ID of 3 without defining exact type.

@themsaid

Copy link
Copy Markdown
Member

@a-komarev makes sense, only the breaking part I mentioned is a concern then.

@antonkomarev

antonkomarev commented Oct 16, 2017

Copy link
Copy Markdown
Contributor

Temporary clone of dropIndex method with old behavior is not a great idea, but I don't see any other ways to deal with it. Or use try catch block and detect if there is no index of this name - try the old one.

@tillkruss tillkruss changed the base branch from 5.5 to master October 16, 2017 18:55
@tillkruss tillkruss changed the title [5.5] Swap the index order of morph type and id [5.6] Swap the index order of morph type and id Oct 16, 2017
@dwightwatson

Copy link
Copy Markdown
Contributor

Do you have to re-order the columns as well though? I know it's only a visual thing, but I've always thought having the ID before the type looked better. That wouldn't affect the performance of the index would it?

@eddiclin

eddiclin commented Oct 17, 2017

Copy link
Copy Markdown
Contributor Author

@dwightwatson Yes, re-order the columns wouldn't affect the performance of the index.
As @a-komarev said, "it's useless to find all entities with ID of 3 without defining exact type". I think that put the type before the morph ID looked better.

@taylorotwell

Copy link
Copy Markdown
Member

If it doesn't affect the performance than what are we doing?

@eddiclin

Copy link
Copy Markdown
Contributor Author

@taylorotwell What I'm talking about with @dwightwatson is the order of columns, not the order of indexes.

@taylorotwell taylorotwell reopened this Oct 17, 2017
@taylorotwell taylorotwell merged commit d15ad0f into laravel:master Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants