Skip to content
This repository has been archived by the owner on Jun 18, 2019. 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
Merged

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

merged 2 commits into from
Feb 3, 2019

Conversation

derekmd
Copy link
Contributor

@derekmd derekmd 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
Copy link
Collaborator

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
Copy link
Contributor Author

derekmd 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
Copy link
Collaborator

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. 😊

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 merged commit 36c729f into dimsav:master Feb 3, 2019
@Gummibeer
Copy link
Collaborator

https://github.com/dimsav/laravel-translatable/releases/tag/v9.3.0

@derekmd derekmd deleted the fix-untranslated-save-n-plus-1-queries branch February 3, 2019 18:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants