sql: AS OF SYSTEM TIME transactions don't keep their timestamp across restarts

[andreimatei]

If one restarts an AS OF SYSTEM TIME transaction, the timestamp after the restart should be what it was before the restart. Unfortunately, that's not what happens when restarting the transaction after an error (either after a retriable error or a non-retriable one); the transaction's timestamp becomes current. Retriable errors are not supposed to happen for these transactions since they have no uncertainty and they're supposed to be read-only. But that leaves the non-retriable errors - executing a rollback to savepoint cockroach_restart after such an error wipes out the historical attributes of the txn.

Check out the proof pudding below - notice how the cluster_logical_timestamp() changes.

This is because of how we restart the transaction - we fail to provide a "historicalTimestamp" for the new txn:

cockroach/pkg/sql/conn_executor_savepoints.go

Lines 165 to 167 in afc823e

	payload := makeEventTxnStartPayload(
	ex.state.priority, rwMode, ex.state.sqlTimestamp,
	nil /* historicalTimestamp */, ex.transitionCtx)

Ironically, we even have a test for this specific case:

cockroach/pkg/sql/txn_state_test.go

Line 650 in aa76180

evPayload: makeEventTxnStartPayload(pri, tree.ReadOnly, now.GoTime(),

Amusingly, the test mocks out the very thing that it ought to be testing - the event generation.

I think the upcoming savepoints code will fix this for the non-retriable error case because, in the case discussed, we're not going to be creating a new transaction any more. We're actually going to be rolling back the existing transaction, so all its attributes should naturally stay in place.

Repro:

root@:26257/defaultdb> BEGIN AS OF SYSTEM TIME '-1h';
Now adding input for a multi-line SQL transaction client-side (smart_prompt enabled).
Press Enter two times to send the SQL text collected so far to the server, or Ctrl+C to cancel.
You can also use \show to display the statements entered so far.
                    ->
BEGIN

Time: 169µs

root@:26257/defaultdb  OPEN> savepoint cockroach_restart;
SAVEPOINT

root@:26257/defaultdb  OPEN> select cluster_logical_timestamp();
    cluster_logical_timestamp
----------------------------------
  1582918814760951000.0000000000
(1 row)

root@:26257/defaultdb  OPEN> select crdb_internal.force_error('a','b');
ERROR: b
SQLSTATE: a
root@:26257/? ERROR> rollback to savepoint cockroach_restart;
ROLLBACK

root@:26257/defaultdb  OPEN> select cluster_logical_timestamp();
    cluster_logical_timestamp
----------------------------------
  1582922445591281000.0000000000
(1 row)

cc @ajwerner @knz

Jira issue: CRDB-5147

[ajwerner]

@andreimatei what's the status on this guy? Does it even matter? I could be more offended by this. I'm tempted to close and let this document the situation should anybody stumble into it again.

[andreimatei]

The sequence above works fine now. It appears to still be broken in case of retriable errors (but not non-retriable ones). AOST transactions shouldn't get any retriable errors though (being r/o with no uncertainty); I've injected one with force_retry() to check.
So I think we're pretty good.