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

Do not clobber the baserel cache on UDF error #4890

Merged
merged 2 commits into from
Nov 1, 2022
Merged

Conversation

akuzm
Copy link
Member

@akuzm akuzm commented Oct 27, 2022

The baserel cache should only be allocated and freed by the top-level query.

Also enable and fix -Wclobbered which could have prevented this error.

Fixes #4823

Disable-check: commit-count

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #4890 (7864c4f) into main (d51fefb) will increase coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 7864c4f differs from pull request most recent head 175029c. Consider uploading reports for the commit 175029c to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4890      +/-   ##
==========================================
+ Coverage   89.46%   89.59%   +0.12%     
==========================================
  Files         225      225              
  Lines       50288    50146     -142     
==========================================
- Hits        44992    44928      -64     
+ Misses       5296     5218      -78     
Impacted Files Coverage Δ
tsl/src/remote/async.c 85.07% <ø> (ø)
src/bgw/job_stat.c 91.77% <100.00%> (-0.64%) ⬇️
src/chunk.c 91.94% <100.00%> (+0.83%) ⬆️
src/compression_with_clause.c 88.28% <100.00%> (ø)
src/planner/planner.c 95.98% <100.00%> (+0.18%) ⬆️
src/telemetry/telemetry.c 84.51% <100.00%> (ø)
tsl/src/partialize_finalize.c 95.94% <100.00%> (+0.36%) ⬆️
src/loader/bgw_message_queue.c 86.36% <0.00%> (-2.85%) ⬇️
src/bgw/scheduler.c 83.41% <0.00%> (-1.75%) ⬇️
... and 24 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 20cdd9c...175029c. Read the comment docs.

Comment on lines +443 to +448
/*
* Volatile is needed because these are the local variables that are
* modified between setjmp/longjmp calls.
*/
volatile bool reset_fetcher_type = false;
volatile bool reset_baserel_info = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

So that's what the -Wclobbered warning was about... I'll fix the other places separately.

The baserel cache should only be allocated and freed by the top-level
query.
The one in job_stat.c could probably lead to errors.
@akuzm akuzm merged commit 840f144 into timescale:main Nov 1, 2022
@akuzm
Copy link
Member Author

akuzm commented Nov 14, 2022

Backports should also include this commit to remove the accidental debug output: 9964ba8

@akuzm akuzm added this to the TimescaleDB 2.8.2 milestone Nov 14, 2022
@horzsolt horzsolt modified the milestones: TimescaleDB 2.8.2, TimescaleDB 2.9 Nov 23, 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]: 2.7 crashes when executing multiple nested IMMUTABLE functions (2.6 was fine)
4 participants