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

Improve cagg query chunk exclusion #5824

Merged
merged 1 commit into from Jun 28, 2023

Conversation

svenklemm
Copy link
Member

@svenklemm svenklemm commented Jun 27, 2023

This patch changes the time_bucket exclusion in cagg queries to distinguish between < and <=. Previously those were treated the same leading to failure to exclude chunks when the constraints where exactly at the bucket boundary.

@github-actions
Copy link

@erimatnor, @pmwkaa: please review this pull request.

Powered by pull-review

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #5824 (f216302) into main (e2e7e5f) will decrease coverage by 0.77%.
The diff coverage is 86.36%.

@@            Coverage Diff             @@
##             main    #5824      +/-   ##
==========================================
- Coverage   87.87%   87.11%   -0.77%     
==========================================
  Files         239      239              
  Lines       55698    55311     -387     
  Branches    12335    12209     -126     
==========================================
- Hits        48945    48183     -762     
+ Misses       4887     4831      -56     
- Partials     1866     2297     +431     
Impacted Files Coverage Δ
src/planner/expand_hypertable.c 89.01% <86.36%> (-0.73%) ⬇️

... and 105 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@svenklemm svenklemm force-pushed the cagg_exclusion branch 3 times, most recently from ff12444 to 92dc54d Compare June 28, 2023 07:35
@svenklemm svenklemm added this to the TimescaleDB 2.11.1 milestone Jun 28, 2023
src/planner/expand_hypertable.c Outdated Show resolved Hide resolved
src/planner/expand_hypertable.c Outdated Show resolved Hide resolved
src/planner/expand_hypertable.c Show resolved Hide resolved
src/planner/expand_hypertable.c Outdated Show resolved Hide resolved
src/planner/expand_hypertable.c Show resolved Hide resolved
src/planner/expand_hypertable.c Show resolved Hide resolved
@svenklemm svenklemm force-pushed the cagg_exclusion branch 2 times, most recently from 5e8ea99 to ef1cb9a Compare June 28, 2023 11:21
@svenklemm svenklemm requested a review from erimatnor June 28, 2023 11:29
jnidzwetzki added a commit to jnidzwetzki/timescaledb that referenced this pull request Jun 28, 2023
This release contains bug fixes since the 2.11.0 release. We recommend
that you upgrade at the next available opportunity.

**Features**
* timescale#5679 Teach loader to load OSM extension

**Bugfixes**
* timescale#5705 Scheduler accidentally getting killed when calling `delete_job`
* timescale#5742 Fix Result node handling with ConstraintAwareAppend on
  compressed chunks
* timescale#5750 Ensure tlist is present in decompress chunk plan
* timescale#5754 Fixed handling of NULL values in bookend_sfunc
* timescale#5798 Fixed batch look ahead in compressed sorted merge
* timescale#5804 Mark cagg_watermark function as PARALLEL RESTRICTED
* timescale#5807 Copy job config JSONB structure into current MemoryContext
* timescale#5824 Improve continuous aggregate query chunk exclusion

**Thanks**
* @JamieD9 for reporting an issue with a wrong result ordering
* @xvaara for reporting an issue with Result node handling in
  ConstraintAwareAppend
This patch changes the time_bucket exclusion in cagg queries to
distinguish between < and <=. Previously those were treated the
same leading to failure to exclude chunks when the constraints
where exactly at the bucket boundary.
Copy link
Contributor

@erimatnor erimatnor left a comment

Choose a reason for hiding this comment

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

Approving, but I still have a concern about that inclusive check; see inline comment.

src/planner/expand_hypertable.c Show resolved Hide resolved
return TimestampTzGetDatum(value);
}

elog(ERROR, "unsupported datatype in int_get_datum: %s", format_type_be(type));
Copy link
Contributor

@erimatnor erimatnor Jun 28, 2023

Choose a reason for hiding this comment

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

Still not sure why we have this error since it is impossible to reach here given the type check already outside this function. Also codecov is complaining. pg_unreachable() should be sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit of a grey area. Code gets refactored all the time and conditions that are true now might not be later. I prefer to have an error here over just pg_unreachable because an error will only affect the session hitting this while pg_unreachable will kill all sessions and have postgres go into recovery.

@svenklemm svenklemm enabled auto-merge (rebase) June 28, 2023 13:56
@svenklemm svenklemm merged commit 118526e into timescale:main Jun 28, 2023
40 of 41 checks passed
jnidzwetzki added a commit that referenced this pull request Jun 28, 2023
This release contains bug fixes since the 2.11.0 release. We recommend
that you upgrade at the next available opportunity.

**Features**
* #5679 Teach loader to load OSM extension

**Bugfixes**
* #5705 Scheduler accidentally getting killed when calling `delete_job`
* #5742 Fix Result node handling with ConstraintAwareAppend on
  compressed chunks
* #5750 Ensure tlist is present in decompress chunk plan
* #5754 Fixed handling of NULL values in bookend_sfunc
* #5798 Fixed batch look ahead in compressed sorted merge
* #5804 Mark cagg_watermark function as PARALLEL RESTRICTED
* #5807 Copy job config JSONB structure into current MemoryContext
* #5824 Improve continuous aggregate query chunk exclusion

**Thanks**
* @JamieD9 for reporting an issue with a wrong result ordering
* @xvaara for reporting an issue with Result node handling in
  ConstraintAwareAppend
jnidzwetzki added a commit to jnidzwetzki/timescaledb that referenced this pull request Jun 28, 2023
This release contains bug fixes since the 2.11.0 release. We recommend
that you upgrade at the next available opportunity.

**Features**
* timescale#5679 Teach loader to load OSM extension

**Bugfixes**
* timescale#5705 Scheduler accidentally getting killed when calling `delete_job`
* timescale#5742 Fix Result node handling with ConstraintAwareAppend on
  compressed chunks
* timescale#5750 Ensure tlist is present in decompress chunk plan
* timescale#5754 Fixed handling of NULL values in bookend_sfunc
* timescale#5798 Fixed batch look ahead in compressed sorted merge
* timescale#5804 Mark cagg_watermark function as PARALLEL RESTRICTED
* timescale#5807 Copy job config JSONB structure into current MemoryContext
* timescale#5824 Improve continuous aggregate query chunk exclusion

**Thanks**
* @JamieD9 for reporting an issue with a wrong result ordering
* @xvaara for reporting an issue with Result node handling in
  ConstraintAwareAppend
jnidzwetzki added a commit that referenced this pull request Jun 28, 2023
This release contains bug fixes since the 2.11.0 release. We recommend
that you upgrade at the next available opportunity.

**Features**
* #5679 Teach loader to load OSM extension

**Bugfixes**
* #5705 Scheduler accidentally getting killed when calling `delete_job`
* #5742 Fix Result node handling with ConstraintAwareAppend on
  compressed chunks
* #5750 Ensure tlist is present in decompress chunk plan
* #5754 Fixed handling of NULL values in bookend_sfunc
* #5798 Fixed batch look ahead in compressed sorted merge
* #5804 Mark cagg_watermark function as PARALLEL RESTRICTED
* #5807 Copy job config JSONB structure into current MemoryContext
* #5824 Improve continuous aggregate query chunk exclusion

**Thanks**
* @JamieD9 for reporting an issue with a wrong result ordering
* @xvaara for reporting an issue with Result node handling in
  ConstraintAwareAppend
jnidzwetzki added a commit that referenced this pull request Jun 28, 2023
This release contains bug fixes since the 2.11.0 release. We recommend
that you upgrade at the next available opportunity.

**Features**
* #5679 Teach loader to load OSM extension

**Bugfixes**
* #5705 Scheduler accidentally getting killed when calling `delete_job`
* #5742 Fix Result node handling with ConstraintAwareAppend on
  compressed chunks
* #5750 Ensure tlist is present in decompress chunk plan
* #5754 Fixed handling of NULL values in bookend_sfunc
* #5798 Fixed batch look ahead in compressed sorted merge
* #5804 Mark cagg_watermark function as PARALLEL RESTRICTED
* #5807 Copy job config JSONB structure into current MemoryContext
* #5824 Improve continuous aggregate query chunk exclusion

**Thanks**
* @JamieD9 for reporting an issue with a wrong result ordering
* @xvaara for reporting an issue with Result node handling in
  ConstraintAwareAppend
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants