Add optional `force` argument to `refresh_continuous_aggregate` #7521

zilder · 2024-12-05T17:29:06Z

Following the SDC. Continuous aggregates that are built on top of tiered hypertables cannot access and materialize tiered data when the timescaledb.enable_tiered_reads GUC is disabled at the server level. And there is no way for a user to manually force the refresh with tiered reads enabled. Here we add an optional force parameter to the refresh_continuous_aggregate procedure that would allow user to partially re-materialize cagg within a time window that has already been materialized.

tsl/test/expected/cagg_refresh.out

erimatnor · 2024-12-11T10:35:20Z

I think we discussed this previously, and one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on". This would not require changes to our API and is the natural way to make the region refresh with current mechanisms. Was that approach discarded for some reason? If so, can someone explain what disqualified it?

zilder · 2024-12-11T13:33:03Z

@erimatnor

one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on"

I probably didn't catch that. Do you mean that in the GUC callback we set some sort of a flag that would indicate that we should insert an invalidation record whenever we refresh a range? I see a couple of issues:

Customer decides to change the GUC on server level. Now if they want to refresh missing aggregates over tiered data they would need to turn the GUC "off" and then "on" again.
This would alter the behavior of the refresh procedure for regular caggs (over non-tiered data).

In both cases it feels like not a clear UX.

fabriziomello · 2024-12-11T14:10:19Z

I think we discussed this previously, and one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on". This would not require changes to our API and is the natural way to make the region refresh with current mechanisms. Was that approach discarded for some reason? If so, can someone explain what disqualified it?

The problem with this approach is the case when the tiered data is already materialized that will generate unnecessary invalidation logs.

erimatnor · 2024-12-11T14:48:22Z

I think we discussed this previously, and one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on". This would not require changes to our API and is the natural way to make the region refresh with current mechanisms. Was that approach discarded for some reason? If so, can someone explain what disqualified it?

The problem with this approach is the case when the tiered data is already materialized that will generate unnecessary invalidation logs.

@erimatnor

one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on"

I probably didn't catch that. Do you mean that in the GUC callback we set some sort of a flag that would indicate that we should insert an invalidation record whenever we refresh a range? I see a couple of issues:

I meant adding the invalidations in the GUC, not setting a flag. Nothing else needs to change.

1. Customer decides to change the GUC on server level. Now if they want to refresh missing aggregates over tiered data they would need to turn the GUC "off" and then "on" again.

Not sure I follow why they'd have to turn off and on again. I believe guc callbacks are called on parsing the server config file as well.

2. This would alter the behavior of the refresh procedure for regular caggs (over non-tiered data).

I don't think this would be necessary if the invalidations are added via the guc callback.

In both cases it feels like not a clear UX.

The idea is that this would be completely transparent and no need for user to learn about a force parameter, so IMO the UX is better because it "just works" as expected.

With the other approach, the user first has to enable the GUC for tiered caggs to work, then use the force parameter for the first refresh. That seems like a strictly worse UX to me.

erimatnor · 2024-12-11T14:49:39Z

I think we discussed this previously, and one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on". This would not require changes to our API and is the natural way to make the region refresh with current mechanisms. Was that approach discarded for some reason? If so, can someone explain what disqualified it?

The problem with this approach is the case when the tiered data is already materialized that will generate unnecessary invalidation logs.

That's just one large invalidation entry per affected cagg. It should not be an issue right? Also, can't we detect whether it is up-to-date or not and avoid adding the invalidation? If we can't detect it, it would be "correct" to add the invalidation AFAICT.

fabriziomello · 2024-12-11T16:52:24Z

I think we discussed this previously, and one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on". This would not require changes to our API and is the natural way to make the region refresh with current mechanisms. Was that approach discarded for some reason? If so, can someone explain what disqualified it?

The problem with this approach is the case when the tiered data is already materialized that will generate unnecessary invalidation logs.

That's just one large invalidation entry per affected cagg. It should not be an issue right?

It is a problem (actually this is exact the problem I want to avoid) because the current architecture don't deal with large ranges very well and doing that will make things worse for the customer.

Also, can't we detect whether it is up-to-date or not and avoid adding the invalidation? If we can't detect it, it would be "correct" to add the invalidation AFAICT.

We can check if exists some tiered data already materialized but it don't guarantee that everything is actually materialized and this is the reason to have this force to make sure we'll rematerialize what is needed.

erimatnor · 2024-12-16T08:12:48Z

I think we discussed this previously, and one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on". This would not require changes to our API and is the natural way to make the region refresh with current mechanisms. Was that approach discarded for some reason? If so, can someone explain what disqualified it?

The problem with this approach is the case when the tiered data is already materialized that will generate unnecessary invalidation logs.

That's just one large invalidation entry per affected cagg. It should not be an issue right?

It is a problem (actually this is exact the problem I want to avoid) because the current architecture don't deal with large ranges very well and doing that will make things worse for the customer.

Ok, sounds like a bigger problem then. It is just that my alarm bells start to ring when I see that we are trying to build around the current architecture. I'd prefer we fix the underlying problem, but maybe this is not possible in the short term.

We can check if exists some tiered data already materialized but it don't guarantee that everything is actually materialized and this is the reason to have this force to make sure we'll rematerialize what is needed.

But it will only work for one cagg, so you'd have to manually force refresh all caggs that are connected to a tiered hypertable. If you forget one or more, you are now actually in a bad state where some caggs aren't accurately representing the data even if you refresh normally. In contrast, adding the invalidation will add the proper state to remember that this region is no longer valid and needs a refresh. This is what invalidations are for. But now we are saying we cannot use invalidations because they don't work well. So, now we use them sometimes, but in other times we don't. To me that sounds like a system where you cannot fully trust that a refresh is actually a refresh, which might lead to users always forcing a refresh just to be sure.

Anyway, I am just pointing these things out to make sure we thought this through, but if you are convinced that relying on the user forcing a refresh is the best option right now, then I trust you that there is no better option.

fabriziomello · 2024-12-16T14:04:42Z

I think we discussed this previously, and one idea was to invalidate the region covered by the tiered data when the GUC to enable tiered refreshing was changed to "true/on". This would not require changes to our API and is the natural way to make the region refresh with current mechanisms. Was that approach discarded for some reason? If so, can someone explain what disqualified it?

The problem with this approach is the case when the tiered data is already materialized that will generate unnecessary invalidation logs.

That's just one large invalidation entry per affected cagg. It should not be an issue right?

It is a problem (actually this is exact the problem I want to avoid) because the current architecture don't deal with large ranges very well and doing that will make things worse for the customer.

Ok, sounds like a bigger problem then. It is just that my alarm bells start to ring when I see that we are trying to build around the current architecture. I'd prefer we fix the underlying problem, but maybe this is not possible in the short term.

Unfortunately no... the short term solution we're working on is build some custom jobs to execute the refresh in small ranges instead of a big one and this new option will be used by OSM team when they have this situation when the tiered reads are disabled then they enable it and want to materialize the data.

But it will only work for one cagg, so you'd have to manually force refresh all caggs that are connected to a tiered hypertable. If you forget one or more, you are now actually in a bad state where some caggs aren't accurately representing the data even if you refresh normally. In contrast, adding the invalidation will add the proper state to remember that this region is no longer valid and needs a refresh. This is what invalidations are for. But now we are saying we cannot use invalidations because they don't work well. So, now we use them sometimes, but in other times we don't. To me that sounds like a system where you cannot fully trust that a refresh is actually a refresh, which might lead to users always forcing a refresh just to be sure.

Anyway, I am just pointing these things out to make sure we thought this through, but if you are convinced that relying on the user forcing a refresh is the best option right now, then I trust you that there is no better option.

Your points are valid and many thanks to jump into the discussion, but this option is not for wide audience but instead to ourselves use it in some custom jobs to execute the refresh in an incremental fashion. We don't want to rely on users to make sure that their data is in the correct state.

Anyway all of this is for short term solution but starting on Q1 2025 Mats and I will start to work on a new architecture for refreshing caggs to address all this write amplification we have nowadays, but it is not a small project.

github-actions · 2025-01-02T13:22:57Z

@akuzm, @fabriziomello: please review this pull request.

Powered by pull-review

fabriziomello · 2025-01-07T19:00:44Z

tsl/test/sql/include/cagg_refresh_common.sql

+DELETE FROM :mat_ht;
+
+-- Run regular refresh, it should not touch previously materialized range
+-- CALL refresh_continuous_aggregate('daily_temp', '2020-05-04 00:00 UTC', '2020-05-05 00:00 UTC');


mkindahl

A few nits and some minor comments.

Although I agree that the "correct" solution is to ensure that the invalidation tables are correct rather than force the refresh, I think this option is a useful workaround for those cases where things break.

Although the commit comment is correct and gives a rationale behind why it exists, this is more useful in a design document. I would argue that the commit comment should just contain what the change does and how it works, not why it is implemented.

I also miss a test using an empty table: if you force-refresh an empty range, you could potentially end up with a situation where you try to use a min and max that does not exist, so it's prudent to have a test for that.

sql/updates/latest-dev.sql

mkindahl · 2025-01-13T15:50:48Z

tsl/src/continuous_aggs/invalidation.c

@@ -140,7 +140,8 @@ static Invalidation cut_cagg_invalidation_and_compute_remainder(
 	const CaggInvalidationState *state, const InternalTimeRange *refresh_window,
 	const Invalidation *mergedentry, const Invalidation *current_remainder);
 static void clear_cagg_invalidations_for_refresh(const CaggInvalidationState *state,
-												 const InternalTimeRange *refresh_window);
+												 const InternalTimeRange *refresh_window,
+												 const bool force);


Nit: The previous ones are "pointers to constant data", but this one is a "constant boolean" which is not part of the function signature, so better skip it. There are some compilers that make a big deal out of it.

Suggested change

const bool force);

bool force);

agree, I removed consts

mkindahl · 2025-01-13T15:51:18Z

tsl/src/continuous_aggs/invalidation.c

@@ -878,7 +879,7 @@ cut_cagg_invalidation_and_compute_remainder(const CaggInvalidationState *state,
 */
 static void
 clear_cagg_invalidations_for_refresh(const CaggInvalidationState *state,
-									 const InternalTimeRange *refresh_window)
+									 const InternalTimeRange *refresh_window, const bool force)


Suggested change

const InternalTimeRange *refresh_window, const bool force)

const InternalTimeRange *refresh_window, bool force)

mkindahl · 2025-01-14T08:18:04Z

tsl/test/expected/cagg_refresh.out

+-- because it would skip previously materialized ranges, but it should be
+-- possible with `force=>true` parameter. To simulate this use-case we clear
+-- the materialization hypertable and forefully re-materialize it.
+SELECT ht.schema_name || '.' || ht.table_name AS mat_ht, mat_hypertable_id FROM _timescaledb_catalog.continuous_agg cagg


Nit: If the schema name or table name is "strange", this code might generate something that does not parse as a table. Compare these:

mats=# select format('%I.%I', nspname, relname), nspname || '.' || relname from pg_class cls join pg_namespace nsp on relnamespace = nsp.oid where nspname = 'public'; format | ?column? ---------------------------------------+--------------------------------------- public."fnupp'" | public.fnupp' public.foo | public.foo public.conditions | public.conditions public.conditions_time_idx | public.conditions_time_idx public.events_event_id_seq | public.events_event_id_seq public.events_updated_at_event_id_idx | public.events_updated_at_event_id_idx public.events | public.events public.unique_key3 | public.unique_key3 public.testtable3_time_idx3 | public.testtable3_time_idx3 public.testtable3 | public.testtable3 public.metric | public.metric (11 rows)

Suggested change

SELECT ht.schema_name || '.' || ht.table_name AS mat_ht, mat_hypertable_id FROM _timescaledb_catalog.continuous_agg cagg

SELECT format('%I.%I', ht.schema_name, ht.table_name) AS mat_ht, mat_hypertable_id FROM _timescaledb_catalog.continuous_agg cagg

I changed the test according to your suggestion, though in this particular case this should not be a problem because it is a standard auto-generated name like _timescaledb_internal._materialized_hypertable_xxx

Correct, it works now and is not dependent on user data, hence a nit.

codecov · 2025-01-14T11:33:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.25%. Comparing base (59f50f2) to head (9c9f04f).
Report is 693 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7521      +/-   ##
==========================================
+ Coverage   80.06%   82.25%   +2.18%     
==========================================
  Files         190      238      +48     
  Lines       37181    43798    +6617     
  Branches     9450    10997    +1547     
==========================================
+ Hits        29770    36024    +6254     
- Misses       2997     3428     +431     
+ Partials     4414     4346      -68

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Add optional `force` parameter to the `refresh_continuous_aggregate` procedure that would allow user to partially re-materialize cagg within a time window that has been previously materialized.

@bharrisau

This release contains performance improvements and bug fixes since the 2.17.2 release. We recommend that you upgrade at the next available opportunity. **Features** * timescale#6901: Add hypertable support for transition tables. * timescale#7104: Hypercore table access method. * timescale#7271: Push down `order by` in real-time continuous aggregate queries. * timescale#7295: Support `alter table set access method` on hypertable. * timescale#7341: Vectorized aggregation with grouping by one fixed-size by-value compressed column * timescale#7390: Disable custom `hashagg` planner code. * timescale#7411: Change parameter name to enable hypercore table access method. * timescale#7412: Add GUC for `hypercore_use_access_method` default. * timescale#7413: Add GUC for segmentwise recompression. * timescale#7433 Add support for merging chunks * timescale#7436 Add index creation on orderby columns * timescale#7443: Add hypercore function and view aliases. * timescale#7455: Support `drop not null` on compressed hypertables. * timescale#7458: Support vecorized aggregation with aggregate `filter` clauses that are also vectorizable. * timescale#7482: Optimize recompression of partially compressed chunks. * timescale#7486: Prevent building against postgres versions with broken ABI. * timescale#7521 Add optional `force` argument to `refresh_continuous_aggregate` * timescale#7528 Transform sorting on `time_bucket` to sorting on time for compressed chunks in some cases. * timescale#7565 Add hint when hypertable creation fails * timescale#7587 Add `include_tiered_data` parameter to `add_continuous_aggregate_policy` API **Bugfixes** * timescale#7378: Remove obsolete job referencing `policy_job_error_retention`. * timescale#7409: Update `bgw_job` table when altering procedure. * timescale#7410: Fix the `aggregated compressed column not found` error on aggregation query. * timescale#7426: Fix `datetime` parsing error in chunk constraint creation. * timescale#7432: Verify that the heap tuple is valid before using. * timescale#7434: Fixes the segfault when internally setting the replica identity for a given chunk. * timescale#7488: Emit error for transition table trigger on chunks. * timescale#7514: Fix the error: `invalid child of chunk append`. * timescale#7517 Fixes performance regression on `cagg_migrate` procedure * timescale#7527 Restart scheduler on error * timescale#7557: Fix null handling for in-memory tuple filtering. * timescale#7566 Improve transaction check in CAgg refresh * timescale#7584 Fix NaN-handling for vectorized aggregation **Thanks** * @bharrisau for reporting the segfault when creating chunks. * @k-rus for suggesting the improvement * @pgloader for reporting the issue in an internal background job. * @staticlibs for sending PR to improve transaction check in CAgg refresh * @uasiddiqi for reporting the `aggregated compressed column not found` error.

@bharrisau

This release contains performance improvements and bug fixes since the 2.17.2 release. We recommend that you upgrade at the next available opportunity. **Features** * timescale#6901: Add hypertable support for transition tables. * timescale#7104: Hypercore table access method. * timescale#7271: Push down `order by` in real-time continuous aggregate queries. * timescale#7295: Support `alter table set access method` on hypertable. * timescale#7341: Vectorized aggregation with grouping by one fixed-size by-value compressed column * timescale#7390: Disable custom `hashagg` planner code. * timescale#7411: Change parameter name to enable hypercore table access method. * timescale#7412: Add GUC for `hypercore_use_access_method` default. * timescale#7413: Add GUC for segmentwise recompression. * timescale#7433 Add support for merging chunks * timescale#7436 Add index creation on orderby columns * timescale#7443: Add hypercore function and view aliases. * timescale#7455: Support `drop not null` on compressed hypertables. * timescale#7458: Support vecorized aggregation with aggregate `filter` clauses that are also vectorizable. * timescale#7482: Optimize recompression of partially compressed chunks. * timescale#7486: Prevent building against postgres versions with broken ABI. * timescale#7521 Add optional `force` argument to `refresh_continuous_aggregate` * timescale#7528 Transform sorting on `time_bucket` to sorting on time for compressed chunks in some cases. * timescale#7565 Add hint when hypertable creation fails * timescale#7587 Add `include_tiered_data` parameter to `add_continuous_aggregate_policy` API **Bugfixes** * timescale#7378: Remove obsolete job referencing `policy_job_error_retention`. * timescale#7409: Update `bgw_job` table when altering procedure. * timescale#7410: Fix the `aggregated compressed column not found` error on aggregation query. * timescale#7426: Fix `datetime` parsing error in chunk constraint creation. * timescale#7432: Verify that the heap tuple is valid before using. * timescale#7434: Fixes the segfault when internally setting the replica identity for a given chunk. * timescale#7488: Emit error for transition table trigger on chunks. * timescale#7514: Fix the error: `invalid child of chunk append`. * timescale#7517 Fixes performance regression on `cagg_migrate` procedure * timescale#7527 Restart scheduler on error * timescale#7557: Fix null handling for in-memory tuple filtering. * timescale#7566 Improve transaction check in CAgg refresh * timescale#7584 Fix NaN-handling for vectorized aggregation **Thanks** * @bharrisau for reporting the segfault when creating chunks. * @k-rus for suggesting the improvement * @pgloader for reporting the issue in an internal background job. * @staticlibs for sending PR to improve transaction check in CAgg refresh * @uasiddiqi for reporting the `aggregated compressed column not found` error.

zilder requested a review from fabriziomello December 5, 2024 17:29

fabriziomello assigned zilder Dec 5, 2024

fabriziomello added the continuous_aggregate label Dec 5, 2024

zilder force-pushed the zilder/force_refresh branch from af8c263 to bde8617 Compare December 9, 2024 09:58

fabriziomello reviewed Dec 10, 2024

View reviewed changes

tsl/test/expected/cagg_refresh.out Outdated Show resolved Hide resolved

zilder force-pushed the zilder/force_refresh branch from bde8617 to 40f1986 Compare December 16, 2024 21:35

fabriziomello added this to the TimescaleDB 2.18.0 milestone Dec 18, 2024

zilder requested a review from fabriziomello January 2, 2025 13:20

zilder force-pushed the zilder/force_refresh branch from 40f1986 to d932511 Compare January 2, 2025 13:22

zilder marked this pull request as ready for review January 2, 2025 13:22

github-actions bot requested a review from akuzm January 2, 2025 13:22

zilder force-pushed the zilder/force_refresh branch from d932511 to 742ec32 Compare January 7, 2025 18:00

fabriziomello reviewed Jan 7, 2025

View reviewed changes

fabriziomello approved these changes Jan 7, 2025

View reviewed changes

zilder force-pushed the zilder/force_refresh branch 2 times, most recently from 7b1f47a to 70e7628 Compare January 8, 2025 09:25

zilder requested a review from gayyappan January 8, 2025 09:26

zilder force-pushed the zilder/force_refresh branch from 70e7628 to e977ffe Compare January 9, 2025 09:23

gayyappan approved these changes Jan 9, 2025

View reviewed changes

zilder force-pushed the zilder/force_refresh branch from e977ffe to 23f851d Compare January 9, 2025 17:44

fabriziomello requested a review from mkindahl January 13, 2025 14:06

mkindahl reviewed Jan 14, 2025

View reviewed changes

zilder force-pushed the zilder/force_refresh branch from 23f851d to b784c84 Compare January 14, 2025 11:22

zilder force-pushed the zilder/force_refresh branch from b784c84 to b9ab8e5 Compare January 14, 2025 13:42

Add optional force argument to refresh_continuous_aggregate

9c9f04f

Add optional `force` parameter to the `refresh_continuous_aggregate` procedure that would allow user to partially re-materialize cagg within a time window that has been previously materialized.

zilder force-pushed the zilder/force_refresh branch from b9ab8e5 to 9c9f04f Compare January 14, 2025 13:49

mkindahl approved these changes Jan 14, 2025

View reviewed changes

zilder merged commit 1718120 into timescale:main Jan 14, 2025
51 checks passed

pallavisontakke mentioned this pull request Jan 16, 2025

Release 2.18.0 #7596

Open

pallavisontakke mentioned this pull request Jan 17, 2025

Release 2.18.0 #7599

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional `force` argument to `refresh_continuous_aggregate` #7521

Add optional `force` argument to `refresh_continuous_aggregate` #7521

zilder commented Dec 5, 2024

erimatnor commented Dec 11, 2024

zilder commented Dec 11, 2024 •

edited

Loading

fabriziomello commented Dec 11, 2024

erimatnor commented Dec 11, 2024 •

edited

Loading

erimatnor commented Dec 11, 2024 •

edited

Loading

fabriziomello commented Dec 11, 2024

erimatnor commented Dec 16, 2024

fabriziomello commented Dec 16, 2024 •

edited

Loading

github-actions bot commented Jan 2, 2025

fabriziomello Jan 7, 2025

mkindahl left a comment

mkindahl Jan 13, 2025

zilder Jan 14, 2025

mkindahl Jan 13, 2025

mkindahl Jan 14, 2025

zilder Jan 14, 2025

mkindahl Jan 14, 2025

codecov bot commented Jan 14, 2025 •

edited

Loading

	const InternalTimeRange *refresh_window, const bool force)
	const InternalTimeRange *refresh_window, bool force)

	SELECT ht.schema_name \|\| '.' \|\| ht.table_name AS mat_ht, mat_hypertable_id FROM _timescaledb_catalog.continuous_agg cagg
	SELECT format('%I.%I', ht.schema_name, ht.table_name) AS mat_ht, mat_hypertable_id FROM _timescaledb_catalog.continuous_agg cagg

Add optional force argument to refresh_continuous_aggregate #7521

Add optional force argument to refresh_continuous_aggregate #7521

Conversation

zilder commented Dec 5, 2024

erimatnor commented Dec 11, 2024

zilder commented Dec 11, 2024 • edited Loading

fabriziomello commented Dec 11, 2024

erimatnor commented Dec 11, 2024 • edited Loading

erimatnor commented Dec 11, 2024 • edited Loading

fabriziomello commented Dec 11, 2024

erimatnor commented Dec 16, 2024

fabriziomello commented Dec 16, 2024 • edited Loading

github-actions bot commented Jan 2, 2025

fabriziomello Jan 7, 2025

Choose a reason for hiding this comment

mkindahl left a comment

Choose a reason for hiding this comment

mkindahl Jan 13, 2025

Choose a reason for hiding this comment

zilder Jan 14, 2025

Choose a reason for hiding this comment

mkindahl Jan 13, 2025

Choose a reason for hiding this comment

mkindahl Jan 14, 2025

Choose a reason for hiding this comment

zilder Jan 14, 2025

Choose a reason for hiding this comment

mkindahl Jan 14, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

Add optional `force` argument to `refresh_continuous_aggregate` #7521

Add optional `force` argument to `refresh_continuous_aggregate` #7521

zilder commented Dec 11, 2024 •

edited

Loading

erimatnor commented Dec 11, 2024 •

edited

Loading

erimatnor commented Dec 11, 2024 •

edited

Loading

fabriziomello commented Dec 16, 2024 •

edited

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading