sql: prepared statement with AS OF SYSTEM TIME can create txn atomicity violation

[nvanbenschoten]

In 078c58a, we added an assertion that transaction timestamps are not manually set using the (*Txn).SetFixedTimestamp API after a transaction has already been used. This would be very unsafe, as it would allow transactions to silently change timestamps and violate atomicity. It turns out that we do hit this assertion, which revealed that something was wrong.

The good news is that even without the assertion, we don't seem to be silently violating atomicity in these cases, as another assertion in KV notices the changed txn timestamp. The bad news is that this is possible to hit thanks to the unchecked use of SetFixedTimestamp during statement preparation.

> create table t (x int);
CREATE TABLE
> insert into t values (1);
INSERT 1
> select crdb_internal_mvcc_timestamp, * from t;
   crdb_internal_mvcc_timestamp  | x
---------------------------------+----
  1627526754922910000.0000000000 | 1
(1 row)
> begin as of system time (1627526754922910000.0000000000-1)::string;
BEGIN
  OPEN> select * from t;
  x
-----
(0 rows)
  OPEN> prepare y as select * from t as of system time 1627526754922910000.0000000000;
PREPARE
  OPEN> select * from t;
ERROR: internal error: unexpected batch read timestamp: 1627526754.922910000,0. Expected refreshed timestamp: 1627526754.922909999,0. ba: Scan [/Table/59/1,/Table/59/2), [txn: ad8d53e5]. txn: "sql txn" meta={id=ad8d53e5 pri=0.03468194 epo=0 ts=1627526754.922910000,0 min=1627526754.922909999,0 seq=0} lock=false stat=PENDING rts=1627526754.922910000,0 wto=false gul=1627526754.922910000,0
SQLSTATE: XX000
DETAIL: stack trace:
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_span_refresher.go:160: SendLocked()
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go:285: SendLocked()
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_seq_num_allocator.go:105: SendLocked()
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_interceptor_heartbeater.go:240: SendLocked()
github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_coord_sender.go:513: Send()
github.com/cockroachdb/cockroach/pkg/kv/db.go:831: sendUsingSender()
github.com/cockroachdb/cockroach/pkg/kv/txn.go:982: Send()
github.com/cockroachdb/cockroach/pkg/sql/row/kv_batch_fetcher.go:224: func1()
github.com/cockroachdb/cockroach/pkg/sql/row/kv_batch_fetcher.go:443: fetch()
github.com/cockroachdb/cockroach/pkg/sql/row/kv_batch_fetcher.go:589: nextBatch()
github.com/cockroachdb/cockroach/pkg/sql/row/kv_fetcher.go:147: NextKV()
github.com/cockroachdb/cockroach/pkg/sql/colfetcher/cfetcher.go:839: nextBatch()
github.com/cockroachdb/cockroach/pkg/sql/colfetcher/cfetcher.go:798: nextAdapter()
github.com/cockroachdb/cockroach/pkg/sql/colmem/allocator.go:299: PerformOperation()
github.com/cockroachdb/cockroach/pkg/sql/colfetcher/cfetcher.go:790: NextBatch()
github.com/cockroachdb/cockroach/pkg/sql/colfetcher/colbatch_scan.go:97: Next()
github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecutils/cancel_checker.go:59: Next()
github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecbase/simple_project.go:125: Next()
github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:239: nextAdapter()
github.com/cockroachdb/cockroach/pkg/sql/colexecerror/error.go:91: CatchVectorizedRuntimeError()
github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:243: next()
github.com/cockroachdb/cockroach/pkg/sql/colflow/flow_coordinator.go:282: Run()
github.com/cockroachdb/cockroach/pkg/sql/colflow/vectorized_flow.go:257: Run()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:525: Run()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:1262: PlanAndRun()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1110: execWithDistSQLEngine()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:972: dispatchToExecutionEngine()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:690: execStmtInOpenState()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:126: execStmt()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1625: func1()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1627: execCmd()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1549: run()

Jira issue: CRDB-8932

[nvanbenschoten]

#68426 provides enough of a backstop for this so that it is no longer high severity.

[michae2]

@nvanbenschoten We don't fully understand. Should we be disallowing prepared queries with AOST?

[nvanbenschoten]

I don't think we should be disallowing prepared queries with AOST, but I also don't think I've fully wrapped my head around why we're setting a fixed timestamp on the active transaction when doing so. Is this because we use the transaction during planning of the prepared statement, and we need to plan the query with the correct schema immediately at prepare time, instead of at execution time?

Regardless of the reason, I think we need some kind of check like we have for query execution that prevents inconsistent AOST timestamps in explicit transactions:

cockroach/pkg/sql/conn_executor_exec.go

Lines 614 to 619 in b365f26

	if readTs := ex.state.getReadTimestamp(); asOf.Timestamp != readTs {
	err = pgerror.Newf(pgcode.Syntax,
	"inconsistent AS OF SYSTEM TIME timestamp; expected: %s", readTs)
	err = errors.WithHint(err, "try SET TRANSACTION AS OF SYSTEM TIME")
	return makeErrEvent(err)
	}

As of #68426, we have that check, but it's in the form of an internal error.

[RaduBerinde]

I'm not sure why we set it. Perhaps indeed the idea was that we want to use the schema as of that time (though we should be re-resolving all datasources so it's not a correctness issue). There is also the somewhat related issue of allowing the AOST value to be a placeholder #30955

[ajwerner]

Indeed the idea is to resolve the schema as of that timestamp. I think maybe what we'd want to do is set it explicitly if that timestamp is an exact value and not set it to anything if the timestamp is one of the allowed function calls.

[rafiss]

I can't reproduce. I'm pretty sure 9fdb39b fixes this.