Skip to content
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

Improved performance with counter_cache for tags (tag.taggings_count) #126

Closed
wants to merge 1 commit into from
Closed

Conversation

h0jeZvgoxFepBQ2C
Copy link
Contributor

Improved performance with counter_cache for tags (tag.taggings_count)

@bitflorist
Copy link

will this be accepted? This would be great!

@dgilperez
Copy link

I implemented the same improvement in my local fork.

dgilperez@bb8c8d3

So goes my +1 to this feature !!!

@jaimeiniesta
Copy link

This would be great for performance reasons, here goes my +1

@artemk
Copy link
Collaborator

artemk commented Dec 9, 2011

I wonder if adding counter_cache won't break existing apps, which don't want to rerun migrations are add new one.

@jaimeiniesta
Copy link

Good point @artemk - the migration that adds the counter_cache column should set the current value for the existing tags, if not, it will be set to 0, which is the default.

@artemk
Copy link
Collaborator

artemk commented Dec 9, 2011

Do you know a way how to generate new migration on bundle update ?

@jaimeiniesta
Copy link

I think that running a migration on bundle update is not desirable, this should be executed manually by the user.

Instead, I would put this new columns on the acts_as_taggable_on:migration generator, so it's all set up on new projects; for existing ones, I would offer a separate rake task that updates the existing data, like "rake acts_as_taggable_on:update_counters", and tell users to do that on the README.

@dgilperez
Copy link

I agree with @jaimeiniesta on that.

@artemk
Copy link
Collaborator

artemk commented Dec 10, 2011

Do you read readme every time you update your bundle?

@jaimeiniesta
Copy link

I do, and I never update the entire bundle at once, only every individual gem, checking what's new and what needs updating.

It's just not a good idea to modify the user's database automatically, that's dangerous. It should be done by the users.

@rromanchuk
Copy link
Contributor

+1, hi @jaimeiniesta :)

@rromanchuk
Copy link
Contributor

@lichtamberg it would awesome if you had time to update the README file too. There is no mention of caching, would be nice if people didn't have to dig around.

@diegorv
Copy link

diegorv commented Feb 2, 2012

+1 :D

@brookr
Copy link

brookr commented Jun 26, 2012

Uh oh, a 2 year old pull request... Does it still have any hope… or relevancy?

@artemk artemk closed this Jun 27, 2012
@dgilperez
Copy link

Well, I keep my branch updated against master, only to have this feature included.

https://github.com/dgilperez/acts-as-taggable-on

I still think this would be a great thing to have.

@iamjoshua
Copy link

+1 Seriously, how has this not been added yet...

@tilsammans
Copy link
Contributor

I love this pull request and would like it resubmitted (I am the new maintainer).

Because we're asking people to run an "update migration" I think this PR should target version 3.0.0. Because there are a lot of issues still waiting to be fixed, it will be a while before we can make that happen. I would say at least three months away.

Is everyone on board with it? If so, please exercise a little patience while I try to fix actual breakage first.

Ideally the new migration also includes #343 and we can work with an updated schema during the entire 3.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants