Refresh cagg uses min value for dimension when start_time is NULL #7546
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7546 +/- ##
==========================================
+ Coverage 80.06% 82.15% +2.09%
==========================================
Files 190 230 +40
Lines 37181 43360 +6179
Branches 9450 10912 +1462
==========================================
+ Hits 29770 35624 +5854
- Misses 2997 3412 +415
+ Partials 4414 4324 -90 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The description of the PR explains what is done, but it is more important why it is done. In particular:
The old behavior take the minimum value of the type when NULL is provided to create an open-ended range. What is the situation that break prompting you to do this approach instead?
src/hypertable.c
Outdated
| appendStringInfo(command, | ||
| "SELECT pg_catalog.min(%s) FROM %s.%s", | ||
| quote_identifier(NameStr(dim->fd.column_name)), | ||
| quote_identifier(NameStr(ht->fd.schema_name)), | ||
| quote_identifier(NameStr(ht->fd.table_name))); |
There was a problem hiding this comment.
This does a full table scan of the entire hypertable. With a hypertable that is big, this is going to be a significant problem, especially if tiering is involved.
There was a problem hiding this comment.
Changed this to get min value from chunk metadata.
|
@akuzm, @erimatnor: please review this pull request.
|
When the refresh_continuous_aggregate window's start is NULL, use the min value in the hypertable to determine the beginning of the range instead of the min value for the partition column. We do this only if enable_tiered_reads is set to false.
| * Query for the oldest chunk in the hypertable. | ||
| */ | ||
| command = makeStringInfo(); | ||
| appendStringInfo(command, query_str, ht->fd.id, dim->fd.id); |
There was a problem hiding this comment.
Is this correct arguments order? From looking at the query string it seems dim->fd.id should come first.
There was a problem hiding this comment.
nit: psprintf would do the same as StringInfo here, but would be shorter.
This solves the problem only for the special case described here. And yes, we need a more general solution. This PR is meant to address the more common case (where we have bad materialization entries) and a stop gap until we implement the incremental solution. |
When the refresh_continuous_aggregate window's start is NULL, use the min value in the hypertable to determine the beginning of the range instead of the min value for the partition column.
We do this only if enable_tiered_reads is set to false.