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

PERF: reduce SQL queries by aggreating records #23

Merged
merged 1 commit into from Dec 22, 2016

Conversation

erickguan
Copy link
Contributor

The PR is a huge hack for performance but it works (somehow). Loading records for individual topics takes a huge amount of time in total. It's better to load them once.

I removed first_post_id to previewed_post_id. I don't understand the logic in the serializer but I think you are limiting the field when the post is loaded. It would be better to use another custom_fields to store the post_id so they can be loaded together.

There are many post action related methods in the serializer. It might be better to check the topic status in the client side to decide whether showing the button. Pushing permission check when the user click the button would save us from pushing can_like? to the client.

@angusmcleod
Copy link
Member

angusmcleod commented Dec 18, 2016

@fantasticfears Thanks for taking a shot at this. I was thinking along similar lines, but didn't know where to start.

However I'm getting this error when running this branch locally.

screen shot 2016-12-18 at 3 55 01 pm

It seems that the previewed_post attribute is not getting assigned for existing topics here. Did you have to drop your local DB for this to work? We need to figure out an alternate solution if that is the case as there are a number of existing users (assuming it's still not possible to migrate DBs from a plugin)

@erickguan
Copy link
Contributor Author

erickguan commented Dec 19, 2016

Fixed! I was only testing on a given backup. On that backup, it doesn't show a problem. But I don't know there can be a topic without a previewed_post. It's loaded from either the first post or post as the accepted answer.

@angusmcleod
Copy link
Member

@fantasticfears hm I'm still seeing the same issue in a different place now. Could you try checking out master, create some topics, then check out this branch and see if you get the same error? I'm not sure it's because there are topics on my local with no posts in them. I'm super busy at work right now, otherwise I'd dig into a bit more.

screenshot at dec 20 18-20-14

@erickguan
Copy link
Contributor Author

No problem. I'll try to dig different scenarios.

@erickguan
Copy link
Contributor Author

I've updated the pull request and it's good in my end. I've removed some SQLs which require postgresql 9.4+.

@angusmcleod
Copy link
Member

@fantasticfears Thanks! I've now got it working on my local. Perhaps it was the postgres version, albeit I seem to be running 9.6.1.

screen shot 2016-12-22 at 5 20 33 pm

Merging this now.

@angusmcleod angusmcleod merged commit 71fa292 into paviliondev:master Dec 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants