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

Relax some JOIN restrictions on CAggs #7032

Conversation

fabriziomello
Copy link
Contributor

@fabriziomello fabriziomello commented Jun 14, 2024

Relax JOIN restrictions on CAggs by allowing

  • INNER/LEFT join
  • LATERAL join
  • Join between 1 hypertable and N regular tables or materialized views
  • Remove restriction of only ONE equality operator on JOIN clause

Closes #1446, #1516, #1717, #2400, #3314, #4088

Disable-check: force-changelog-file

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.79%. Comparing base (59f50f2) to head (b90b464).
Report is 245 commits behind head on main.

Files Patch % Lines
tsl/src/continuous_aggs/common.c 81.81% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7032      +/-   ##
==========================================
+ Coverage   80.06%   81.79%   +1.72%     
==========================================
  Files         190      202      +12     
  Lines       37181    37355     +174     
  Branches     9450     9741     +291     
==========================================
+ Hits        29770    30555     +785     
+ Misses       2997     2891     -106     
+ Partials     4414     3909     -505     

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

@fabriziomello fabriziomello force-pushed the cagg_fix_segfault_on_join_validation branch 2 times, most recently from 762e673 to 0e09a0e Compare June 17, 2024 17:20
@svenklemm
Copy link
Member

Why are we not just allowing this?

@fabriziomello
Copy link
Contributor Author

Why are we not just allowing this?

We will and it will be part of Q3, but for now I just want make sure it will not segfault.

@fabriziomello fabriziomello force-pushed the cagg_fix_segfault_on_join_validation branch from 0e09a0e to c20262c Compare June 18, 2024 17:16
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.

Could you please describe why the segfault occurs and how it was fixed in the commit comment?

tsl/src/continuous_aggs/common.c Outdated Show resolved Hide resolved
tsl/src/continuous_aggs/common.c Outdated Show resolved Hide resolved
tsl/src/continuous_aggs/common.c Outdated Show resolved Hide resolved
@fabriziomello fabriziomello force-pushed the cagg_fix_segfault_on_join_validation branch from c20262c to 56117dc Compare June 19, 2024 14:02
@fabriziomello
Copy link
Contributor Author

Could you please describe why the segfault occurs and how it was fixed in the commit comment?

Done!

@fabriziomello fabriziomello force-pushed the cagg_fix_segfault_on_join_validation branch 2 times, most recently from e3d51f6 to 79bd882 Compare June 20, 2024 20:33
Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

Still segfaults with following query:
CREATE MATERIALIZED VIEW cagg1 with(timescaledb.continuous) as select time_bucket('1d',time) from metrics m1 join metrics m2 using(time) join metrics m3 using(time) group by 1;

@fabriziomello fabriziomello force-pushed the cagg_fix_segfault_on_join_validation branch from 79bd882 to f200ed7 Compare June 21, 2024 20:38
@fabriziomello
Copy link
Contributor Author

Still segfaults with following query: CREATE MATERIALIZED VIEW cagg1 with(timescaledb.continuous) as select time_bucket('1d',time) from metrics m1 join metrics m2 using(time) join metrics m3 using(time) group by 1;

Actually it is failing on main with my patch is even works... it is allowing it :-(

151388 (leader) fabrizio=# CREATE MATERIALIZED VIEW cagg1 with(timescaledb.continuous) as select time_bucket('1d',time) from metrics m1 join metrics m2 using(time) join metrics m3 using(time) group by 1;
NOTICE:  00000: continuous aggregate "cagg1" is already up-to-date
LOCATION:  emit_up_to_date_notice, refresh.c:674
CREATE MATERIALIZED VIEW
Time: 32,843 ms

@fabriziomello
Copy link
Contributor Author

@svenklemm I'll rewrite all this messy join validation... it can be way simpler!!!

@svenklemm
Copy link
Member

It probably takes the same amount of time to change the code to allow it, so why not just do that?

@fabriziomello fabriziomello force-pushed the cagg_fix_segfault_on_join_validation branch from f200ed7 to 2206a22 Compare June 24, 2024 20:46
@fabriziomello fabriziomello removed the segfault Segmentation fault label Jun 24, 2024
@fabriziomello fabriziomello changed the title Fix segfault creating CAgg with multiple joins Relax some JOIN restrictions on CAggs Jun 24, 2024
@fabriziomello fabriziomello marked this pull request as draft June 24, 2024 20:48
@fabriziomello fabriziomello dismissed svenklemm’s stale review June 24, 2024 20:53

Changed the original PR direction

@fabriziomello fabriziomello force-pushed the cagg_fix_segfault_on_join_validation branch 3 times, most recently from 1396ea5 to 6e72d99 Compare June 26, 2024 13:34
Remove some Continuous Aggregates JOIN restrictions by allowing:
* INNER/LEFT join
* LATERAL join
* Join between 1 hypertable and N regular tables or materialized views
* Remove restriction of only ONE equality operator on JOIN clause
@fabriziomello fabriziomello force-pushed the cagg_fix_segfault_on_join_validation branch from b90b464 to 9ee1480 Compare July 8, 2024 13:57
@fabriziomello fabriziomello deleted the cagg_fix_segfault_on_join_validation branch July 8, 2024 13:58
@fabriziomello fabriziomello removed this from the TimescaleDB 2.16.0 milestone Jul 8, 2024
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.

Create continuous aggregate from multiple tables
3 participants