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

Don't capture errors estimating time max spread #7912

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkindahl
Copy link
Contributor

@mkindahl mkindahl commented Apr 4, 2025

In the time utility functions errors were captured to, it seems, handle syntax errors and out-of-range errors, but this also capture hard errors like out-of-memory and continued to execute, which can cause panics as a result of error stack being exhausted.

This commit removes the capture and allow the error to propagate to callers, including out-of-range errors and syntax errors.

The PG_TRY in estimate_max_spread_var was added in commit 9a29f04 but it is not clear in that commit why it was needed but removing it does not cause any test failures.

@mkindahl mkindahl force-pushed the remove-catch-in-time-utils branch 4 times, most recently from 291d714 to 98dd6a7 Compare April 4, 2025 09:34
In the time utility functions errors were captured to, it seems, handle
syntax errors and out-of-range errors, but this also capture hard
errors like out-of-memory and continued to execute, which can cause
panics as a result of error stack being exhausted.

This commit removes the capture and allow the error to propagate to
callers, including out-of-range errors and syntax errors.

The `PG_TRY` in `estimate_max_spread_var` was added in commit
9a29f04 but it is not clear in that
commit why it was needed but removing it does not cause any test
failures.
@mkindahl mkindahl force-pushed the remove-catch-in-time-utils branch from 98dd6a7 to a62abbb Compare April 4, 2025 09:35
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.91%. Comparing base (59f50f2) to head (a62abbb).
Report is 866 commits behind head on main.

Files with missing lines Patch % Lines
src/estimate.c 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7912      +/-   ##
==========================================
+ Coverage   80.06%   81.91%   +1.84%     
==========================================
  Files         190      249      +59     
  Lines       37181    46129    +8948     
  Branches     9450    11563    +2113     
==========================================
+ Hits        29770    37787    +8017     
- Misses       2997     3785     +788     
- Partials     4414     4557     +143     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

1 participant