Consume ReadyForQuery message after an error over SQL #7558
Conversation
|
This seems basically reasonable to me, but I'd like @fantix to take a look, since I don't understand the state machine fully. In particular, I don't understand the intricacies of |
| else: | ||
| break | ||
|
|
||
| # don't break, because we want to also consume ReadyForQuery |
There was a problem hiding this comment.
| # don't break, because we want to also consume ReadyForQuery | |
| # don't break, because we want to also exhaust the | |
| # remaining buffer with "ignore_till_sync" and consume | |
| # ReadyForQuery if there is one |
There was a problem hiding this comment.
I'm sorry. Now, I think it was right to break here - because it only breaks the inner loop that handles pg responses for the outer loop's current action, and an ErrorResponse does mean the action is resolved.
Then, the outer loop will continue with ignore_till_sync=True and skip all the actions until SYNC, which is guaranteed to be there in pg_ext.simple_query() (which is the only case when send_sync_on_error=True). The ReadyForQuery will then be processed and forwarded to the frontend correctly.
We had an issue because send_sync_on_error=True sent an extra SYNC to PG, and we didn't have a corresponding action to consume its resulting ReadyForQuery.
The first returning value of _parse_sql_extended_query() simply means that the frontend should remain in its own ignore_till_sync mode. For simple query, this returned value is simply ignored because simple query should always clear ignore_till_sync state returning a ReadyForQuery (equivalent to finalizing with SYNC).
#7560 should do
|
Oh wait I just saw Sully saying Hmm, I think that was just doing the same thing as this PR, but in a wrong way, as we already included a |
| else: | ||
| break | ||
|
|
||
| # don't break, because we want to also consume ReadyForQuery |
There was a problem hiding this comment.
I'm sorry. Now, I think it was right to break here - because it only breaks the inner loop that handles pg responses for the outer loop's current action, and an ErrorResponse does mean the action is resolved.
Then, the outer loop will continue with ignore_till_sync=True and skip all the actions until SYNC, which is guaranteed to be there in pg_ext.simple_query() (which is the only case when send_sync_on_error=True). The ReadyForQuery will then be processed and forwarded to the frontend correctly.
We had an issue because send_sync_on_error=True sent an extra SYNC to PG, and we didn't have a corresponding action to consume its resulting ReadyForQuery.
The first returning value of _parse_sql_extended_query() simply means that the frontend should remain in its own ignore_till_sync mode. For simple query, this returned value is simply ignored because simple query should always clear ignore_till_sync state returning a ReadyForQuery (equivalent to finalizing with SYNC).
#7560 should do
|
Great, thanks fantix. |
Trying to fix #7471