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

[5.2] Stabilized table aliases for self joins by adding count #13401

Merged
merged 3 commits into from
May 9, 2016
Merged

[5.2] Stabilized table aliases for self joins by adding count #13401

merged 3 commits into from
May 9, 2016

Conversation

JeroenVanOort
Copy link
Contributor

@JeroenVanOort JeroenVanOort commented May 2, 2016

In order to improve query cacheability.


Fixes #13353.

@GrahamCampbell
Copy link
Member

No need to hash that value?

@GrahamCampbell GrahamCampbell changed the title stabilized table aliases for self joins by adding count, in order to … [5.2[ Stabilized table aliases for self joins by adding count May 2, 2016
@GrahamCampbell GrahamCampbell changed the title [5.2[ Stabilized table aliases for self joins by adding count [5.2] Stabilized table aliases for self joins by adding count May 2, 2016
@JeroenVanOort
Copy link
Contributor Author

The only reason to hash is in case someone has tables like 'self_1', 'self_2', etc.

@GrahamCampbell
Copy link
Member

how is the hash anymore resistant?

@GrahamCampbell
Copy link
Member

maybe just call it laravel_reserved_1

@JeroenVanOort
Copy link
Contributor Author

I think it's less likely for someone to have a table like 'self_c4ca4238a0b923820dcc509a6f75849b' than 'self_1'. The same reason why it was used in the first place, I assume; I just left it in.

Sure, the prefix could be changed to 'laravel_reserved_' to avoid this all together.

@taylorotwell
Copy link
Member

Holding off on this.

@GrahamCampbell
Copy link
Member

@taylorotwell What about for 5.3?

@JeroenVanOort
Copy link
Contributor Author

@taylorotwell So what do I do now? I'd really like to have this merged in some shape or form. I guess you want to have a good look at this before merging and I get that, but closing the PR isn't very motivating for me.

@JeroenVanOort
Copy link
Contributor Author

@GrahamCampbell Could we please look at this once more?

@taylorotwell taylorotwell reopened this May 5, 2016
@taylorotwell
Copy link
Member

Will re-open for reconsideration.

@GrahamCampbell
Copy link
Member

@GrahamCampbell Could we please look at this once more?

Not upto me. Upto @taylorotwell. ;)

@JeroenVanOort
Copy link
Contributor Author

Benchmarks for this request are difficult in the sense that this change has it's largest effects on long, more complicated queries on data that doesn't change by the second (in which case caching isn't helpful anyway), which I can't easily replicate in code I can share.

The most simple scenario is making a list of children that have a parent: in the current situation the generated table alias is different every time the query is executed and thus can't be cached. In the situation requested the alias will be the same every time, so it can be cached by the database system itself. On these simple queries, I've seen the query time halved, but they didn't take very much time to run anyway.

What you'll have to take my word for, is that I observed the time taken to execute a longer query drop from about 350ms to 1.1ms by simply executing the same thing twice (on a MySQL server that is set up to support caching, Homestead doesn't by default). For one project I'm working on, the page load time (as reported by the debug bar) goes down from ~500ms to just ~160ms, after the query has been cache by the first page load.

As far as I know, this change doesn't break anything; it just enables the database engine do it's caching job.

@taylorotwell taylorotwell merged commit e86c4a4 into laravel:5.2 May 9, 2016
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.

None yet

3 participants