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: limit positions to set elevation for to last 10 days #4228

Conversation

swiffer
Copy link
Contributor

@swiffer swiffer commented Sep 28, 2024

This limits the positions to set elevation for to the last 10 days greatly speeding up the query and reducing stress on the database.

fixes #4225

Copy link

netlify bot commented Sep 28, 2024

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit be38045
🔍 Latest deploy log https://app.netlify.com/sites/teslamate/deploys/67025315cba3f300086dc544
😎 Deploy Preview https://deploy-preview-4228--teslamate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@swiffer swiffer force-pushed the limit-positions-to-last-10-days branch 2 times, most recently from a7bc48d to 1c4860a Compare September 28, 2024 11:22
@swiffer
Copy link
Contributor Author

swiffer commented Sep 28, 2024

need support on this one - maybe someone with elixir knowledge can step in?

@JakobLichterfeld JakobLichterfeld added the enhancement New feature or request label Sep 28, 2024
@JakobLichterfeld JakobLichterfeld changed the title limit positions to set elevation for to last 10 days perf: limit positions to set elevation for to last 10 days Sep 29, 2024
@JakobLichterfeld
Copy link
Collaborator

@brianmay Can you take a look into this PR? Would love to include this in next release as it greatly improve background performance on small hosts with large logging base.

@swiffer swiffer force-pushed the limit-positions-to-last-10-days branch from c6d6bf2 to 7215b6a Compare September 29, 2024 18:34
@swiffer
Copy link
Contributor Author

swiffer commented Sep 29, 2024

@JakobLichterfeld did it ! ✌️also having a local dev env now... this is ready.

Before you are releasing a new version - do you want to add a hint to the release mentioning availability of postgres 17 and also recommend users checking their current indexes by linking to an issue / discussion?

1.) Upgrade TeslaMate
2.) Ensure everything still working
3.) Create Backup
4.) Run Index Fix Script
5.) Create Backup
6.) Upgrade PostgreSQL to 17 in Docker
7.) Restore from Backup via updated Restore Guide

  • we shouldn't wait with release that long as the docs have already been published.

Copy link
Collaborator

@brianmay brianmay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly, the one tradeoff with this is if I can't run teslamate for some reason (e.g. hardware failure) for 10+ days, the elevations of the last drives may not be set.

Probably a reasonable tradeoff.

lib/teslamate/log.ex Outdated Show resolved Hide resolved
lib/teslamate/log.ex Show resolved Hide resolved
@swiffer
Copy link
Contributor Author

swiffer commented Sep 30, 2024

If I understand this correctly, the one tradeoff with this is if I can't run teslamate for some reason (e.g. hardware failure) for 10+ days, the elevations of the last drives may not be set.

Probably a reasonable tradeoff.

true, but in this case you won't have any new data collected. so the potential case would be, hardware failure while driving and then maybe some elevation data updates missing. but in that case you're missing more than just that... think it's ok

@swiffer swiffer force-pushed the limit-positions-to-last-10-days branch from 7215b6a to 4d59544 Compare September 30, 2024 08:00
@JakobLichterfeld
Copy link
Collaborator

@JakobLichterfeld did it ! ✌️also having a local dev env now... this is ready.

Awesome! Love to see it!

Before you are releasing a new version - do you want to add a hint to the release mentioning availability of postgres 17 and also recommend users checking their current indexes by linking to an issue / discussion?

Yeah, debating the best way to do it, perhaps including the index checker in the docs as well, but every additional script is a potential source of unrelated support issues.

  • we shouldn't wait with release that long as the docs have already been published.

From what I get, the updated restore doc does not harm if used with postgres16 or before, innit?

@swiffer
Copy link
Contributor Author

swiffer commented Sep 30, 2024

@JakobLichterfeld did it ! ✌️also having a local dev env now... this is ready.

Awesome! Love to see it!

Before you are releasing a new version - do you want to add a hint to the release mentioning availability of postgres 17 and also recommend users checking their current indexes by linking to an issue / discussion?

Yeah, debating the best way to do it, perhaps including the index checker in the docs as well, but every additional script is a potential source of unrelated support issues.

Agree - would like to avoid having it in the docs but maybe give a hint to users that we had cases and asking them to check their instance once in the release notes pointing to the issue?

  • we shouldn't wait with release that long as the docs have already been published.

From what I get, the updated restore doc does not harm if used with postgres16 or before, innit?

Agree

@swiffer swiffer force-pushed the limit-positions-to-last-10-days branch from 4d59544 to be38045 Compare October 6, 2024 09:06
@swiffer
Copy link
Contributor Author

swiffer commented Oct 6, 2024

@JakobLichterfeld - rebased on latest master. should be ready for merge.

@JakobLichterfeld JakobLichterfeld merged commit b582b0b into teslamate-org:master Oct 6, 2024
12 checks passed
@swiffer swiffer deleted the limit-positions-to-last-10-days branch October 9, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reccuring Background SQL Statement times out leading to high load on postgres
3 participants