-
Notifications
You must be signed in to change notification settings - Fork 7
fix: redefine new CAs to group by test_id #390
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
Conversation
Codecov Report❌ Patch coverage is Please upload reports for the commit c5e613f to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
=======================================
Coverage 94.19% 94.19%
=======================================
Files 1261 1262 +1
Lines 46611 46646 +35
Branches 1491 1493 +2
=======================================
+ Hits 43903 43936 +33
- Misses 2404 2405 +1
- Partials 304 305 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #390 will not alter performanceComparing Summary
Footnotes |
1eee682
to
91bfd96
Compare
bc20e36
to
ed441ee
Compare
...ared/django_apps/ta_timeseries/migrations/0031_redefine_test_aggregate_dailies_by_test_id.py
Outdated
Show resolved
Hide resolved
ed441ee
to
c5e613f
Compare
* build: upgrade timescale version used in devenv we were previously using timescaledb-ha:pg14-latest which hasn't been updated in 2 years and is using a quite old version of timescale. So old in fact that it predates timescale 2.13 which changed the default for CAs to being non-realtime this broke a bunch of tests, which i fixed by manually calling the refresh CA function in each test after creating measurements I also took the liberty of switching from using django's timezone.datetime to datetime with tzinfo UTC because those were showing up as errors in my linter * fix: redefine new CAs to group by test_id this commit also adds some utility functions for use in migrations for making defining new CAs easier i'm choosing to make the refresh CAs "risky" argument a required kwarg so that it's obvious to the developer that they're either making the choice to have this run on deploy or that they're going to have to run it themselves
we were previously using timescaledb-ha:pg14-latest which hasn't been updated in 2 years and is using a quite old version of timescale. So old in fact that it predates timescale 2.13 which changed the default for CAs to being non-realtime this broke a bunch of tests, which i fixed by manually calling the refresh CA function in each test after creating measurements I also took the liberty of switching from using django's timezone.datetime to datetime with tzinfo UTC because those were showing up as errors in my linter
c5e613f
to
690b304
Compare
...ared/django_apps/ta_timeseries/migrations/0031_redefine_test_aggregate_dailies_by_test_id.py
Outdated
Show resolved
Hide resolved
2da6cbb
to
001f87e
Compare
this commit also adds some utility functions for use in migrations for making defining new CAs easier i'm choosing to make the refresh CAs "risky" argument a required kwarg so that it's obvious to the developer that they're either making the choice to have this run on deploy or that they're going to have to run it themselves
001f87e
to
42ceb38
Compare
forward_sql = f""" | ||
CREATE MATERIALIZED VIEW {view_name} | ||
WITH ( | ||
{",\n ".join(options)} | ||
) AS | ||
{select_sql_clean}{with_no_data_sql}; | ||
""".strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The create_continuous_aggregate
function lacks IF NOT EXISTS
, making migrations non-idempotent. Re-running a failed migration will cause a database error, halting the process and leaving the schema inconsistent.
-
Description: The
create_continuous_aggregate
function inmigration_utils.py
generates aCREATE MATERIALIZED VIEW
SQL statement without theIF NOT EXISTS
clause. While the migration drops views before recreating them, this is not robust. If the migration fails and is retried, the attempt to recreate an existing view will cause arelation "view_name" already exists
error from PostgreSQL, halting the process. Because the migration isatomic = False
, this failure can leave the database schema in a partially migrated, inconsistent state requiring manual intervention. This breaks the established pattern of idempotent migrations. -
Suggested fix: Modify the
create_continuous_aggregate
function to includeIF NOT EXISTS
in the generatedCREATE MATERIALIZED VIEW
statement. This will make the migration idempotent and robust against retries, aligning with repository standards and preventing failures in operational scenarios like rollbacks or retries.
severity: 0.72, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
there's no index currently covering the testsuite, classname and name fields, and we don't want to create an index on those fields because names can be long, so we should group by test_id, because an index already exists