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

Set cache pointer to null before destroying #5070

Closed
wants to merge 1 commit into from

Conversation

mkindahl
Copy link
Contributor

@mkindahl mkindahl commented Dec 9, 2022

A cache is used for the duration of a query to speed up planning time, but when destroying the cache, an error in the destruction would leave the cache pointer pointing to a partially or fully destroyed cache, which would give incorrect cache hits for the next query.

This commit changes the order of the assignment so that the cache pointer is cleared before destroying the cache. This will allocate a new cache for the next query, even if the previous destruction had an error.

Fixes #4795

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #5070 (7127138) into main (1a806e2) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5070      +/-   ##
==========================================
+ Coverage   89.64%   89.70%   +0.06%     
==========================================
  Files         227      227              
  Lines       51616    51513     -103     
==========================================
- Hits        46270    46210      -60     
+ Misses       5346     5303      -43     
Impacted Files Coverage Δ
src/planner/planner.c 95.97% <100.00%> (+0.14%) ⬆️
tsl/src/remote/async.c 78.43% <0.00%> (-6.64%) ⬇️
src/loader/bgw_launcher.c 89.51% <0.00%> (-2.55%) ⬇️
src/bgw/scheduler.c 83.41% <0.00%> (-2.01%) ⬇️
tsl/src/debug.c 49.36% <0.00%> (-0.33%) ⬇️
tsl/src/nodes/decompress_chunk/decompress_chunk.c 94.91% <0.00%> (-0.20%) ⬇️
tsl/test/src/test_ddl_hook.c 93.18% <0.00%> (-0.08%) ⬇️
src/cache.c 94.93% <0.00%> (-0.07%) ⬇️
tsl/src/bgw_policy/job.c 88.31% <0.00%> (-0.05%) ⬇️
src/planner/expand_hypertable.c 94.22% <0.00%> (-0.02%) ⬇️
... and 13 more

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 1a806e2...7127138. Read the comment docs.

A cache is used for the duration of a query to speed up planning time,
but when destroying the cache, an error in the destruction would leave
the cache pointer pointing to a partially or fully destroyed cache,
which would give incorrect cache hits for the next query.

This commit changes the order of the assignment so that the cache
pointer is cleared before destroying the cache. This will allocate a
new cache for the next query, even if the previous destruction had an
error.

Fixes timescale#4795
@mkindahl
Copy link
Contributor Author

Closing issue in favor of #5086.

@mkindahl mkindahl closed this Dec 14, 2022
@mkindahl mkindahl deleted the baserel-cache-destruction branch December 14, 2022 11:24
@mkindahl mkindahl self-assigned this Dec 14, 2022
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]: Assertion failure in ts_hypertable_has_compression_table
1 participant