Skip to content

Fix tryGetTableMetadata to catch all exception types#100990

Open
tuanpach wants to merge 1 commit intoClickHouse:masterfrom
tuanpach:fix-rest-catalog-try-get-table-metadata-exceptions
Open

Fix tryGetTableMetadata to catch all exception types#100990
tuanpach wants to merge 1 commit intoClickHouse:masterfrom
tuanpach:fix-rest-catalog-try-get-table-metadata-exceptions

Conversation

@tuanpach
Copy link
Copy Markdown
Member

Previously only DB::Exception was caught, allowing Poco::Exception and std::exception thrown by Poco::JSON::Parser or other code inside getTableMetadataImpl to propagate unexpectedly from a try* API that is supposed to return false on failure.

Major issue of #92993

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

...

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Previously only `DB::Exception` was caught, allowing `Poco::Exception`
and `std::exception` thrown by `Poco::JSON::Parser` or other code inside
`getTableMetadataImpl` to propagate unexpectedly from a `try*` API that
is supposed to return `false` on failure.
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 28, 2026

Workflow [PR], commit [d600685]

Summary:

job_name test_name status info comment
Stateless tests (amd_debug, distributed plan, s3 storage, parallel) failure
03632_skip_indices_logged_in_query_log FAIL cidb
Stateless tests (amd_tsan, s3 storage, parallel, 1/2) failure
03632_skip_indices_logged_in_query_log FAIL cidb

AI Review

Summary

This PR updates RestCatalog::tryGetTableMetadata to handle non-DB::Exception failures from getTableMetadataImpl and return false consistently for the try* API contract. The change is small, localized, and aligns with the stated intent; I did not find a high-confidence correctness, safety, or performance issue that requires an inline comment.

ClickHouse Rules
Item Status Notes
Deletion logging
Serialization versioning
Core-area scrutiny
No test removal
Experimental gate
No magic constants
Backward compatibility
SettingsChangesHistory.cpp
PR metadata quality
Safe rollout
Compilation time
Final Verdict
  • Status: ✅ Approve

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 28, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Mar 28, 2026

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.10% 84.20% +0.10%
Functions 24.60% 24.50% -0.10%
Branches 76.70% 76.70% +0.00%

Changed lines: 0.00% (0/14) · Uncovered code

Full report · Diff report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant