kv: assert txn unused in SetFixedTimestamp#68426
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Aug 5, 2021
Merged
kv: assert txn unused in SetFixedTimestamp#68426craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
knz
approved these changes
Aug 4, 2021
Contributor
knz
left a comment
There was a problem hiding this comment.
Reviewed 31 of 31 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks, @jordanlewis, @nvanbenschoten, and @shermanCRL)
pkg/sql/txn_state.go, line 272 at r1 (raw file):
ts.mu.Lock() defer ts.mu.Unlock() ts.isHistorical = true
do you want to set isHistorical even if there's an error returned?
This commit asserts that a transaction has not been used to read or to write by the time that `SetFixedTimestamp` is called on it. This was extracted from cockroachdb#68194 and modified to return an error from `SetFixedTimestamp` on misuse instead of fatal-ing. This provides a sufficient, temporary backstop for cockroachdb#68216 until the conn executor logic is fixed: ``` root@127.0.0.1:26257/movr> create table t (x int); CREATE TABLE root@127.0.0.1:26257/movr> insert into t values (1); INSERT 1 root@127.0.0.1:26257/movr> select crdb_internal_mvcc_timestamp, * from t; crdb_internal_mvcc_timestamp | x ---------------------------------+---- 1628094563935439000.0000000000 | 1 (1 row) root@127.0.0.1:26257/movr> begin as of system time (1628094563935439000.0000000000-1)::string; BEGIN root@127.0.0.1:26257/movr OPEN> select * from t; x ----- (0 rows) root@127.0.0.1:26257/movr OPEN> prepare y as select * from t as of system time 1628094563935439000.0000000000; ERROR: internal error: cannot set fixed timestamp, txn "sql txn" meta={id=e5e81c19 pri=0.01517572 epo=0 ts=1628094563.935438999,0 min=1628094563.935438999,0 seq=0} lock=false stat=PENDING rts=1628094563.935438999,0 wto=false gul=1628094563.935438999,0 already performed reads SQLSTATE: XX000 DETAIL: stack trace: github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord/txn_coord_sender.go:1016: SetFixedTimestamp() github.com/cockroachdb/cockroach/pkg/kv/txn.go:1200: SetFixedTimestamp() github.com/cockroachdb/cockroach/pkg/sql/conn_executor_prepare.go:278: populatePrepared() github.com/cockroachdb/cockroach/pkg/sql/conn_executor_prepare.go:220: func1() github.com/cockroachdb/cockroach/pkg/sql/conn_executor_prepare.go:226: prepare() github.com/cockroachdb/cockroach/pkg/sql/conn_executor_prepare.go:112: addPreparedStmt() github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:570: execStmtInOpenState() github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:126: execStmt() github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1626: func1() github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1628: execCmd() github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1550: run() github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:627: ServeConn() github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:645: func1() runtime/asm_amd64.s:1371: goexit() HINT: You have encountered an unexpected error. Please check the public issue tracker to check whether this problem is already tracked. If you cannot find it there, please report the error with details by creating a new issue. If you would rather not post publicly, please contact us directly using the support form. We appreciate your feedback. root@127.0.0.1:26257/? ERROR> ```
75cbc29 to
b761eba
Compare
nvb
commented
Aug 4, 2021
Contributor
Author
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/txn_state.go, line 272 at r1 (raw file):
Previously, knz (kena) wrote…
do you want to set isHistorical even if there's an error returned?
Good point, done.
jordanlewis
reviewed
Aug 5, 2021
Member
jordanlewis
left a comment
There was a problem hiding this comment.
Reviewable status:
Contributor
Author
|
TFTRs! bors r+ |
Contributor
|
Build succeeded: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit asserts that a transaction has not been used to read or to
write by the time that
SetFixedTimestampis called on it.This was extracted from #68194 and modified to return an error from
SetFixedTimestampon misuse instead of fatal-ing. This provides asufficient, temporary backstop for #68216 until the conn executor logic
is fixed: