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

Fix next_start calculation for fixed schedules #5239

Merged

Conversation

konskov
Copy link
Contributor

@konskov konskov commented Jan 27, 2023

This patch fixes several issues with next_start calculation.

  • Previously, the offset was added twice in some cases.
    This is fixed by this patch.

  • Additionally, schedule intervals with month components
    were not handled correctly.
    Internally, time_bucket with origin is used to calculate
    the next start. However, in the case of month intervals, the
    timestamp calculated for a bucket is always aligned on the first
    day of the month, regardless of origin.
    Therefore, previously the result was aligned with origin by adding
    the difference between origin and its respective time bucket.
    This difference was computed as a fixed length interval in terms
    of days and time. That computation led to incorrect computation of
    next start occasionally, for example when a job should be executed on
    the last day of a month.
    That is fixed by adding an appropriate interval of months to
    initial_start and letting Postgres handle this computation properly.

Fixes #5216

@konskov konskov force-pushed the month_intervals_error_fixed_schedules branch 4 times, most recently from 9b2a5e5 to 6360bfa Compare January 27, 2023 15:30
@konskov konskov mentioned this pull request Jan 27, 2023
@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #5239 (7fa6217) into main (cad2440) will decrease coverage by 0.02%.
The diff coverage is 97.70%.

❗ Current head 7fa6217 differs from pull request most recent head 0a94670. Consider uploading reports for the commit 0a94670 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5239      +/-   ##
==========================================
- Coverage   89.35%   89.34%   -0.02%     
==========================================
  Files         225      225              
  Lines       51914    51953      +39     
==========================================
+ Hits        46389    46415      +26     
- Misses       5525     5538      +13     
Impacted Files Coverage Δ
tsl/src/fdw/estimate.c 79.53% <75.00%> (-0.23%) ⬇️
tsl/src/fdw/data_node_scan_plan.c 98.10% <98.07%> (+0.09%) ⬆️
src/bgw/job_stat.c 92.60% <100.00%> (+0.41%) ⬆️
src/guc.c 94.33% <100.00%> (+0.10%) ⬆️
src/loader/bgw_launcher.c 89.51% <0.00%> (-2.55%) ⬇️
src/bgw/scheduler.c 83.63% <0.00%> (-1.80%) ⬇️
tsl/src/bgw_policy/job.c 87.45% <0.00%> (-0.05%) ⬇️
tsl/src/fdw/deparse.c 64.43% <0.00%> (+0.22%) ⬆️
tsl/src/fdw/shippable.c 97.72% <0.00%> (+2.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 756ef68...0a94670. Read the comment docs.

@mkindahl mkindahl self-requested a review January 30, 2023 08:33
@konskov konskov force-pushed the month_intervals_error_fixed_schedules branch from 6360bfa to f50a087 Compare January 30, 2023 12:17
@konskov konskov marked this pull request as ready for review January 30, 2023 12:23
@github-actions github-actions bot requested review from gayyappan and mahipv January 30, 2023 12:40
@github-actions
Copy link

@gayyappan, @mahipv: please review this pull request.

Powered by pull-review

Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

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

You need a better commit message, in particular, the first line.

@konskov konskov force-pushed the month_intervals_error_fixed_schedules branch from f50a087 to 33d0389 Compare January 31, 2023 10:56
@konskov konskov changed the title Fix reported bug Fix next_start calculation for fixed schedules Jan 31, 2023
@konskov konskov force-pushed the month_intervals_error_fixed_schedules branch from 33d0389 to 2ee3944 Compare January 31, 2023 10:58
@konskov
Copy link
Contributor Author

konskov commented Jan 31, 2023

Fixed the commit message I hope @mkindahl ?

@konskov konskov added this to the TimescaleDB 2.9.3 milestone Jan 31, 2023
Copy link
Contributor

@mahipv mahipv left a comment

Choose a reason for hiding this comment

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

The patch looks good. Thank you for the extensive test case.

@konskov konskov requested a review from mkindahl February 1, 2023 17:51
@konskov konskov force-pushed the month_intervals_error_fixed_schedules branch 5 times, most recently from 17bebcb to 6291a6d Compare February 8, 2023 15:26
@konskov konskov force-pushed the month_intervals_error_fixed_schedules branch 2 times, most recently from 7fa6217 to 611a885 Compare February 9, 2023 09:52
This patch fixes several issues with next_start calculation.

- Previously, the offset was added twice in some cases.
This is fixed by this patch.

- Additionally, schedule intervals with month components
were not handled correctly.
Internally, time_bucket with origin is used to calculate
the next start. However, in the case of month intervals, the
timestamp calculated for a bucket is always aligned on the first
day of the month, regardless of origin.
Therefore, previously the result was aligned with origin by adding
the difference between origin and its respective time bucket.
This difference was computed as a fixed length interval in terms
of days and time. That computation led to incorrect computation of
next start occasionally, for example when a job should be executed on
the last day of a month.
That is fixed by adding an appropriate interval of months to
initial_start and letting Postgres handle this computation properly.

Fixes timescale#5216
@konskov konskov force-pushed the month_intervals_error_fixed_schedules branch from 611a885 to 0a94670 Compare February 9, 2023 14:26
@konskov konskov merged commit 348796f into timescale:main Feb 9, 2023
@mahipv mahipv mentioned this pull request Feb 14, 2023
mahipv added a commit that referenced this pull request Feb 20, 2023
This release contains new features and bug fixes since the 2.9.3
release.

This release is high priority for upgrade. We strongly recommend that
you upgrade as soon as possible.

**Features**
* #5241 Allow RETURNING clause when inserting into compressed chunks
* #5245 Manage life-cycle of connections via memory contexts
* #5246 Make connection establishment interruptible
* #5253 Make data node command execution interruptible
* #5243 Enable real-time aggregation for continuous aggregates with
joins
* #5262 Extend enabling compression on a continuous aggregrate with
'compress_segmentby' and 'compress_orderby' parameters

**Bugfixes**
* #4926 Fix corruption when inserting into compressed chunks
* #5218 Add role-level security to job error log
* #5214 Fix use of prepared statement in async module
* #5290 Compression can't be enabled on continuous aggregates when
segmentby/orderby columns need quotation
* #5239 Fix next_start calculation for fixed schedules
mahipv pushed a commit that referenced this pull request Feb 21, 2023
This release contains new features and bug fixes since the 2.9.3 release.
We deem it moderate priority for upgrading.

This release includes these noteworthy features:
* Joins in continuous aggregates
* Re-architecture of how compression works: ~2x improvement on INSERT rate into compressed chunks.
* Full PostgreSQL 15 support for all existing features. Support for the newly introduced MERGE command on hypertables will be introduced on a follow-up release.

**PostgreSQL 12 deprecation announcement**
We will continue supporting PostgreSQL 12 until July 2023. Sooner to that time, we will announce the specific version of TimescaleDB in which PostgreSQL 12 support will not be included going forward.

**Old format of Continuous Aggregates deprecation announcement**
TimescaleDB 2.7 introduced a new format for continuous aggregates that improves performance.
All instances with Continuous Aggregates using the old format should [migrate to the new format](https://docs.timescale.com/api/latest/continuous-aggregates/cagg_migrate/) by July 2023,
when support for the old format will be removed.
Sooner to that time, we will announce the specific version of TimescaleDB in which support for this feature will not be included going forward.

**Features**
* #4874 Allow joins in continuous aggregates
* #4926 Refactor INSERT into compressed chunks
* #5241 Allow RETURNING clause when inserting into compressed chunks
* #5245 Manage life-cycle of connections via memory contexts
* #5246 Make connection establishment interruptible
* #5253 Make data node command execution interruptible
* #5262 Extend enabling compression on a continuous aggregrate with 'compress_segmentby' and 'compress_orderby' parameters

**Bugfixes**
* #5214 Fix use of prepared statement in async module
* #5218 Add role-level security to job error log
* #5239 Fix next_start calculation for fixed schedules
* #5290 Fix enabling compression on continuous aggregates with columns requiring quotation

**Thanks**
* @henriquegelio for reporting the issue on fixed schedules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: add_job() is not respecting scheduled interval when it's big enough
5 participants