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 race condition when creating CAgg #6001

Conversation

fabriziomello
Copy link
Contributor

@fabriziomello fabriziomello commented Aug 22, 2023

Creating one or more Continuous Aggregates over the same hypertable
concurrently in different sessions lead to a race condition on checking
for the existance of the trigger ts_cagg_invalidation_trigger.

Fixed it by using the OR REPLACE option for the CREATE TRIGGER
statement introduced in PG14 and for prior versions taking an
ShareUpdateExclusiveLock and holding it until the end of the
transaction when checking the trigger existance in the
check_trigger_exists_hypertable function.

Disable-check: force-changelog-file

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #6001 (3f4ccd6) into main (edf0bfa) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main    #6001      +/-   ##
==========================================
- Coverage   81.53%   81.53%   -0.01%     
==========================================
  Files         246      246              
  Lines       56581    56539      -42     
  Branches    12537    12519      -18     
==========================================
- Hits        46135    46099      -36     
+ Misses       8085     8051      -34     
- Partials     2361     2389      +28     
Files Changed Coverage Δ
tsl/src/deparse.c 72.10% <66.66%> (-0.04%) ⬇️
src/trigger.c 91.17% <100.00%> (+0.13%) ⬆️
tsl/src/continuous_aggs/create.c 90.16% <100.00%> (ø)

... and 45 files with indirect coverage changes

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

@fabriziomello fabriziomello force-pushed the fix_race_condition_when_adding_cagg_trigger branch from adcc62f to 7b2390f Compare August 23, 2023 12:41
@fabriziomello fabriziomello force-pushed the fix_race_condition_when_adding_cagg_trigger branch 2 times, most recently from 307913b to 60b6a27 Compare August 31, 2023 15:42
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 should probably have a better commit message and consider if a test needs to be added. Otherwise, I see no problems with this patch.

@fabriziomello fabriziomello force-pushed the fix_race_condition_when_adding_cagg_trigger branch from 60b6a27 to 2a5a970 Compare September 11, 2023 14:26
@fabriziomello fabriziomello marked this pull request as ready for review September 11, 2023 14:26
@fabriziomello
Copy link
Contributor Author

You should probably have a better commit message and consider if a test needs to be added. Otherwise, I see no problems with this patch.

Done!

@github-actions
Copy link

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

Powered by pull-review

@fabriziomello fabriziomello force-pushed the fix_race_condition_when_adding_cagg_trigger branch 2 times, most recently from 6a43979 to 4c986d3 Compare September 12, 2023 13:33
@fabriziomello fabriziomello force-pushed the fix_race_condition_when_adding_cagg_trigger branch from 4c986d3 to 3b2e592 Compare September 15, 2023 12:06
@fabriziomello fabriziomello added the disable-auto-backport Do not automatically backport this PR or fix of this issue label Sep 15, 2023
@fabriziomello fabriziomello enabled auto-merge (rebase) September 15, 2023 12:07
@fabriziomello fabriziomello force-pushed the fix_race_condition_when_adding_cagg_trigger branch from 3b2e592 to 0a4b43c Compare September 15, 2023 13:46
Creating one or more Continuous Aggregates over the same hypertable
concurrently in different sessions lead to a race condition on checking
for the existance of the trigger `ts_cagg_invalidation_trigger`.

Fixed it by using the `OR REPLACE` option for the `CREATE TRIGGER`
statement introduced in PG14 and for prior versions taking an
`ShareUpdateExclusiveLock` and holding it until the end of the
transaction when checking the trigger existance in the
`check_trigger_exists_hypertable` function.
@fabriziomello fabriziomello force-pushed the fix_race_condition_when_adding_cagg_trigger branch from 0a4b43c to 3f4ccd6 Compare September 15, 2023 13:58
@fabriziomello fabriziomello merged commit 2e363e0 into timescale:main Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug continuous_aggregate disable-auto-backport Do not automatically backport this PR or fix of this issue Team: Core Database
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants