Cap invalidation threshold at last data bucket #2338
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2338 +/- ##
==========================================
+ Coverage 90.13% 90.39% +0.25%
==========================================
Files 212 213 +1
Lines 34391 34898 +507
==========================================
+ Hits 31000 31545 +545
+ Misses 3391 3353 -38
Continue to review full report at Codecov.
|
| 0 /*count*/); | ||
| if (res < 0) | ||
| ereport(ERROR, | ||
| (errcode(ERRCODE_INTERNAL_ERROR), |
There was a problem hiding this comment.
I wonder, if this case is possible and should we test for it
There was a problem hiding this comment.
I don't know a good way to make SPI fail. Maybe inject a bad query; but it seems odd to deliberately do that. I think we have to trust that SPI works the way it should.
|
@erimatnor Is it applicable only in the case of refresh policy or with manual refresh too? From the commit message it sounds that both ways are covered. |
Both are covered. The policy uses the same "manual" refresh API underneath the hood. |
src/hypertable.c
Outdated
| if (SPI_connect() != SPI_OK_CONNECT) | ||
| elog(ERROR, "could not connect to SPI"); | ||
|
|
||
| res = SPI_execute_with_args(command->data, |
There was a problem hiding this comment.
Why not just SPI_execute here?
There was a problem hiding this comment.
Yes, that's a good question. This was code that existed previously, which I moved here for code-reuse purposes. I will see if a simple SPI_execute works (it should).
| * | ||
| * The new invalidation threshold returned is the end of the given refresh | ||
| * window, unless it ends at "infinity" in which case the threshold is capped | ||
| * at the end of the last bucket materialized. |
There was a problem hiding this comment.
Looks like this will return infinity when that's the end of the refresh window and there's no materialized data yet. Is this correct? Should you mention this case in the comment here?
There was a problem hiding this comment.
Good catch. Not exactly intended, not wrong either, but certainly non-optimal. I fixed this so that it returns the min time value in that case and we avoid moving the threshold unnecessarily. I added extra test cases to cover this corner-case.
|
@erimatnor A suggestion to improve the commit message, the second paragraph:
|
erimatnor
force-pushed
the
caggs-cap-invalidation-threshold
branch
5 times, most recently
from
34cf22a
to
726cf06
Compare
I changed to commit message, hopefully to your liking. I didn't really understand your suggested change, however, in particular "Subsequently the refresh window is caped backward from +infinity to the end of the last bucket...". |
| bool | ||
| continuous_agg_invalidation_threshold_set(int32 raw_hypertable_id, int64 invalidation_threshold) | ||
| int64 | ||
| invalidation_threshold_set_or_get(int32 raw_hypertable_id, int64 invalidation_threshold) |
There was a problem hiding this comment.
set is confusing, since it updates value if applicable. I was already confused by the original function name when reviewed another PR. May be update?
set_or_get is confusing with or, since it always gets up-to-date threshold value. I think or_get can be omitted.
So the suggestion:
| invalidation_threshold_set_or_get(int32 raw_hypertable_id, int64 invalidation_threshold) | |
| invalidation_threshold_update(int32 raw_hypertable_id, int64 invalidation_threshold) |
There was a problem hiding this comment.
I wanted to point out that the existing threshold is returned, which is not clear if it is just called update. Update, like set, implies it is always updated, but it is only set if the new value is greater than the old one.
set_or_get is literally what it is doing; it either sets the threshold to the new value or returns the old one. If the returned value equal or higher than the value given is input, one knows that the threshold was not updated/set.
| if (TS_TIME_IS_INTEGER_TIME(refresh_window->type)) | ||
| max_refresh = TS_TIME_IS_MAX(refresh_window->end, refresh_window->type); | ||
| else | ||
| max_refresh = TS_TIME_IS_END(refresh_window->end, refresh_window->type) || | ||
| TS_TIME_IS_NOEND(refresh_window->end, refresh_window->type); |
There was a problem hiding this comment.
It might be useful to have this in a function, so it can be utilised in other places when needed.
There was a problem hiding this comment.
I think it makes sense to do that when, and if, the need arises.
The new commit message doesn't improve the most confusing sentence. It says:
The refreshed window is capped from
|
erimatnor
force-pushed
the
caggs-cap-invalidation-threshold
branch
from
726cf06
to
b5b30ae
Compare
When refreshing with an "infinite" refresh window going forward in time, the invalidation threshold is also moved forward to the end of the valid time range. This effectively renders the invalidation threshold useless, leading to unnecessary write amplification. To handle infinite refreshes better, this change caps the refresh window at the end of the last bucket of data in the underlying hypertable, as to not move the invalidation threshold further than necessary. For instance, if the max time value in the hypertable is 11, a refresh command such as: ``` CALL refresh_continuous_aggregate(NULL, NULL); ``` would be turned into ``` CALL refresh_continuous_aggregate(NULL, 20); ``` assuming that a bucket starts at 10 and ends at 20 (exclusive). Thus the invalidation threshold would at most move to 20, allowing the threshold to still do its work once time again moves forward and beyond it. Note that one must never process invalidations beyond the invalidation threshold without also moving it, as that would clear that area from invalidations and thus prohibit refreshing that region once the invalidation threshold is moved forward. Therefore, if we do not move the threshold further than a certain point, we cannot refresh beyond it either. An alternative, and perhaps safer, approach would be to always invalidate the region over which the invalidation threshold is moved (i.e., new_threshold - old_threshold). However, that is left for a future change. It would be possible to also cap non-infinite refreshes, e.g., refreshes that end at a higher time value than the max time value in the hypertable. However, when an explicit end is specified, it might be on purpose so optimizing this case is also left for the future. Closes timescale#2333
erimatnor
force-pushed
the
caggs-cap-invalidation-threshold
branch
from
b5b30ae
to
f0e3bfb
Compare
When refreshing with an "infinite" refresh window going forward in
time, the invalidation threshold is also moved forward to the end of
the valid time range. This effectively renders the invalidation
threshold useless, leading to unnecessary write amplification.
To handle infinite refreshes better, this change caps the refresh
window at the end of the last bucket of data in the underlying
hypertable, as to not move the invalidation threshold further than
necessary. For instance, if the max time value in the hypertable is
11, a refresh command such as:
would be turned into
assuming that a bucket starts at 10 and ends at 20 (exclusive). Thus
the invalidation threshold would at most move to 20, allowing the
threshold to still do its work once time again moves forward and
beyond it.
Note that one must never process invalidations beyond the invalidation
threshold without also moving it, as that would clear that area from
invalidations and thus prohibit refreshing that region once the
invalidation threshold is moved forward. Therefore, if we do not move
the threshold further than a certain point, we cannot refresh beyond
it either. An alternative, and perhaps safer, approach would be to
always invalidate the region over which the invalidation threshold is
moved (i.e., new_threshold - old_threshold). However, that is left for
a future change.
It would be possible to also cap non-infinite refreshes, e.g.,
refreshes that end at a higher time value than the max time value in
the hypertable. However, when an explicit end is specified, it might
be on purpose so optimizing this case is also left for the future.
Closes #2333