Add query id to error messages when possible #518
Conversation
slabko
force-pushed
the
exceptions-with-query-id
branch
3 times, most recently
from
9634feb to
7fbe00b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error reporting by adding query IDs to error messages for SQL statement handles and fixes a critical bug in the AmortizedIStreamReader destructor. The changes improve debugging capabilities by making it easier to correlate errors with queries in the system.query_log.
- Added query ID context to error messages for statement-related exceptions
- Fixed an infinite loop bug in the AmortizedIStreamReader destructor
- Added comprehensive tests for error handling scenarios
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| driver/utils/amortized_istream_reader.h | Fixed infinite loop in destructor by using reverse iterators |
| driver/test/error_handling_it.cpp | Added test cases for immediate and mid-stream error scenarios |
| driver/test/CMakeLists.txt | Added error_handling_it.cpp to test targets |
| driver/statement.h | Added query ID storage and getter method |
| driver/statement.cpp | Implemented query ID extraction from HTTP response headers |
| driver/platform/platform.h | Added internal SQL statement attribute for query ID |
| driver/driver.h | Added method to enhance error messages with context |
| driver/driver.cpp | Implemented context-aware error message enhancement |
| driver/api/impl/impl.cpp | Added SQLGetStmtAttr support for query ID attribute |
| if (available() > 0) { | ||
| for (std::size_t i = buffer_.size() - 1; i >= offset_; --i) { | ||
| raw_stream_.putback(buffer_[i]); | ||
| for (auto it = buffer_.rbegin(); it < buffer_.rend() - offset_; ++it) { |
There was a problem hiding this comment.
The loop condition it < buffer_.rend() - offset_ is incorrect. When offset_ is greater than 0, buffer_.rend() - offset_ moves the iterator backward from the end, but reverse iterators should be compared using != or <= operators. The correct condition should be it != buffer_.rend() - offset_ or it <= buffer_.rend() - offset_.
| for (auto it = buffer_.rbegin(); it < buffer_.rend() - offset_; ++it) { | |
| for (auto it = buffer_.rbegin(); it != buffer_.rend() - offset_; ++it) { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Even the suggestion of using != or <= doesn't make any sense. != is non-inclusive, while <= is inclusive.
driver/test/error_handling_it.cpp
Outdated
| */ | ||
| TEST_F(ErrorHandlingTest, ServerExceptionInBeginning) | ||
| { | ||
| static const size_t stream_size = 100000; |
There was a problem hiding this comment.
The variable stream_size is declared but never used in this test function. It appears to be copied from the second test function where it is actually used.
| static const size_t stream_size = 100000; |
Copilot uses AI. Check for mistakes.
| SQLINTEGER query_id_len{}; | ||
| SQLGetStmtAttr(hstmt, SQL_CH_STMT_ATTR_LAST_QUERY_ID, query_id_data, std::size(query_id_data), &query_id_len); | ||
|
|
||
| #ifdef _win_ |
There was a problem hiding this comment.
The preprocessor directive should be #ifdef _WIN32 or #ifdef _WINDOWS, not #ifdef _win_. The underscore at the end makes this an invalid Windows platform check.
| #ifdef _win_ | |
| #ifdef _WIN32 |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
the project has its own definition in platform.h
#if defined(_win32_) || defined(_win64_)
# define _win_ 1
#endif
slabko
force-pushed
the
exceptions-with-query-id
branch
from
7fbe00b to
f47c22f
Compare
SQL_HANDLE_STMT).SQL_CH_STMT_ATTR_LAST_QUERY_IDSQL Statement Attribute was also added to retrieve the last query ID. This is only meant for internal use.AmortizedIStreamReaderdestructor, which previously contained an infinite loop that could cause an overflow.The extended error message should be especially helpful for debugging issues such as #419: Incomplete Input Stream - it will make it possible to find the status of the associated queries in the
system.query_log.