[6.x] Exclude whereNotNull check in relations query

[serpentblade]

Reference:
Original PR that applied this change: #8033
Inquiry from laravel/ideas regarding this fix: laravel/ideas#1341

Change Notes:
A whereNotNull check was added to the constraints of all HasOne, HasMany, and HasMorph relations queries to avoid a corner-case where new models with auto incremented IDs are created and have not had an ID assigned.

This forcibly includes an additional IS NOT NULL clause to all relations queries, where in 99% of cases it is not required.

This rectifies that change by only applying that clause in the unlikely case where the parentKey is still null, and reduces the size of relations queries for most usage.

To visualize the potential impact of this change, here are the queries as they are now compared to with the change applied

Before:

select * from group_members
where group_members.group_id = 28
  and group_members.group_id is not null

After:

select * from group_members
where group_members.group_id = 28

Query when parentKey is null (unchanged):

select * from group_members
where group_members.group_id is null
  and group_members.group_id is not null

I'm unaware of any real-world situation where the corner-case that encouraged this original bug fix would actually apply, so I struggled writing tests for it. I've updated the existing tests to prevent them from failing by adding an optional parameter to the shared getRelations* methods, and only applying the whereNotNull expectation when necessary. However I did not create any actual tests to simulate the actual null id condition. Any suggestions on how those tests should actually be structured would be appreciated.

Impact: Remove unnecessary where clause for most real-world relations queries usages. This results in reduced bandwidth to database and reduced effort by query parsers where the additional clause is (hopefully) optimized away.

I've applied this to 6.x as I consider the fix to be valuable to LTS devs (including myself on several projects) due to the positive impact to databases.

[taylorotwell]

I don't see a big point in changing this for negligible performance benefit. Are you having scaling issues from this?

[serpentblade]

No, I am not experiencing scaling issues directly from it. However there are always downstream effects where cleaning up code like this has benefits whether at scale or not:

• One less call to the query builder to add a not null clause and one less where clause that needs compiling, thus less cycles required to load relations
• Reduced data being sent to DB, which reduces network traffic (which does have a cost, especially at scale)
• Reduced size of query logs/noise in debug logs

When I look at the amazing effort put into getting Laravel as fast as possible, like those efforts put into Octane, this feels like the perfect small change to continue to move the needle with negligible (or zero?) downside.

[taylorotwell]

Well, I realize it may not seem like there is any downside, but anytime we change Eloquent at all there is always the risk (no matter how small it may seem now) that we are breaking existing applications. Potentially thousands of applications. 😅

Therefore, if something is not broken and it doesn't have clear, tangible benefits on improving the user's lives or measurable performance gains on their applications, it isn't worth doing.

[serpentblade]

I can respect it not being merged to 6.x for the sake of avoiding risk. Would you consider the change viable on master for future versions? As all major versions have an implied potential risk when upgrading is it more acceptable to include Eloquent updates then? I believe the change is still worth making to further improve the framework.

[Lundis]

+1 for changing this. It's really confusing seeing pointless "not null" statements when you're analyzing what SQL queries laravel creates to find performance bottlenecks. Everything else in Laravel tends to produce really smooth SQL, but this sure is an eyesore. I immediately assumed that we had done something fundamentally wrong in setting up our models when I saw the queries.

[alecpl]

+1, this would improve readability of the SQL queries, which is not nothing.