Test case & proposed fix for 'unexpected describe rows response' error after transaction rollback

[jmcvetta]

I have encountered a bug in transaction rollback handling. The bug occurs when one opens a transaction, issues a query, rolls back the transaction, then tries to issue another query on the same DB object that was used to create the transaction. It does not matter whether the rolled-back query was an INSERT or a SELECT.

Error message is:

pq: unexpected describe rows response: 'C'

Error is coming from here: https://github.com/lib/pq/blob/master/conn.go#L216

Postgres manual says 'C' is a legitimate response header:

CommandComplete (B)
Byte1('C')
Identifies the message as a command-completed response.

Int32
Length of message contents in bytes, including self.

String
The command tag. This is usually a single word that identifies which SQL command was completed.

For an INSERT command, the tag is INSERT oid rows, where rows is the number of rows inserted. oid is the object ID of the inserted row if rows is 1 and the target table has OIDs; otherwise oid is 0.

For a DELETE command, the tag is DELETE rows where rows is the number of rows deleted.

For an UPDATE command, the tag is UPDATE rows where rows is the number of rows updated.

For a SELECT or CREATE TABLE AS command, the tag is SELECT rows where rows is the number of rows retrieved.

For a MOVE command, the tag is MOVE rows where rows is the number of rows the cursor's position has been changed by.

For a FETCH command, the tag is FETCH rows where rows is the number of rows that have been retrieved from the cursor.

For a COPY command, the tag is COPY rows where rows is the number of rows copied. (Note: the row count appears only in PostgreSQL 8.2 and later.)

Here's how libpq handles it: http://doxygen.postgresql.org/fe-protocol3_8c_source.html#l00199

Test Case

This pull request contains a test case, TestRollback(), that reliably reproduces the bug.

Impact

This bug is currently blocking development of PostgreSQL dialect of qbs ORM: coocood/qbs#11

[jmcvetta]

Commit 6b7b149 adds support for handling a 'C' "command complete" message from server. Not 100% sure this is enough, but it fixes the problem. Passes all pq tests, as well as downstream tests in qbs.

[fdr]

I committed this, but somehow Github is confused. Closing, reopen if there are any problems.

[johto]

In #177 we had a real-world problem which was nastily masked by commit 13601fe. Looking at this test case, it seems to be not closing the rows object as well, and as such the test seems broken, not how pq responds to it.

As far as I can tell, the only time when CommandComplete is actually expected in prepareToSimpleStmt is after an error, but the code does not check for that case. Can anyone see a real-world scenario where that's not the case? If not, I think we should make that function more robust.

[johto]

And sure enough, if I change the INSERT into INSERT .. RETURNING or I use parameters in the INSERT, the test case breaks horribly. It only accidentally fails to break in the simpleQuery code path as long as the server returns no results, which makes the entire test quite pointless.

However, what I suggested isn't entirely accurate either; if an error happens, the server sends just an ErrorResponse followed by ReadyForQuery. I can't come up with a single case where CommandComplete would actually be expected after Parse/Describe/Sync. I think we should get rid of this test and make the code path more robust. I've opened a pull request with a patch implementing that change in #180.

[msakrejda]

Thanks for digging here, and thanks @johto for the PR.