address the comment

Signed-off-by: yisaer <[email protected]> address the comment Signed-off-by: yisaer <[email protected]> address the comment Signed-off-by: yisaer <[email protected]>
pingcap · May 25, 2021 · ce2df75 · ce2df75
1 parent b0fe8e2
commit ce2df75
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 18 deletions.
diff --git a/executor/simple.go b/executor/simple.go
@@ -566,17 +566,6 @@ func (e *SimpleExec) executeUse(s *ast.UseStmt) error {
 }
 
 func (e *SimpleExec) executeBegin(ctx context.Context, s *ast.BeginStmt) error {
-	// When TxnReadTS is not 0, it indicates the transaction is staleness transaction
-	if e.ctx.GetSessionVars().TxnReadTS > 0 {
-		startTS := e.ctx.GetSessionVars().TxnReadTS
-		// clear TxnReadTS after we used it.
-		e.ctx.GetSessionVars().TxnReadTS = 0
-		if err := e.ctx.NewTxnWithStartTS(ctx, startTS); err != nil {
-			return err
-		}
-		e.ctx.GetSessionVars().SetInTxn(true)
-		return nil
-	}
 	// If `START TRANSACTION READ ONLY` is the first statement in TxnCtx, we should
 	// always create a new Txn instead of reusing it.
 	if s.ReadOnly {
@@ -585,6 +574,10 @@ func (e *SimpleExec) executeBegin(ctx context.Context, s *ast.BeginStmt) error {
 			return expression.ErrFunctionsNoopImpl.GenWithStackByArgs("READ ONLY")
 		}
 		if s.AsOf != nil {
+			// start transaction read only as of failed due to we set tx_read_ts before
+			if e.ctx.GetSessionVars().TxnReadTS > 0 {
+				return errors.New("start transaction read only as of is forbidden after set transaction read only as of")
+			}
 			if err := e.ctx.NewTxnWithStartTS(ctx, e.staleTxnStartTS); err != nil {
 				return err
 			}
@@ -595,18 +588,29 @@ func (e *SimpleExec) executeBegin(ctx context.Context, s *ast.BeginStmt) error {
 			return nil
 		}
 	}
+	// When TxnReadTS is not 0, it indicates the transaction is staleness transaction
+	if e.ctx.GetSessionVars().TxnReadTS > 0 {
+		startTS := e.ctx.GetSessionVars().TxnReadTS
+		// clear TxnReadTS after we used it.
+		e.ctx.GetSessionVars().TxnReadTS = 0
+		if err := e.ctx.NewTxnWithStartTS(ctx, startTS); err != nil {
+			return err
+		}
+		e.ctx.GetSessionVars().SetInTxn(true)
+		return nil
+	}
 
 	// If BEGIN is the first statement in TxnCtx, we can reuse the existing transaction, without the
 	// need to call NewTxn, which commits the existing transaction and begins a new one.
 	// If the last un-committed/un-rollback transaction is a time-bounded read-only transaction, we should
-	// always create a new transaction.
 	txnCtx := e.ctx.GetSessionVars().TxnCtx
 	if txnCtx.History != nil || txnCtx.IsStaleness {
 		err := e.ctx.NewTxn(ctx)
 		if err != nil {
 			return err
 		}
-	}
+	} // always create a new transaction.
+
 	// With START TRANSACTION, autocommit remains disabled until you end
 	// the transaction with COMMIT or ROLLBACK. The autocommit mode then
 	// reverts to its previous state.

diff --git a/executor/stale_txn_test.go b/executor/stale_txn_test.go
@@ -284,4 +284,9 @@ func (s *testStaleTxnSerialSuite) TestSetTransactionReadOnlyAsOf(c *C) {
 	err = tk.ExecToErr(`SET TRANSACTION READ ONLY as of timestamp tidb_bounded_staleness(invalid1, invalid2')`)
 	c.Assert(err, NotNil)
 	c.Assert(tk.Se.GetSessionVars().TxnReadTS, Equals, uint64(0))
+
+	tk.MustExec(`SET TRANSACTION READ ONLY as of timestamp '2021-04-21 00:42:12'`)
+	err = tk.ExecToErr(`START TRANSACTION READ ONLY AS OF TIMESTAMP '2020-09-06 00:00:00'`)
+	c.Assert(err, NotNil)
+	c.Assert(err, Equals, "start transaction read only as of is forbidden after set transaction read only as of")
 }
diff --git a/sessionctx/variable/sysvar.go b/sessionctx/variable/sysvar.go
@@ -826,11 +826,7 @@ var defaultSysVars = []*SysVar{
 		return s.TxnScope.GetVarValue(), nil
 	}},
 	{Scope: ScopeSession, Name: TiDBTxnReadTS, Value: "", SetSession: func(s *SessionVars, val string) error {
-		err := setTxnReadTS(s, val)
-		if err != nil {
-			return err
-		}
-		return nil
+		return setTxnReadTS(s, val)
 	}},
 	{Scope: ScopeGlobal | ScopeSession, Name: TiDBAllowMPPExecution, Value: On, Type: TypeEnum, PossibleValues: []string{"OFF", "ON", "ENFORCE"}, SetSession: func(s *SessionVars, val string) error {
 		s.allowMPPExecution = val