Improve subresources DB query performance by removing a subquery

[clemherreman]

Q	A
Bug fix?	yes (kinda)
New feature?	no
BC breaks?	no
Deprecations?	no
Tickets	none
License	MIT
Doc PR	N/A

Abstract

This PR try to change DQL queries generated to fetch subresource, to improve performance on large tables.

Before

The generated SQL to query a subresource uses a WHERE ... IN ( <subquery> ), in order to be recursive when subresources also have subresources (introduced by #1608).

The subquery, on large table (at work 4M+ lines), takes a significant amount of time (2-5s), even with the proper SQL indexes (at least on MySQL).

SELECT
    DISTINCT c0_.idpurchase AS idpurchase_0,
    c0_.idpurchase AS idpurchase_1
FROM purchase c0_
LEFT JOIN customer c1_ ON c0_.idcustomer = c1_.idcustomer
-- Some other LEFT JOINs
WHERE c0_.idcustomer IN 
(  -- here please notice the usage of a subquery
    SELECT c16_.idcustomer
    FROM customer c16_
    WHERE c16_.idcustomer = :identifier
)

After

This PRs tries to optimize for the common case, having only one level of subresources, by replacing the subquery via a direct WHERE <identifier> = <identifier-alias> only on the last recursion level. This means it shouldn't break subresources that have subresources, while still improving the query for the nominal cases.

SELECT
    DISTINCT c0_.idpurchase AS idpurchase_0,
    c0_.idpurchase AS idpurchase_1
FROM purchase c0_
LEFT JOIN customer c1_ ON c0_.idcustomer = c1_.idcustomer
-- Some other LEFT JOINs
WHERE c0_.idcustomer = :identifier

This significantly reduces the SQL query time, going from 2.45s to 15ms on my e-commerce app.

BC & missing things.

I broke the unit tests, mainly the mocking, but also some tests fails as they compare the previously generated query, with the new one (which makes sense). All the Behat tests are green locally though.

I feel out of my depth here, and if someone would help me writing those, ensuring no BC is done, is would be great.

On my side, I run our rather large behat test suite against my fork, and not tests complained about wrong results, so I am fairly confident I haven't introduced some BC.

TODO

• Fix Behat tests
• Fix PHPUnit tests

[soyuka]

All the Behat tests are green locally though.

That's good, I've covered the edge cases of this in the behat test suite. The queries you show give the same results, therefore no BC break will outcome of this change. You can change the broken phpunit tests by fixing the relevant queries.
I like the change 👍

[teohhanhui]

Thanks for doing this! 🎉

[clemherreman]

Fixed the PHPUnit tests, and rebased against latest 2.5.

If the comment I added after @dkarlovi feedback is enough, I'd say this is ready to be merged.

Note: Behat tests are still failing, but because of either ccode coverage generation failing (?) or call to Dummy::hasRole() failing in the serialization. Both errors seems unrelated to that pull request, isn't it?

[teohhanhui]

Behat tests are still failing, but because of either ccode coverage generation failing (?) or call to Dummy::hasRole() failing in the serialization. Both errors seems unrelated to that pull request, isn't it?

Yeah, those are unrelated. Please ignore those failures. 😄

[teohhanhui]

Should we add some tests for this? (How?)

Or are the existing tests sufficient?

[clemherreman]

@teohhanhui Behat tests already covers the correctness of the query results (thanks to @alanpoulain)

Is there a point in checking the generated DQL itself? I doubt so. This is a perf improvement on the query level.

[teohhanhui]

Thanks @clemherreman! 🎉

[mxmp210]

Please have a look at #1542, As it breaks custom identifiers, feel free to review.