Conversation
|
Claude finished @khustup2's task in 1m 25s —— View job PR Review CompleteFound 7 issues requiring attention: Critical Issues
Test Coverage
All issues have inline comments with suggested fixes. SummaryThis PR improves catalog sync latency by:
The optimizations look sound, but need to address the unreachable code and potential nesting issues in the guard implementation. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } catch (const std::exception& e) { | ||
| catalog_table_cache::instance().invalidate(); | ||
| elog(ERROR, "Failed to ensure catalog at %s: %s", root_path.c_str(), e.what()); | ||
| return 0; |
There was a problem hiding this comment.
elog(ERROR, ...) throws an exception in PostgreSQL, so return 0 is unreachable and misleading. Same issue on line 212.
Remove the unreachable return statements after elog(ERROR):
| return 0; | |
| elog(ERROR, "Failed to ensure catalog at %s: %s", root_path.c_str(), e.what()); |
| } catch (...) { | ||
| catalog_table_cache::instance().invalidate(); | ||
| elog(ERROR, "Failed to ensure catalog at %s: unknown error", root_path.c_str()); | ||
| return 0; |
There was a problem hiding this comment.
elog(ERROR, ...) throws an exception in PostgreSQL, so this return is unreachable.
Remove the unreachable return statement:
| return 0; | |
| elog(ERROR, "Failed to ensure catalog at %s: unknown error", root_path.c_str()); |
| public: | ||
| catalog_only_guard() | ||
| { | ||
| catalog_only_create_ = true; |
There was a problem hiding this comment.
Thread-local flags without nested tracking can fail if guards nest (e.g., concurrent calls or recursive create_table). If outer guard exits before inner, inner's restore to false disables the flag prematurely.
Add nesting counter to handle reentrant calls safely:
| catalog_only_create_ = true; | |
| ++catalog_only_create_; |
| } | ||
| ~catalog_only_guard() | ||
| { | ||
| catalog_only_create_ = false; |
There was a problem hiding this comment.
Should decrement instead of setting to false to support nested guards.
Decrement the counter:
| catalog_only_create_ = false; | |
| --catalog_only_create_; |
|
|
||
| static bool is_catalog_only_create() noexcept | ||
| { | ||
| return catalog_only_create_; |
There was a problem hiding this comment.
Should check if counter > 0 to support nesting.
Check if counter is positive:
| return catalog_only_create_; | |
| return catalog_only_create_ > 0; |
|
|
||
| private: | ||
| static inline thread_local bool in_ddl_context_ = false; | ||
| static inline thread_local bool catalog_only_create_ = false; |
There was a problem hiding this comment.
Change to int counter for nesting support.
Use int counter:
| static inline thread_local bool catalog_only_create_ = false; | |
| static inline thread_local int catalog_only_create_ = 0; |
| @@ -19,42 +18,6 @@ def _sql_literal(value: str) -> str: | |||
| return "'" + value.replace("'", "''") + "'" | |||
There was a problem hiding this comment.
Test test_stateless_catalog_recovers_from_legacy_non_catalog_path was removed along with the migration logic. This test verified recovery from corrupted catalog paths. Without it, there's no coverage for scenarios where catalog tables become corrupted or invalid.
Consider: Does the new code handle catalog corruption gracefully, or should this test be updated rather than deleted?
…into stateless-latency
|
1 participant
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context