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

Avoid unnecessary queries on next/previous events. #1196

Merged
merged 1 commit into from Jun 4, 2014

Conversation

umutgultepe
Copy link
Contributor

When running with a highly populated Postgresql database, the query for previous event takes unacceptably long time to fail if there are no previous instances (9-10 mins for our server).

With the .exists() condition, the time consuming query is not executed and we are able to view the new events without timing out.

@dcramer
Copy link
Member

dcramer commented Jun 3, 2014

Can you give me the indexes on sentry_message?

\d sentry_message

@umutgultepe
Copy link
Contributor Author

This are the indexes we have, all created by default syncdb and migrations:

sentry_message_checksum
sentry_message_checksum_like
sentry_message_datetime
sentry_message_group_id
sentry_message_level
sentry_message_logger
sentry_message_logger_like
sentry_message_message_id_like
sentry_message_project_id
sentry_message_server_name
sentry_message_server_name_like
sentry_message_site
sentry_message_site_like
sentry_message_view
sentry_message_view_like

@dcramer
Copy link
Member

dcramer commented Jun 3, 2014

It's really odd it wouldn't optimize on the group_id index here.

What we've done on getsentry.com is index (group_id, datetime) since it speeds this up in every situation. I feel like that might be a better solution. It can also replace the group_id solo index in this case. We really need to import all of our indexes into the migrations.

Thoughts?

@umutgultepe
Copy link
Contributor Author

I initially thought as well that a new index is the right answer. And I agree that a (group_id, datetime) index would speed things up here in general.

However in this particular case, I don't think that would help. I am saying this because I tried to optimize the query by changing its ordering. That did not help but I discovered the problem comes from order_by. When I execute :

    base_qs.filter(datetime__lte=event.datetime)

The response is quite fast and I get the empty query set as expected (same reason why .exists() works). However when I add the order_by clause:

   base_qs.filter(datetime__lte=event.datetime).order_by('-datetime')

That is when everything goes downhill, the query takes minutes to complete. I don't have in depth knowledge of converting django queries to SQL. Maybe psycopg always precedes the order_by, though unlikely.

I think the proper way to go would be test the behavior after adding the said index. I started a concurrent index building, will post updates after completion and testing.

When running with a highly populated Postgresql database, the query for previous event takes unacceptably long time to fail if there are no previous instances (9-10 mins for our server). The index not only solves this problem, it speeds up next/previous event queries in general.
@umutgultepe
Copy link
Contributor Author

The new index solved the problem. I updated the commit to include the migration.

@dcramer
Copy link
Member

dcramer commented Jun 4, 2014

Awesome, thanks!

dcramer added a commit that referenced this pull request Jun 4, 2014
Avoid unnecessary queries on next/previous events.
@dcramer dcramer merged commit 7832f21 into getsentry:master Jun 4, 2014
dcramer added a commit that referenced this pull request Jun 4, 2014
@umutgultepe umutgultepe deleted the patch-1 branch June 4, 2014 19:02
@github-actions github-actions bot locked and limited conversation to collaborators Dec 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants