New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batch touch parent records #19324

Merged
merged 1 commit into from Apr 8, 2015

Conversation

Projects
None yet
8 participants
@arthurnn
Copy link
Member

arthurnn commented Mar 13, 2015

Batch the parent touches. So we doing something like post.comments.each(&:save!) it will only touch the Post parent once, and only before the transaction is sent to the DB.

[fixes #18606]

review @dhh @jeremy

This is heavily inspired from Jeremy's script https://gist.github.com/jeremy/84134ad08f2a137aa1ef . Now that we have the before_commit hooks and the touch(*, time:)

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Mar 13, 2015

this is still missing more tests and CHANGELOG, but I want to validate the idea first.

@_touch_time = current_time_from_proper_timezone

@_defer_touch_attrs.each { |attr| write_attribute attr, @_touch_time }
clear_attribute_changes @_defer_touch_attrs

This comment has been minimized.

@jeremy

jeremy Mar 13, 2015

Member

Would keep #surreptitiously_touch to encapsulate this bit of trickery. Easier to understand

This comment has been minimized.

@chancancode

chancancode Mar 13, 2015

Member

TIL a new word!

This comment has been minimized.

@arthurnn

arthurnn Mar 14, 2015

Member

me too =)

extend ActiveSupport::Concern

included do
before_commit_without_transaction_enrollment :touch_defered_attributes

This comment has been minimized.

@jeremy

jeremy Mar 13, 2015

Member

s/defered/deferred

line_item2.touch
end
end

This comment has been minimized.

@jeremy

jeremy Mar 13, 2015

Member

Need tests demonstrating that

  1. a non-persisted record isn't touched before commit
  2. the touched timestamp is the expected current_time_from_proper_timezone
  3. both named attributes and timestamp_attributes_for_update_in_model are touched
  4. calling #touch_later on a non-persisted record raises (arguably we could relax this, since the record may be persisted by the time the txn commits!)
  5. that #touch_later on an associated record doesn't cause it to get autosaved by the main record save
  6. that #touch_later doesn't mark the record as changed, or the timestamp attributes as changed
  7. that calling #touch_now(*names) touches both names as well as any outstanding deferred touches
  8. that calling #touch_later doesn't immediately hit the db, that it's triggered before commit
end

def touch(*) # :nodoc:
@_defer_touch_attrs = nil

This comment has been minimized.

@jeremy

jeremy Mar 13, 2015

Member

This will drop names from an earlier #touch_later invocation that aren't in the #touch args. Need to super(names | @_defer_touch_attrs)

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Mar 14, 2015

One issue about this implementation is that if we have a 3 level belongs_to, once we touch the grand-child , that will touch_later the child, which will not touch the parent, until the transaction is about to be committed. I am not sure how to fix that, @jeremy any ideas?

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Mar 28, 2015

@jeremy Thanks a lot for your review . ❤️ 💛 💚
I did the changed you suggested, also added tests for the touch_later module.
Let me know what else you think I am missing..

@arthurnn arthurnn closed this Mar 30, 2015

@arthurnn arthurnn reopened this Mar 30, 2015

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Mar 30, 2015

(travis was not running on this PR, so I closed and opened again)

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Apr 8, 2015

Is this good to go? Would be nice to have it committed then. (cc @jeremy)

Batch touch parent records
[fixes #18606]

Make belongs_to use touch over touch_later when running the callbacks.

Add more tests and small method rename

Thanks Jeremy for the feedback.

@arthurnn arthurnn force-pushed the arthurnn:batch_touch branch from 143b7b4 to 8971389 Apr 8, 2015

arthurnn pushed a commit that referenced this pull request Apr 8, 2015

Arthur Nogueira Neves
Merge pull request #19324 from arthurnn/batch_touch
Batch touch parent records

@arthurnn arthurnn merged commit 991875f into rails:master Apr 8, 2015

@arthurnn arthurnn deleted the arthurnn:batch_touch branch Apr 8, 2015

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Apr 8, 2015

🤘

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Jun 25, 2015

This breaks great-grandparent touching chains, like:

Comment.belongs_to :message, touch: true
Message.belongs_to :project, touch: true
Project.belongs_to :account, touch: true
Account

When we do Comment.create!, it'll only trigger a touch on its parent (message) and grandparent (project), not the great-grandparent (account). This is because we're not doing a full save on the Project, so the touch: true callback that Project has against Account is never triggered.

I've committed a skipped, but failing test for this in 5f5e6d9.

dhh added a commit that referenced this pull request Jun 25, 2015

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Sep 13, 2015

thanks @dhh , I will take a look at it.

@brodock

This comment has been minimized.

Copy link

brodock commented Oct 27, 2015

Is it rails 5 only?

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Oct 27, 2015

@brodock Rails 5

arthurnn added a commit that referenced this pull request Dec 6, 2015

Make sure we touch all the parents when touch_later.
The problem was that when saving an object, we would
call touch_later on the parent which wont be saved immediteally, and
it wont call any callbacks. That was working one level up because
we were calling touch, during the touch_later commit phase. However that still
didnt solve the problem when you have a 3+ levels of parents to be touched,
as calling touch would affect the parent, but it would be too late to run callbacks
on its grand-parent.

The solution for this, is instead, call touch_later upwards when the first
touch_later is called. So we make sure all the timestamps are updated without relying
on callbacks.

This also removed the hard dependency BelongsTo builder had with the TouchLater module.
So we can still have the old behaviour if TouchLater module is not included.

[fixes 5f5e6d9]
[related #19324]

arthurnn added a commit that referenced this pull request Mar 7, 2016

@daniel-rikowski

This comment has been minimized.

Copy link

daniel-rikowski commented Jul 4, 2016

Is there a way to disable this feature? I'm changing the schema search path during a transaction and when it finishes, the queued touch operations try to access tables, which are no longer available...

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Jul 13, 2016

@daniel-rikowski I think that sounds probably like a bug. Do you mind opening a new issue with a script that simulates this situation?
thanks

@daniel-rikowski

This comment has been minimized.

Copy link

daniel-rikowski commented Jul 13, 2016

I don't think it's a bug. A missing feature perhaps... Rails would have to monitor changes to the schema search path (is that even possible?) and when it changes the delayed touch must be performed beforehand. Much too specialized if you ask me.

Anyway, I created a runnable script which demonstrates the problem: https://gist.github.com/daniel-rikowski/fea2a87dbab975bd5d72556f0a426e34 (You'll need PostgreSQL and an existing database)

If you still think this is a bug I'll gladly open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment