[E084-MG < T0557-MG] Fix error handling in explicit transactional mode #11
Conversation
src/cursor.c
Outdated
| @@ -298,7 +298,7 @@ PyObject *cursor_fetchone(CursorObject *cursor, PyObject *args) { | |||
| if (row) { | |||
| Py_DECREF(row); | |||
| } | |||
| connection_handle_error(cursor->conn); | |||
| connection_handle_error(cursor->conn, -1); | |||
There was a problem hiding this comment.
Is it possible to replace -1 with something more understandable?
There was a problem hiding this comment.
I was thinking about what could be done better, but I don't want to speculate based on the return values. At this point the only sure thing is either fetch_status_first or fetch_status_second is -1, therefore I went with -1 intentionally avoiding any kind of speculation about what could go wrong.
To be honest in my opinion the connection_handle_error should be called from inside connection_* functions, because at that point we have more accurate information about the error. Similarly as mg_session handles the error by itself, connection should handle the errors that occur in connection_* functions. Though this would improve the quality of the code base, we have more important things to do with pymgclient. If we have time (or we consider this important), then I can get back to it.
There was a problem hiding this comment.
You are comparing it to MG_ERROR why not use something like MG_ERROR_UNKNOWN_ERROR?
Also shouldn't this be handled correctly because the error could be CLIENT, TRANSIENT or DATABASE, so maybe have connection_fetch return the status directly instead of returning -1 for every failure?
There was a problem hiding this comment.
After going through the changes again after yesterday, I realized a few things:
connection_*functions doesn't returnMG_ERROR_*values, but only-1(failure),0(success),1(onlyconnection_fetchwhen the fetch was successful). We could change this behavior, but that is a workaround, not a solution. The solution would be to callconnection_handle_errorinside theconnection_*functions as I mentioned above.- This refactoring shouldn't be that hard, so I will try to do that. That would solve this problem and make the error handling more straightforward. This is not a "must have" improvement, but in my opinion the best resolution of @antonio2368 's comment.
There was a problem hiding this comment.
I agree, handling of the connection errors should happen directly in the connection functions.
| int fetch_status_second = | ||
| connection_fetch(cursor->conn, NULL, &has_more_second); | ||
| int fetch_status_second = 0; | ||
| if (fetch_status_first == 1) { | ||
| fetch_status_second = | ||
| connection_fetch(cursor->conn, NULL, &has_more_second); | ||
| } |
There was a problem hiding this comment.
This is a small fix for fetches in lazy mode without any result. See the new tests and the failed actions run.
If the first fetch receives the SUCCESS message, then the second fetch failed before. It didn't occurred when the cursor could fetch at least one row, because then the status of the cursor was set to CURSOR_STATUS_READY when the last row is fetched, and the check around line 268 handled this case.
antaljanosbenjamin
deleted the
T0557-MG-fix-error-handling-in-multicommand-mode
branch
This PR aims to fix the handling of the state of the connection when an error happens during query execution which doesn't move the session to MG_SESSION_BAD state, only resets it. When such error happens, then
connection_runshould start a new transaction before running the next query.