kv: alter error for SET TRANSACTION AS OF SYSTEM TIME #79090
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @rafiss)
pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 1085 at r1 (raw file):
// The transaction must not have already been used in this epoch. if !tc.interceptorAlloc.txnSpanRefresher.refreshFootprint.empty() { return errors.WithContextTags(errors.Newf(
hm after thinking about it more, i think maybe we want to keep the assertion error here, since there are other code paths to this logic that still should be treated as programming errors
i wonder if there's a way to make pkg/sql/set_transaction.go smart enough to detect when it's allowed to run. that is, can we return a friendly user-error before calling setTransactionModes? @ajwerner do you know if that's feasible?
also, please add tests in pkg/sql/logictest/testdata/logic_test/as_of. (use the repro in #77265 (comment) as inspiration)
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 1085 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
hm after thinking about it more, i think maybe we want to keep the assertion error here, since there are other code paths to this logic that still should be treated as programming errors
i wonder if there's a way to make pkg/sql/set_transaction.go smart enough to detect when it's allowed to run. that is, can we return a friendly user-error before calling
setTransactionModes? @ajwerner do you know if that's feasible?also, please add tests in pkg/sql/logictest/testdata/logic_test/as_of. (use the repro in #77265 (comment) as inspiration)
we should be able to add a way to check for it. in txn_state.go we can add methods for txnState like hasPerformedReads and hasPerformedWrites and then have that delegate to the TxnCoordSender. the new functions in TxnCoordSender can do the same check that exists here.
e-mbrown
force-pushed
the
eb/txnreaderr
branch
4 times, most recently
from
1f9db0e to
3f90e37
Compare
There was a problem hiding this comment.
looks like the right direction!
Reviewable status:
pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 152 at r3 (raw file):
// hasPerformedWrites is set true when a write has been performed. hasPerformedWrites bool
you shouldn't need either of these booleans - it looks like they are never used
however, you do need to add the functions (HasPerformedReads and HasPerformedWrites) to the TxnSender interface in pkg/kv/sender.go
pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 1414 at r3 (raw file):
// HasPerformedReads is part of the TxnSender interface. func (tc *TxnCoordSender) HasPerformedReads() bool {
at the beginning of the function, can you add
tc.mu.Lock()
defer tc.mu.Unlock()
(like how SetFixedTimestamp has). this acquires a lock, which is needed to prevent multiple goroutines from interacting with the TxnCoordSender at the same time
pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 1423 at r3 (raw file):
// HasPerformedWrites is part of the TxnSender interface. func (tc *TxnCoordSender) HasPerformedWrites() bool { if tc.mu.txn.Sequence != 0 {
tc.mu.Lock()
defer tc.mu.Unlock()
here as well
pkg/sql/txn_state.go, line 294 at r3 (raw file):
defer ts.mu.Unlock() if sender, ok := ts.mu.txn.Sender().(*kvcoord.TxnCoordSender); ok {
you shouldn't need the (*kvcoord.TxnCoordSender) type assertion
pkg/sql/logictest/testdata/logic_test/as_of, line 118 at r3 (raw file):
SELECT * from t statement error cannot set fixed timestamp, .* already performed reads or writes
the error message should just be "already performed reads" now
pkg/sql/logictest/testdata/logic_test/as_of, line 119 at r3 (raw file):
statement error cannot set fixed timestamp, .* already performed reads or writes SET TRANSACTION AS OF system time '-1s'
right after this line, add:
statement ok
ROLLBACK
pkg/sql/logictest/testdata/logic_test/as_of, line 127 at r3 (raw file):
INSERT INTO t VALUES(1) statement error cannot set fixed timestamp, .* already performed reads or writes
the error message should just be "already performed writes" now
pkg/sql/logictest/testdata/logic_test/as_of, line 128 at r3 (raw file):
statement error cannot set fixed timestamp, .* already performed reads or writes SET TRANSACTION AS OF system time '-1s'
right after this line, add:
statement ok
ROLLBACK
There was a problem hiding this comment.
Reviewable status:
pkg/kv/sender.go, line 341 at r4 (raw file):
// HasPerformedReads returns true if a read has been performed. HasPerformedReads() bool
nit: these two functions also need to be implemented in pkg/kv/mock_transactional_sender.go. i'm pretty sure you can just make the implementation panic, like how it does for some of the other funcitons:
cockroach/pkg/kv/mock_transactional_sender.go
Line 147 in 1f61d74
e-mbrown
force-pushed
the
eb/txnreaderr
branch
2 times, most recently
from
416957d to
a5aab2e
Compare
There was a problem hiding this comment.
There are two lint issues:
pkg/kv/kvclient/kvcoord/txn_coord_sender.go:1411:2: should use 'return !tc.interceptorAlloc.txnSpanRefresher.refreshFootprint.empty()' instead of 'if !tc.interceptorAlloc.txnSpanRefresher.refreshFootprint.empty() { return true }; return false' (S1008)
And make sure to run crlfmt so that import ordering is correct:
diff -u old/sql/txn_state.go new/sql/txn_state.go
--- old/sql/txn_state.go 2022-04-21 20:47:42.634886654 +0000
+++ new/sql/txn_state.go 2022-04-21 20:47:42.634886654 +0000
@@ -12,13 +12,13 @@
import (
"context"
- "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
- "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"time"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+ "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+ "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
161673d to
0d8fb26
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go line 2595 at r8 (raw file):
"cannot set fixed timestamp, txn %s already performed writes", ex.state.mu.txn) }
could you move all of this to a new function in txn_state.go instead?
it would be something like
// checkReadsAndWrites returns an error if the transaction has performed reads or writes.
func (ts *txnState) checkReadsAndWrites(ctx context.Context) error {
ts.mu.Lock()
defer ts.mu.Unlock()
if ts.mu.txn.Sender().HasPerformedReads() {
return pgerror.Newf(
pgcode.InvalidTransactionState,
"cannot set fixed timestamp, txn %s already performed reads",
ex.state.mu.txn)
}
if ts.mu.txn.Sender().HasPerformedWrites() {
return pgerror.Newf(
pgcode.InvalidTransactionState,
"cannot set fixed timestamp, txn %s already performed writes",
ex.state.mu.txn)
}
return nil
}
There was a problem hiding this comment.
looks close! just had a small nit. also, you will need to rebase to address the test flake
Reviewable status:
pkg/sql/txn_state.go line 474 at r9 (raw file):
// checkReadsAndWrites returns an error if the transaction has performed reads // or writes. func (ts *txnState) checkReadsAndWrites() error {
a the beginning of this function, can you add
ts.mu.Lock()
defer ts.mu.Unlock()
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1086 at r10 (raw file):
tc.mu.Lock() defer tc.mu.Unlock() // The transaction must not have already been used in this epoch.
i'm realizing another thing -- we will want to make sure the check here is always identical to the new function that you added. so we should avoid code duplication
to do that, we can make new functions like this:
func (tc *TxnCoordSender) hasPerformedReadsLocked() bool {
return !tc.interceptorAlloc.txnSpanRefresher.refreshFootprint.empty()
}
func (tc *TxnCoordSender) hasPerformedWritesLocked() bool {
return tc.mu.txn.Sequence != 0
}
The "locked" in the name is a convention that indicates that you can only call this if you have acquired the lock on tc.
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1087 at r10 (raw file):
defer tc.mu.Unlock() // The transaction must not have already been used in this epoch. if !tc.interceptorAlloc.txnSpanRefresher.refreshFootprint.empty() {
then replace this with if tc.hasPerformedReadsLocked() {
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1091 at r10 (raw file):
"cannot set fixed timestamp, txn %s already performed reads", tc.mu.txn), ctx) } if tc.mu.txn.Sequence != 0 {
and replace this with if tc.hasPerformedWritesLocked() {
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1414 at r10 (raw file):
tc.mu.Lock() defer tc.mu.Unlock() return !tc.interceptorAlloc.txnSpanRefresher.refreshFootprint.empty()
and replace this with return tc.hasPerformedReadsLocked() {
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1421 at r10 (raw file):
tc.mu.Lock() defer tc.mu.Unlock() return tc.mu.txn.Sequence != 0
and replace this with if tc.hasPerformedWritesLocked() {
e-mbrown
force-pushed
the
eb/txnreaderr
branch
2 times, most recently
from
ac81058 to
cf03833
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1425 at r11 (raw file):
// HasPerformedReadsLocked is part of the TxnSender interface. func (tc *TxnCoordSender) HasPerformedReadsLocked() bool {
nit: make this lower case (so that the function is not visible outside of this package). also, remove the comment
pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1430 at r11 (raw file):
// HasPerformedWritesLocked is part of the TxnSender interface. func (tc *TxnCoordSender) HasPerformedWritesLocked() bool {
nit: make this lower case (so that the function is not visible outside of this package). also, remove the comment
There was a problem hiding this comment.
Reviewed 2 of 4 files at r4, 1 of 1 files at r5, 1 of 3 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status:
if reads or writes are already performed When a txn performed a read or write before setting up a historical timestamp a crash report was generated. This commit handles the error case. Release note: None
There was a problem hiding this comment.
lgtm!
Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewed 2 of 4 files at r4, 1 of 1 files at r5, 1 of 1 files at r10, 1 of 1 files at r12, all commit messages.
Reviewable status:
pkg/kv/sender.go line 340 at r12 (raw file):
ClearTxnRetryableErr(ctx context.Context) // HasPerformedReads returns true if a read has been performed.
Let's add "in the transaction's current epoch" at the end of both of these comments.
|
TFTR! bors r+ |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 40151de to blathers/backport-release-21.2-79090: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 21.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
if reads or writes are already performed
Resolves #77265
When a txn performed a read or write before setting up a historical timestamp a crash report was generated. This commit handles the error case.
Release note: None