Skip to content
This repository has been archived by the owner. It is now read-only.

Fix N+1 queries when updating non-translated model attributes #533

Merged
merged 2 commits into from Feb 3, 2019

Conversation

@derekmd
Copy link
Contributor

commented Nov 3, 2018

One of the joys outcomes of Laravel Telescope is discovering cron jobs that have been causing a nightmare-ish number of N+1 database queries.

I have a (partially) translated model with a boolean status column frequently updated. Yesterday I discovered this package lazy-loads the $this->translations relationships even when there aren't changes to non-translated attributes.

To give you an idea of scale, it was 500k+ redundant translation SELECT database queries run in a 15min period every day.

e.g.,

App\PressRelease::first()->save()

^ that needlessly lazy-loads relationship PressRelease@translations when neither translated attributes nor "regular" Eloquent model attributes are flagged as dirty.

This pull request adds a check on Illuminate\Database\Eloquent\Model@relationLoaded('translations')
when saving the model. An Eloquent relationship's models can't be marked "dirty" if they have yet to be loaded into PHP memory.

@Gummibeer

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2018

Thanks for your PR! Your changes look good. I will merge it later and before releasing I will adjust some code duplicates in the save() method.
So far I know there is also a way to check the amount of executed queries in phpunit. Will check this out - your current test works but doesn't test your issue/bug in a direct way. If I find a way I won't change your test but add a second one because your current one is also a good test.

@Gummibeer Gummibeer added the bug label Nov 3, 2018

@derekmd

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

  • Added $this->assertCount(..., DB::getQueryLog()) to inspect queries if that's what you're looking for.
  • I also added a second commit to remove the duplicate save() conditional branch.
@Gummibeer

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2018

Great work! Yes this is what I want. 🎉
I will have to review this later on my development PC - I think monday. But so far it looks good. 😊

derekmd added some commits Nov 3, 2018

Don't force translations lazy-load on save
Prevent N+1 queries when creating/updating
non-translated model attributes.

Don't lazy-load the 'translations'
relationship when saving the base
translated model.

@Gummibeer Gummibeer self-requested a review Feb 3, 2019

@Gummibeer Gummibeer self-assigned this Feb 3, 2019

@Gummibeer Gummibeer merged commit 36c729f into dimsav:master Feb 3, 2019

1 check passed

continuous-integration/styleci/pr The analysis has passed
Details
@Gummibeer

This comment has been minimized.

@derekmd derekmd deleted the derekmd:fix-untranslated-save-n-plus-1-queries branch Feb 3, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.