sql: transaction error state is not entered after a serializable restart

[jordanlewis]

REPRO:

Open 2 connections to a 20.2 binary.

Connection 1:

root@:26257/defaultdb> create table test (a int);
CREATE TABLE
root@:26257/defaultdb> begin;
BEGIN
root@:26257/defaultdb  OPEN> select count(*) from a;
  count
---------
    102
(1 row)

Connection 2:

root@:26257/defaultdb> begin;
BEGIN
root@:26257/defaultdb  OPEN> select count(*) from a;
  count
---------
    101
(1 row)
root@:26257/defaultdb  OPEN> insert into a values(3);
INSERT 1

Connection 1:

root@:26257/defaultdb  OPEN> insert into a values(3);
INSERT 1

root@:26257/defaultdb  OPEN> commit;
COMMIT

Time: 13ms total (execution -85636855ms / network 85636868ms)

Connection 2:

root@:26257/defaultdb  OPEN> commit;
ERROR: restart transaction: TransactionRetryWithProtoRefreshError: TransactionRetryError: retry txn (RETRY_SERIALIZABLE - failed preemptive refresh): "sql txn" meta={id=e1d89077 key=/Table/517/1/595963585227325441/0 pri=0.01703040 epo=0 ts=1601944049.581889000,2 min=1601944044.603606000,0 seq=2} lock=true stat=PENDING rts=1601944044.603606000,0 wto=false max=1601944045.103606000,0
SQLSTATE: 40001
root@:26257/defaultdb>
root@:26257/defaultdb> rollback;
ERROR: there is no transaction in progress

The bug, in my opinion, is that in connection 2, we don't go into the error state: we simply leave the transaction completely. I think that we should be able to type rollback to get out of the transaction.

It seems like this bug was also present in 20.1. I noticed this while running some of the tests for Diesel (#13787).

[jordanlewis]

This is unexpected, right? cc @andreimatei @rafiss

[rafiss]

Should that first create be create table a instead of test?

Also, why does select count(*) from a; disagree between the two sessions?

I ran the commands on Postgres (with serializable isolation), and I was also unable to run rollback in the second session.

[jordanlewis]

I ran the commands on Postgres (with serializable isolation), and I was also unable to run rollback in the second session.

Huh, really? So it went from "Open" to "no transaction" state without an intermediary error state? If so, I'd retract this issue. Maybe I mis-attributed the cause of the Rust test failure.

[rafiss]

Correct. Here's the PG output from the last bit of session 2:

rafiss@127:postgres> commit;
could not serialize access due to read/write dependencies among transactions
DETAIL:  Reason code: Canceled on identification as a pivot, during commit attempt.
HINT:  The transaction might succeed if retried.

Time: 0.001s
rafiss@127:postgres> rollback;
WARNING:  there is no transaction in progress

ROLLBACK

There is a difference here though -- in PG the rollback is a warning, while CRDB treats it as an error. Maybe that's why the Rust test failed?

Seems related to #54954

[jordanlewis]

Whew, that's good. I should have tried this myself. It still feels weird? But as long as it's compatible.

Yes, it looks like the issue you linked is related. We should link that one to the Diesel issue instead of this one.

[andreimatei]

Huh, really? So it went from "Open" to "no transaction" state without an intermediary error state? If so, I'd retract this issue.

Yeah, COMMIT is final, you can't hold locks after it. That's why we have rollback to savepoint cockroach_restart - if you want to catch these retriable errors.