Get rid of unsafe logs. Fix NULL UUID case. #3102
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes unsafe logging that could cause issues during error handling and fixes a bug where empty UUID strings were not properly handled as NULL values, causing parsing errors when querying tables with UUID columns.
Key Changes:
- Removed diagnostic logging (memory reports, backtraces, runtime stats) that was being called in unsafe contexts
- Added NULL handling for empty UUID strings in two code paths to prevent "Failed to parse UUID string" errors
- Added comprehensive test coverage for the UUID empty string scenario
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cpp/deeplake_pg/reporter.hpp |
Removed memory reporting, backtrace logging, and runtime statistics functions that were unsafe during signal handling and cleanup |
cpp/deeplake_pg/extension_init.cpp |
Removed warning logs from exception handlers during transaction cleanup to prevent cascading errors |
cpp/deeplake_pg/duckdb_deeplake_scan.cpp |
Added empty string checks before UUID parsing to treat empty strings as NULL and prevent parsing errors |
postgres/tests/py_tests/test_uuid.py |
Added test case to verify empty UUID strings are handled correctly when adding UUID columns to existing tables |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inline static void signal_handler(int signal) | ||
| { | ||
| elog(NOTICE, "Caught signal %d (%s)\n", signal, strsignal(signal)); | ||
| elog(NOTICE, "%s", base::backtrace().c_str()); | ||
| print_memory_report(); | ||
| fflush(stderr); | ||
| _exit(signal); | ||
| } |
There was a problem hiding this comment.
The signal_handler function now only calls _exit without any logging or diagnostic information. While this may be intentional to fix unsafe logging, it makes debugging signal-related crashes much more difficult. Consider adding a comment explaining why logging was removed and what alternative debugging approach should be used.
| elog(INFO, "%s: %.2f %s", task_name_.data(), duration, period_name.data()); | ||
| } | ||
| } | ||
| ~runtime_printer() = default; |
There was a problem hiding this comment.
The runtime_printer destructor is now defaulted, but the class still stores start_time_ member that appears unused. This creates dead code - either remove the start_time_ member variable or restore the timing functionality with a safer logging approach.
| pg::table_storage::instance().reset_requested_columns(); | ||
| } catch (const std::exception& e) { | ||
| elog(WARNING, "Error during transaction cleanup: %s", e.what()); | ||
| // Silently handle cleanup errors |
There was a problem hiding this comment.
While silencing errors may prevent cascading failures, completely silent error handling makes debugging difficult. Consider using a safer logging mechanism (e.g., appending to a file) or add a comment explaining what specific cascading error this prevents and how to debug issues when cleanup fails.
|
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context