This repository was archived by the owner on Jan 27, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 19
WIP - delay_touching on relationship table may cause "update where NULL" query #20
Open
oehlschl
wants to merge
3
commits into
godaddy:master
Choose a base branch
from
goatapp:update_where_null
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good start. However, since we're skipping the actual update for non-persisted records, it seems we should also skip calling
run_callbackson those records on line 101, right? In the case where these records didn't persist, and thus the ID falls back to null, I think we're also in the same boat on identifying those records in the Set, right? To that end, there's little value in callingstate.updatedfor those records, as they cannot be identified from the Set for removal (same core problem as #14).Presuming the above is correct, does it make sense to pre-filter
recordsto only persisted records, and reference that updated collection in this method?The unfortunate thing here is that
delay_touchingclass now has awareness of this AR persistence problem, where we managed to isolate that concern to thestateclass in the infinite loop problem. I'm not sure there's a way to have State handle this scenario, though.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtuckergd I know you're not thrilled by the performance, but I think one way of doing the filtering upstream is the old implementation: https://github.com/godaddy/activerecord-delay_touching/pull/17/files#diff-c3d5a91eaad03c43d84f142a4473a06dR56
I think that would prevent unpersisted records from ever getting into this
touch_recordsmethod and keep the persistence logic well encapsulated within the State. FWIW, with that you might even be able to remove theif persisted?(previouslyunless destroyed?) check here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also thanks for your continued support on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is a performance-focused library, I think compromising clean encapsulation to favor performance is probably the right direction, if we can't find a way to preserve both.
The old implementation introduces a 3N iteration overhead (all guaranteed full iterations) of the collection of records, where the final merged implementation adds at most a 1N iteration (possibly less, since there's a shortcut on first persisted record). Even introducing the iteration in the
select!, added to the merged solution, leaves us at worst with 2N overhead.Also notable: the old implementation still introduced state awareness to
delay_touchingbecause it had to trigger thestateobject to remove unpersisted records, so I don't think it saves us much in the leaky abstraction problem.What do you guys think about the proposal above, adding the
records.select!(&:persisted?)call? Open to other suggestions, as well.@oehlschl ☝️