From 7d76bdcce8aefbf3d1056748303b77529e16f04d Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 9 Dec 2019 05:06:47 +0100 Subject: [PATCH 1/2] sql: separate metrics for restart and non-restart savepoints Release note (sql change): CockroachDB now collects separate sets of metrics for usage of SAVEPOINT: one set for regular SQL savepoints and one set for uses dedicated to CockroachDB's client-side transaction retry protocol. --- pkg/sql/conn_executor.go | 38 +++++++++++++++++++++++++-------- pkg/sql/exec_util.go | 24 +++++++++++++++++++++ pkg/ts/catalog/chart_catalog.go | 11 +++++++++- 3 files changed, 63 insertions(+), 10 deletions(-) diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index 8f183d5fbbee..c871c073a437 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -2298,11 +2298,12 @@ type StatementCounters struct { TxnCommitCount telemetry.CounterWithMetric TxnRollbackCount telemetry.CounterWithMetric - // Savepoint operations. SavepointCount is for real SQL savepoints - // (which we don't yet support; this is just a placeholder for - // telemetry); the RestartSavepoint variants are for the + // Savepoint operations. SavepointCount is for real SQL savepoints; + // the RestartSavepoint variants are for the // cockroach-specific client-side retry protocol. SavepointCount telemetry.CounterWithMetric + ReleaseSavepointCount telemetry.CounterWithMetric + RollbackToSavepointCount telemetry.CounterWithMetric RestartSavepointCount telemetry.CounterWithMetric ReleaseRestartSavepointCount telemetry.CounterWithMetric RollbackToRestartSavepointCount telemetry.CounterWithMetric @@ -2322,14 +2323,18 @@ func makeStartedStatementCounters(internal bool) StatementCounters { getMetricMeta(MetaTxnCommitStarted, internal)), TxnRollbackCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaTxnRollbackStarted, internal)), - SavepointCount: telemetry.NewCounterWithMetric( - getMetricMeta(MetaSavepointStarted, internal)), RestartSavepointCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaRestartSavepointStarted, internal)), ReleaseRestartSavepointCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaReleaseRestartSavepointStarted, internal)), RollbackToRestartSavepointCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaRollbackToRestartSavepointStarted, internal)), + SavepointCount: telemetry.NewCounterWithMetric( + getMetricMeta(MetaSavepointStarted, internal)), + ReleaseSavepointCount: telemetry.NewCounterWithMetric( + getMetricMeta(MetaReleaseSavepointStarted, internal)), + RollbackToSavepointCount: telemetry.NewCounterWithMetric( + getMetricMeta(MetaRollbackToSavepointStarted, internal)), SelectCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaSelectStarted, internal)), UpdateCount: telemetry.NewCounterWithMetric( @@ -2355,14 +2360,18 @@ func makeExecutedStatementCounters(internal bool) StatementCounters { getMetricMeta(MetaTxnCommitExecuted, internal)), TxnRollbackCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaTxnRollbackExecuted, internal)), - SavepointCount: telemetry.NewCounterWithMetric( - getMetricMeta(MetaSavepointExecuted, internal)), RestartSavepointCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaRestartSavepointExecuted, internal)), ReleaseRestartSavepointCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaReleaseRestartSavepointExecuted, internal)), RollbackToRestartSavepointCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaRollbackToRestartSavepointExecuted, internal)), + SavepointCount: telemetry.NewCounterWithMetric( + getMetricMeta(MetaSavepointExecuted, internal)), + ReleaseSavepointCount: telemetry.NewCounterWithMetric( + getMetricMeta(MetaReleaseSavepointExecuted, internal)), + RollbackToSavepointCount: telemetry.NewCounterWithMetric( + getMetricMeta(MetaRollbackToSavepointExecuted, internal)), SelectCount: telemetry.NewCounterWithMetric( getMetricMeta(MetaSelectExecuted, internal)), UpdateCount: telemetry.NewCounterWithMetric( @@ -2398,15 +2407,26 @@ func (sc *StatementCounters) incrementCount(ex *connExecutor, stmt tree.Statemen case *tree.RollbackTransaction: sc.TxnRollbackCount.Inc() case *tree.Savepoint: + // TODO(knz): Sanitize this. if err := ex.validateSavepointName(t.Name); err == nil { sc.RestartSavepointCount.Inc() } else { sc.SavepointCount.Inc() } case *tree.ReleaseSavepoint: - sc.ReleaseRestartSavepointCount.Inc() + // TODO(knz): Sanitize this. + if err := ex.validateSavepointName(t.Savepoint); err == nil { + sc.ReleaseRestartSavepointCount.Inc() + } else { + sc.ReleaseSavepointCount.Inc() + } case *tree.RollbackToSavepoint: - sc.RollbackToRestartSavepointCount.Inc() + // TODO(knz): Sanitize this. + if err := ex.validateSavepointName(t.Savepoint); err == nil { + sc.RollbackToRestartSavepointCount.Inc() + } else { + sc.RollbackToSavepointCount.Inc() + } default: if tree.CanModifySchema(stmt) { sc.DdlCount.Inc() diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 7812c02b3481..8b866d0e3125 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -380,6 +380,18 @@ var ( Measurement: "SQL Statements", Unit: metric.Unit_COUNT, } + MetaReleaseSavepointStarted = metric.Metadata{ + Name: "sql.savepoint.release.started.count", + Help: "Number of `RELEASE SAVEPOINT` statements started", + Measurement: "SQL Statements", + Unit: metric.Unit_COUNT, + } + MetaRollbackToSavepointStarted = metric.Metadata{ + Name: "sql.savepoint.rollback.started.count", + Help: "Number of `ROLLBACK TO SAVEPOINT` statements started", + Measurement: "SQL Statements", + Unit: metric.Unit_COUNT, + } MetaRestartSavepointStarted = metric.Metadata{ Name: "sql.restart_savepoint.started.count", Help: "Number of `SAVEPOINT cockroach_restart` statements started", @@ -466,6 +478,18 @@ var ( Measurement: "SQL Statements", Unit: metric.Unit_COUNT, } + MetaReleaseSavepointExecuted = metric.Metadata{ + Name: "sql.savepoint.release.count", + Help: "Number of `RELEASE SAVEPOINT` statements successfully executed", + Measurement: "SQL Statements", + Unit: metric.Unit_COUNT, + } + MetaRollbackToSavepointExecuted = metric.Metadata{ + Name: "sql.savepoint.rollback.count", + Help: "Number of `ROLLBACK TO SAVEPOINT` statements successfully executed", + Measurement: "SQL Statements", + Unit: metric.Unit_COUNT, + } MetaRestartSavepointExecuted = metric.Metadata{ Name: "sql.restart_savepoint.count", Help: "Number of `SAVEPOINT cockroach_restart` statements successfully executed", diff --git a/pkg/ts/catalog/chart_catalog.go b/pkg/ts/catalog/chart_catalog.go index ff2e247e486b..27e5db8c6b0c 100644 --- a/pkg/ts/catalog/chart_catalog.go +++ b/pkg/ts/catalog/chart_catalog.go @@ -711,7 +711,8 @@ var charts = []sectionDescription{ Title: "Count", Metrics: []string{ "sql.savepoint.count", - "sql.savepoint.count.internal", + "sql.savepoint.release.count", + "sql.savepoint.rollback.count", }, AxisLabel: "SQL Statements", }, @@ -1714,6 +1715,14 @@ var charts = []sectionDescription{ "sql.savepoint.count.internal", "sql.savepoint.started.count", "sql.savepoint.started.count.internal", + "sql.savepoint.rollback.count", + "sql.savepoint.rollback.count.internal", + "sql.savepoint.rollback.started.count", + "sql.savepoint.rollback.started.count.internal", + "sql.savepoint.release.count", + "sql.savepoint.release.count.internal", + "sql.savepoint.release.started.count", + "sql.savepoint.release.started.count.internal", }, AxisLabel: "SQL Statements", }, From 69533b32e82e6e5ab8ebe9530bf49587e7078f56 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 10 Dec 2019 18:20:03 +0100 Subject: [PATCH 2/2] sql: preliminary executor logic for savepoints This commit introduces support for regular SQL savepoints in the executor. The following is supported: - recognizing the SAVEPOINT / ROLLBACK TO SAVEPOINT / RELEASE SAVEPOINT statements even when the savepoint name is not "cockroach_restart". - maintaining a list of current active savepoints, and detect when SAVEPOINT and RELEASE/ROLLBACK are mismatched. - issue the `GetSavepoint()` / `RollbackToSavepoint()` / `ReleaseSavepoint()` API calls to the KV layers. - enforce that `cockroarch_restart` (and retry savepoints in general) are not started "under" a regular savepoint. What is not supported yet: - Rejecting ROLLBACK after a DDL statement has executed. - ROLLBACK when the txn is aborted (i.e. after an error). Release note (sql change): CockroachDB now provides rudimentary (incomplete) and experimental support for SQL savepoints. Release note (sql change): CockroachDB supports a new statement `SHOW SAVEPOINT STATUS` which reveals the current stack of active savepoints. This can be used to teach savepoint usage. --- pkg/sql/conn_executor.go | 13 +- pkg/sql/conn_executor_exec.go | 18 + pkg/sql/conn_executor_savepoints.go | 382 +++++++++---- pkg/sql/conn_executor_savepoints_test.go | 194 +++++++ .../testdata/logic_test/manual_retry | 31 +- pkg/sql/logictest/testdata/logic_test/txn | 148 ----- pkg/sql/metric_test.go | 4 +- pkg/sql/opt/exec/execbuilder/builder.go | 5 + pkg/sql/opt/exec/execbuilder/relational.go | 14 +- pkg/sql/parser/help_test.go | 1 + pkg/sql/parser/parse_test.go | 2 + pkg/sql/parser/sql.y | 13 + pkg/sql/plan.go | 3 + pkg/sql/plan_opt.go | 6 +- pkg/sql/sem/tree/show.go | 9 + pkg/sql/sem/tree/stmt.go | 9 + pkg/sql/testdata/savepoints | 519 ++++++++++++++++++ pkg/sql/txn_restart_test.go | 6 - pkg/sql/txn_state.go | 6 +- 19 files changed, 1088 insertions(+), 295 deletions(-) create mode 100644 pkg/sql/conn_executor_savepoints_test.go create mode 100644 pkg/sql/testdata/savepoints diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index c871c073a437..bb82e089e414 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -919,6 +919,10 @@ type connExecutor struct { // stateOpen. autoRetryCounter int + // numDDL keeps track of how many DDL statements have been + // executed so far. + numDDL int + // txnRewindPos is the position within stmtBuf to which we'll rewind when // performing automatic retries. This is more or less the position where the // current transaction started. @@ -2407,22 +2411,19 @@ func (sc *StatementCounters) incrementCount(ex *connExecutor, stmt tree.Statemen case *tree.RollbackTransaction: sc.TxnRollbackCount.Inc() case *tree.Savepoint: - // TODO(knz): Sanitize this. - if err := ex.validateSavepointName(t.Name); err == nil { + if ex.isRestartSavepoint(t.Name) { sc.RestartSavepointCount.Inc() } else { sc.SavepointCount.Inc() } case *tree.ReleaseSavepoint: - // TODO(knz): Sanitize this. - if err := ex.validateSavepointName(t.Savepoint); err == nil { + if ex.isRestartSavepoint(t.Savepoint) { sc.ReleaseRestartSavepointCount.Inc() } else { sc.ReleaseSavepointCount.Inc() } case *tree.RollbackToSavepoint: - // TODO(knz): Sanitize this. - if err := ex.validateSavepointName(t.Savepoint); err == nil { + if ex.isRestartSavepoint(t.Savepoint) { sc.RollbackToRestartSavepointCount.Inc() } else { sc.RollbackToSavepointCount.Inc() diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index 1a962868e4e8..feb5f55a7506 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -573,6 +573,15 @@ func (ex *connExecutor) commitSQLTransactionInternal( ) (ev fsm.Event, payload fsm.EventPayload, ok bool) { ex.clearSavepoints() + return ex.finalizeCommitTransactionInternal(ctx, stmt) +} + +// finalizeCommitTrransaction is like commitSQLTransactionInternal but +// preserves the savepoint structure. Used by ROLLBACK TO SAVEPOINT +// cockroach_restart. +func (ex *connExecutor) finalizeCommitTransactionInternal( + ctx context.Context, stmt tree.Statement, +) (ev fsm.Event, payload fsm.EventPayload, ok bool) { if err := ex.checkTableTwoVersionInvariant(ctx); err != nil { ev, payload = ex.makeErrEvent(err, stmt) return ev, payload, false @@ -743,6 +752,13 @@ func (ex *connExecutor) makeExecPlan(ctx context.Context, planner *planner) erro log.VEventf(ctx, 1, "optimizer plan failed: %v", err) return err } + + // TODO(knz): Remove this accounting if/when savepoint rollbacks + // support rolling back over DDL. + if planner.curPlan.flags.IsSet(planFlagIsDDL) { + ex.extraTxnState.numDDL++ + } + return nil } @@ -1018,6 +1034,8 @@ func (ex *connExecutor) runObserverStatement( switch sqlStmt := stmt.AST.(type) { case *tree.ShowTransactionStatus: return ex.runShowTransactionState(ctx, res) + case *tree.ShowSavepointStatus: + return ex.runShowSavepointState(ctx, res) case *tree.ShowSyntax: return ex.runShowSyntax(ctx, sqlStmt.Statement, res) case *tree.SetTracing: diff --git a/pkg/sql/conn_executor_savepoints.go b/pkg/sql/conn_executor_savepoints.go index 8d22d2cc733a..56abb38c99bc 100644 --- a/pkg/sql/conn_executor_savepoints.go +++ b/pkg/sql/conn_executor_savepoints.go @@ -14,9 +14,12 @@ import ( "context" "strings" + "github.com/cockroachdb/cockroach/pkg/internal/client" "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/sqlbase" + "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" "github.com/cockroachdb/cockroach/pkg/util/fsm" ) @@ -26,33 +29,59 @@ import ( func (ex *connExecutor) execSavepointInOpenState( ctx context.Context, s *tree.Savepoint, res RestrictedCommandResult, ) (retEv fsm.Event, retPayload fsm.EventPayload, retErr error) { - // Ensure that the user isn't trying to run BEGIN; SAVEPOINT; SAVEPOINT; - if ex.state.activeRestartSavepointName != "" { - err := unimplemented.NewWithIssueDetail(10735, "nested", "SAVEPOINT may not be nested") - ev, payload := ex.makeErrEvent(err, s) - return ev, payload, nil - } - if err := ex.validateSavepointName(s.Name); err != nil { - ev, payload := ex.makeErrEvent(err, s) - return ev, payload, nil + entry := savepointEntry{ + name: s.Name, + isRestartSavepoint: ex.isRestartSavepoint(s.Name), } - // We want to disallow SAVEPOINTs to be issued after a KV transaction has - // started running. The client txn's statement count indicates how many - // statements have been executed as part of this transaction. It is - // desirable to allow metadata queries against vtables to proceed - // before starting a SAVEPOINT for better ORM compatibility. - // See also: - // https://github.com/cockroachdb/cockroach/issues/15012 - if ex.state.mu.txn.Active() { - err := pgerror.Newf(pgcode.Syntax, - "SAVEPOINT %s needs to be the first statement in a transaction", restartSavepointName) - ev, payload := ex.makeErrEvent(err, s) - return ev, payload, nil + + if entry.isRestartSavepoint { + /* SAVEPOINT cockroach_restart */ + + if !ex.state.savepointEnv.isEmpty() { + err := pgerror.Newf(pgcode.Syntax, + "SAVEPOINT %s cannot be nested", + tree.ErrNameString(restartSavepointName)) + ev, payload := ex.makeErrEvent(err, s) + return ev, payload, nil + } + // We want to disallow restart SAVEPOINTs to be issued after a KV + // transaction has started running. The client txn's statement count + // indicates how many statements have been executed as part of this + // transaction. It is desirable to allow metadata queries against + // vtables to proceed before starting a SAVEPOINT for better ORM + // compatibility. + // + // See also: + // https://github.com/cockroachdb/cockroach/issues/15012 + if ex.state.mu.txn.Active() { + err := pgerror.Newf(pgcode.Syntax, + "SAVEPOINT %s needs to be the first statement in a transaction", + tree.ErrNameString(restartSavepointName)) + ev, payload := ex.makeErrEvent(err, s) + return ev, payload, nil + } + + retEv = eventRetryIntentSet{} + } else { + /* other savepoints */ + + token, err := ex.state.mu.txn.CreateSavepoint(ctx) + if err != nil { + ev, payload := ex.makeErrEvent(err, s) + return ev, payload, nil + } + entry.kvToken = token + // Nothing special - also no special event for the txn. + retEv = nil } - ex.state.activeRestartSavepointName = s.Name - // Note that Savepoint doesn't have a corresponding plan node. - // This here is all the execution there is. - return eventRetryIntentSet{}, nil /* payload */, nil + + // Remember how many DDL statements were created so far. We'll need + // this information if the savepoint is ever rolled back. + entry.numDDL = ex.extraTxnState.numDDL + + ex.state.savepointEnv.createSavepoint(entry) + + return retEv, nil, nil } // execReleaseSavepointInOpenState runs a RELEASE SAVEPOINT statement @@ -60,19 +89,52 @@ func (ex *connExecutor) execSavepointInOpenState( func (ex *connExecutor) execReleaseSavepointInOpenState( ctx context.Context, s *tree.ReleaseSavepoint, res RestrictedCommandResult, ) (retEv fsm.Event, retPayload fsm.EventPayload, retErr error) { - if err := ex.validateSavepointName(s.Savepoint); err != nil { - ev, payload := ex.makeErrEvent(err, s) + entry, idx, ok := ex.state.savepointEnv.findSavepoint(s.Savepoint) + if !ok { + ev, payload := ex.makeErrEvent( + pgerror.Newf(pgcode.InvalidSavepointSpecification, + "savepoint %s does not exist", &s.Savepoint), s) return ev, payload, nil } - if !ex.machine.CurState().(stateOpen).RetryIntent.Get() { - ev, payload := ex.makeErrEvent(errSavepointNotUsed, s) - return ev, payload, nil + + if entry.isRestartSavepoint { + /* RELEASE SAVEPOINT cockroach_restart */ + + if !ex.machine.CurState().(stateOpen).RetryIntent.Get() { + ev, payload := ex.makeErrEvent(errSavepointNotUsed, s) + return ev, payload, nil + } + + // ReleaseSavepoint with a restart savepoint operates as a txn commit. + res.ResetStmtType((*tree.CommitTransaction)(nil)) + var ok bool + retEv, retPayload, ok = ex.finalizeCommitTransactionInternal(ctx, s) + if ok { + retEv = eventTxnReleased{} + retPayload = nil + } else { + // The release failed. All the savepoints are trashed, so we're + // going to unwind the savepoint stack below in any case. + // However, because the release failed it's likely the client + // will want to rewind to the restart savepoint instead. So we + // don't finalize the release for that one and instead keep it, + // operating like a rollback. + idx++ + } + } else { + /* regular savepoint */ + + if err := ex.state.mu.txn.ReleaseSavepoint(ctx, entry.kvToken); err != nil { + ev, payload := ex.makeErrEvent(err, s) + return ev, payload, nil + } } - // ReleaseSavepoint is executed fully here; there's no plan for it. - ev, payload := ex.runReleaseRestartSavepointAsTxnCommit(ctx, s) - res.ResetStmtType((*tree.CommitTransaction)(nil)) - return ev, payload, nil + // RELEASE means the savepoint name is not valid any more. + // We rewind to one position higher. + ex.state.savepointEnv.rewindToSavepoint(idx - 1) + + return retEv, retPayload, nil } // execRollbackToSavepointInOpenState runs a ROLLBACK TO SAVEPOINT @@ -80,144 +142,228 @@ func (ex *connExecutor) execReleaseSavepointInOpenState( func (ex *connExecutor) execRollbackToSavepointInOpenState( ctx context.Context, s *tree.RollbackToSavepoint, res RestrictedCommandResult, ) (retEv fsm.Event, retPayload fsm.EventPayload, retErr error) { - if err := ex.validateSavepointName(s.Savepoint); err != nil { - ev, payload := ex.makeErrEvent(err, s) + entry, idx, ok := ex.state.savepointEnv.findSavepoint(s.Savepoint) + if !ok { + ev, payload := ex.makeErrEvent( + pgerror.Newf(pgcode.InvalidSavepointSpecification, + "savepoint %s does not exist", &s.Savepoint), s) return ev, payload, nil } - if !ex.machine.CurState().(stateOpen).RetryIntent.Get() { - ev, payload := ex.makeErrEvent(errSavepointNotUsed, s) - return ev, payload, nil + + if entry.isRestartSavepoint { + /* ROLLBACK TO SAVEPOINT cockroach_restart */ + + if !ex.machine.CurState().(stateOpen).RetryIntent.Get() { + ev, payload := ex.makeErrEvent(errSavepointNotUsed, s) + return ev, payload, nil + } + + res.ResetStmtType((*tree.Savepoint)(nil)) + retEv = eventTxnRestart{} + } else { + /* regular savepoint */ + + // In the initial implementation, we don't yet support rolling + // back over DDL. Instead of creating an inconsistent txn or + // schema state, prefer to tell the users we don't know how to + // proceed yet. + if ex.extraTxnState.numDDL > entry.numDDL { + ev, payload := ex.makeErrEvent(unimplemented.NewWithIssueDetail(10735, "rollback-after-ddl", + "ROLLBACK TO SAVEPOINT not yet supported after DDL statements"), s) + return ev, payload, nil + } + + if err := ex.state.mu.txn.RollbackToSavepoint(ctx, entry.kvToken); err != nil { + ev, payload := ex.makeErrEvent(err, s) + return ev, payload, nil + } } - ex.state.activeRestartSavepointName = "" - res.ResetStmtType((*tree.Savepoint)(nil)) - return eventTxnRestart{}, nil /* payload */, nil + ex.state.savepointEnv.rewindToSavepoint(idx) + + return retEv, retPayload, nil } // execSavepointInAbortedState runs a SAVEPOINT statement when a txn is aborted. -// It also contains the logic for ROLLBACK TO SAVEPOINT. -// TODO(knz): split this in different functions. func (ex *connExecutor) execSavepointInAbortedState( - ctx context.Context, inRestartWait bool, s tree.Statement, res RestrictedCommandResult, + ctx context.Context, inRestartWait bool, s *tree.Savepoint, res RestrictedCommandResult, ) (fsm.Event, fsm.EventPayload) { - // We accept both the "ROLLBACK TO SAVEPOINT cockroach_restart" and the - // "SAVEPOINT cockroach_restart" commands to indicate client intent to - // retry a transaction in a RestartWait state. - var spName tree.Name - var isRollback bool - switch n := s.(type) { - case *tree.RollbackToSavepoint: - spName = n.Savepoint - isRollback = true - case *tree.Savepoint: - spName = n.Name - default: - panic("unreachable") - } - // If the user issued a SAVEPOINT in the abort state, validate - // as though there were no active savepoint. - if !isRollback { - ex.state.activeRestartSavepointName = "" - } - if err := ex.validateSavepointName(spName); err != nil { + makeErr := func(err error) (fsm.Event, fsm.EventPayload) { ev := eventNonRetriableErr{IsCommit: fsm.False} payload := eventNonRetriableErrPayload{ err: err, } return ev, payload } - // Either clear or reset the current savepoint name so that - // ROLLBACK TO; SAVEPOINT; works. - if isRollback { - ex.state.activeRestartSavepointName = "" - } else { - ex.state.activeRestartSavepointName = spName + + if !ex.isRestartSavepoint(s.Name) { + // Cannot establish a regular savepoint if the txn is currently in + // error. + return makeErr(sqlbase.NewTransactionAbortedError("" /* customMsg */)) } - if !(inRestartWait || ex.machine.CurState().(stateAborted).RetryIntent.Get()) { - ev := eventNonRetriableErr{IsCommit: fsm.False} - payload := eventNonRetriableErrPayload{ - err: errSavepointNotUsed, - } - return ev, payload + // Establishing a restart savepoint in a txn in error has three + // behaviors: + // - if there was no restart savepoint already, this fails and + // informs the client they should have use SAVEPOINT cockroach_restart + // at the beginning. + // - if the txn is not waiting to retry, ditto. + // - otherwise (there was a restart savepoint and txn ready to retry), then + // it behaves as a ROLLBACK to that savepoint, then starting + // a new restart cycle. + _, idx, ok := ex.state.savepointEnv.findRestart() + if !ok || !(inRestartWait || ex.machine.CurState().(stateAborted).RetryIntent.Get()) { + return makeErr(errSavepointNotUsed) } + ex.state.savepointEnv.rewindToSavepoint(idx) + res.ResetStmtType((*tree.RollbackTransaction)(nil)) if inRestartWait { return eventTxnRestart{}, nil } - // We accept ROLLBACK TO SAVEPOINT even after non-retryable errors to make - // it easy for client libraries that want to indiscriminately issue - // ROLLBACK TO SAVEPOINT after every error and possibly follow it with a - // ROLLBACK and also because we accept ROLLBACK TO SAVEPOINT in the Open - // state, so this is consistent. - // We start a new txn with the same sql timestamp and isolation as the - // current one. - ev := eventTxnStart{ + // We accept new restart SAVEPOINTs even after non-retryable errors + // to make it easy for client libraries that want to + // indiscriminately issue SAVEPOINT / ROLLBACK TO SAVEPOINT after + // every error and possibly follow it with a ROLLBACK and also + // because we accept ROLLBACK TO SAVEPOINT in the Open state, so + // this is consistent. + // + // We start a new txn with the same sql timestamp and isolation as + // the current one. + + retEv := eventTxnStart{ ImplicitTxn: fsm.False, } rwMode := tree.ReadWrite if ex.state.readOnly { rwMode = tree.ReadOnly } - payload := makeEventTxnStartPayload( + retPayload := makeEventTxnStartPayload( ex.state.priority, rwMode, ex.state.sqlTimestamp, nil /* historicalTimestamp */, ex.transitionCtx) - return ev, payload + return retEv, retPayload } -// execRollbackToSavepointInAbortedState runs a ROLLBACK TO SAVEPOINT -// statement when a txn is aborted. func (ex *connExecutor) execRollbackToSavepointInAbortedState( - ctx context.Context, inRestartWait bool, s tree.Statement, res RestrictedCommandResult, + ctx context.Context, inRestartWait bool, s *tree.RollbackToSavepoint, res RestrictedCommandResult, ) (fsm.Event, fsm.EventPayload) { - return ex.execSavepointInAbortedState(ctx, inRestartWait, s, res) + makeErr := func(err error) (fsm.Event, fsm.EventPayload) { + ev := eventNonRetriableErr{IsCommit: fsm.False} + payload := eventNonRetriableErrPayload{ + err: err, + } + return ev, payload + } + + entry, _, ok := ex.state.savepointEnv.findSavepoint(s.Savepoint) + if !ok { + return makeErr(pgerror.Newf(pgcode.InvalidSavepointSpecification, + "savepoint %s does not exist", tree.ErrString(&s.Savepoint))) + } + + if !entry.isRestartSavepoint { + /* regular savepoint */ + return makeErr(unimplemented.NewWithIssueDetail(10735, "rollback-after-error", "ROLLBACK TO SAVEPOINT in aborted state")) + } + + /* ROLLBACK TO SAVEPOINT cockroach_restart */ + // We treat this as a ROLLBACK followed by a new SAVEPOINT. + // The SAVEPOINT logic also automatically rewinds to the restart savepoint, + // so we don't have to do it here. + return ex.execSavepointInAbortedState(ctx, inRestartWait, &tree.Savepoint{Name: s.Savepoint}, res) } -// runReleaseRestartSavepointAsTxnCommit executes a commit after -// RELEASE SAVEPOINT statement when using an explicit transaction. -func (ex *connExecutor) runReleaseRestartSavepointAsTxnCommit( - ctx context.Context, stmt tree.Statement, -) (fsm.Event, fsm.EventPayload) { - if ev, payload, ok := ex.commitSQLTransactionInternal(ctx, stmt); !ok { - return ev, payload +// isRestartSavepoint returns true iff the savepoint name implies +// special "restart" semantics. +func (ex *connExecutor) isRestartSavepoint(savepoint tree.Name) bool { + if ex.sessionData.ForceSavepointRestart { + // The session setting force_savepoint_restart implies that all + // uses of the SAVEPOINT statement are targeting restarts. + return true } - return eventTxnReleased{}, nil + return strings.HasPrefix(string(savepoint), restartSavepointName) +} + +type savepointEntry struct { + name tree.Name + isRestartSavepoint bool + kvToken client.SavepointToken + // In the initial implementation, we refuse to roll back + // a savepoint if there was DDL performed "under it". + // TODO(knz): support partial DDL cancellation in pending txns. + numDDL int } -// validateSavepointName validates that it is that the provided ident -// matches the active savepoint name, begins with RestartSavepointName, -// or that force_savepoint_restart==true. We accept everything with the -// desired prefix because at least the C++ libpqxx appends sequence -// numbers to the savepoint name specified by the user. -func (ex *connExecutor) validateSavepointName(savepoint tree.Name) error { - if ex.state.activeRestartSavepointName != "" { - if savepoint == ex.state.activeRestartSavepointName { - return nil +type savepointEnv struct { + stack []savepointEntry +} + +func (env *savepointEnv) isEmpty() bool { return len(env.stack) == 0 } + +func (env *savepointEnv) clear() { env.stack = env.stack[:0] } + +func (env *savepointEnv) createSavepoint(s savepointEntry) { + env.stack = append(env.stack, s) +} + +// findRestart finds the most recent restart savepoint. +func (env *savepointEnv) findRestart() (savepointEntry, int, bool) { + for i := len(env.stack) - 1; i >= 0; i-- { + if env.stack[i].isRestartSavepoint { + return env.stack[i], i, true } - return pgerror.Newf(pgcode.InvalidSavepointSpecification, - `SAVEPOINT %q is in use`, tree.ErrString(&ex.state.activeRestartSavepointName)) } - if !ex.sessionData.ForceSavepointRestart && !strings.HasPrefix(string(savepoint), restartSavepointName) { - return unimplemented.NewWithIssueHint(10735, - "SAVEPOINT not supported except for "+restartSavepointName, - "Retryable transactions with arbitrary SAVEPOINT names can be enabled "+ - "with SET force_savepoint_restart=true") + return savepointEntry{}, -1, false +} + +// findSavepoint finds the most recent savepoint with the given name. +func (env *savepointEnv) findSavepoint(sn tree.Name) (savepointEntry, int, bool) { + for i := len(env.stack) - 1; i >= 0; i-- { + if env.stack[i].name == sn { + return env.stack[i], i, true + } } - return nil + return savepointEntry{}, -1, false +} + +func (env *savepointEnv) rewindToSavepoint(i int) { + env.stack = env.stack[:i+1] } // clearSavepoints clears all savepoints defined so far. This // occurs when the SQL txn is closed (abort/commit) and upon // a top-level restart. func (ex *connExecutor) clearSavepoints() { - ex.state.activeRestartSavepointName = "" + ex.state.savepointEnv.clear() +} + +// runShowSavepointState executes a SHOW SAVEPOINT STATUS statement. +// +// If an error is returned, the connection needs to stop processing queries. +func (ex *connExecutor) runShowSavepointState( + ctx context.Context, res RestrictedCommandResult, +) error { + res.SetColumns(ctx, sqlbase.ResultColumns{ + {Name: "savepoint_name", Typ: types.String}, + {Name: "is_restart_savepoint", Typ: types.Bool}, + }) + + for _, entry := range ex.state.savepointEnv.stack { + if err := res.AddRow(ctx, tree.Datums{ + tree.NewDString(string(entry.name)), + tree.MakeDBool(tree.DBool(entry.isRestartSavepoint)), + }); err != nil { + return err + } + } + return nil } // restartSavepointName is the only savepoint ident that we accept. -const restartSavepointName string = "cockroach_restart" +const restartSavepointName = "cockroach_restart" var errSavepointNotUsed = pgerror.Newf( pgcode.SavepointException, diff --git a/pkg/sql/conn_executor_savepoints_test.go b/pkg/sql/conn_executor_savepoints_test.go new file mode 100644 index 000000000000..dedc96238c0e --- /dev/null +++ b/pkg/sql/conn_executor_savepoints_test.go @@ -0,0 +1,194 @@ +// Copyright 2020 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package sql_test + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/util" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/datadriven" +) + +func TestSavepoints(t *testing.T) { + defer leaktest.AfterTest(t)() + + ctx := context.Background() + datadriven.Walk(t, "testdata/savepoints", func(t *testing.T, path string) { + + params := base.TestServerArgs{} + s, sqlConn, _ := serverutils.StartServer(t, params) + defer s.Stopper().Stop(ctx) + + if _, err := sqlConn.Exec("CREATE TABLE progress(n INT, marker BOOL)"); err != nil { + t.Fatal(err) + } + + datadriven.RunTest(t, path, func(t *testing.T, td *datadriven.TestData) string { + switch td.Cmd { + case "sql": + // Implicitly abort any previously-ongoing txn. + _, _ = sqlConn.Exec("ABORT") + // Prepare for the next test. + if _, err := sqlConn.Exec("DELETE FROM progress"); err != nil { + td.Fatalf(t, "cleaning up: %v", err) + } + + // Prepare a buffer to accumulate the results. + var buf strings.Builder + + // We're going to execute the input line-by-line. + stmts := strings.Split(td.Input, "\n") + + // progressBar is going to show the cancellation of writes + // during rollbacks. + progressBar := make([]byte, len(stmts)) + erase := func(status string) { + char := byte('.') + if !isOpenTxn(status) { + char = 'X' + } + for i := range progressBar { + progressBar[i] = char + } + } + + // stepNum is the index of the current statement + // in the input. + var stepNum int + + // updateProgress loads the current set of writes + // into the progress bar. + updateProgress := func() { + rows, err := sqlConn.Query("SELECT n FROM progress") + if err != nil { + t.Logf("%d: reading progress: %v", stepNum, err) + // It's OK if we can't read this. + return + } + defer rows.Close() + for rows.Next() { + var n int + if err := rows.Scan(&n); err != nil { + td.Fatalf(t, "%d: unexpected error while reading progress: %v", stepNum, err) + } + if n < 1 || n > len(progressBar) { + td.Fatalf(t, "%d: unexpected stepnum in progress table: %d", stepNum, n) + } + progressBar[n-1] = '#' + } + } + + // getTxnStatus retrieves the current txn state. + // This is guaranteed to always succeed because SHOW TRANSACTION STATUS + // is an observer statement. + getTxnStatus := func() string { + row := sqlConn.QueryRow("SHOW TRANSACTION STATUS") + var status string + if err := row.Scan(&status); err != nil { + td.Fatalf(t, "%d: unable to retrieve txn status: %v", stepNum, err) + } + return status + } + // showSavepointStatus is like getTxnStatus but retrieves the + // savepoint stack. + showSavepointStatus := func() { + rows, err := sqlConn.Query("SHOW SAVEPOINT STATUS") + if err != nil { + td.Fatalf(t, "%d: unable to retrieve savepoint status: %v", stepNum, err) + } + defer rows.Close() + + comma := "" + hasSavepoints := false + for rows.Next() { + var name string + var isRestart bool + if err := rows.Scan(&name, &isRestart); err != nil { + td.Fatalf(t, "%d: unexpected error while reading savepoints: %v", stepNum, err) + } + if isRestart { + name += "(r)" + } + buf.WriteString(comma) + buf.WriteString(name) + hasSavepoints = true + comma = ">" + } + if !hasSavepoints { + buf.WriteString("(none)") + } + } + // report shows the progress of execution so far after + // each statement executed. + report := func(beforeStatus, afterStatus string) { + erase(afterStatus) + if isOpenTxn(afterStatus) { + updateProgress() + } + fmt.Fprintf(&buf, "-- %-11s -> %-11s %s ", beforeStatus, afterStatus, string(progressBar)) + buf.WriteByte(' ') + showSavepointStatus() + buf.WriteByte('\n') + } + + // The actual execution of the statements starts here. + + beforeStatus := getTxnStatus() + for i, stmt := range stmts { + stepNum = i + 1 + // Before each statement, mark the progress so far with + // a KV write. + if isOpenTxn(beforeStatus) { + _, err := sqlConn.Exec("INSERT INTO progress(n, marker) VALUES ($1, true)", stepNum) + if err != nil { + td.Fatalf(t, "%d: before-stmt: %v", stepNum, err) + } + } + + // Run the statement and report errors/results. + fmt.Fprintf(&buf, "%d: %s -- ", stepNum, stmt) + execRes, err := sqlConn.Exec(stmt) + if err != nil { + fmt.Fprintf(&buf, "%v\n", err) + } else { + nRows, err := execRes.RowsAffected() + if err != nil { + fmt.Fprintf(&buf, "error retrieving rows: %v\n", err) + } else { + fmt.Fprintf(&buf, "%d row%s\n", nRows, util.Pluralize(nRows)) + } + } + + // Report progress on the next line + afterStatus := getTxnStatus() + report(beforeStatus, afterStatus) + beforeStatus = afterStatus + } + + return buf.String() + + default: + td.Fatalf(t, "unknown directive: %s", td.Cmd) + } + return "" + }) + }) +} + +func isOpenTxn(status string) bool { + return status == "Open" || status == "NoTxn" +} diff --git a/pkg/sql/logictest/testdata/logic_test/manual_retry b/pkg/sql/logictest/testdata/logic_test/manual_retry index e9a9f70b2a2b..e2ab0a1a8e22 100644 --- a/pkg/sql/logictest/testdata/logic_test/manual_retry +++ b/pkg/sql/logictest/testdata/logic_test/manual_retry @@ -57,9 +57,14 @@ statement ok BEGIN # Ensure that ident case rules are used. -statement error pq: unimplemented: SAVEPOINT not supported except for cockroach_restart +statement ok SAVEPOINT "COCKROACH_RESTART" +query TB +SHOW SAVEPOINT STATUS +---- +COCKROACH_RESTART false + statement ok ROLLBACK; BEGIN @@ -132,34 +137,50 @@ BEGIN TRANSACTION; SAVEPOINT something_else; COMMIT statement ok BEGIN TRANSACTION; SAVEPOINT foo -statement error pq: SAVEPOINT "foo" is in use +statement error pq: savepoint bar does not exist ROLLBACK TO SAVEPOINT bar # Verify we're doing the right thing for non-quoted idents. statement ok ROLLBACK TO SAVEPOINT FOO +statement ok +ABORT; BEGIN TRANSACTION + # Verify use of quoted idents. statement ok SAVEPOINT "Foo Bar" -statement error pq: SAVEPOINT "Foo Bar" is in use +statement error pq: savepoint foobar does not exist ROLLBACK TO SAVEPOINT FooBar # Verify case-sensitivity of quoted idents. -statement error pq: SAVEPOINT "Foo Bar" is in use +statement error pq: savepoint foo bar does not exist ROLLBACK TO SAVEPOINT "foo bar" statement ok ROLLBACK TO SAVEPOINT "Foo Bar" +query TB +SHOW SAVEPOINT STATUS +---- +Foo Bar true + +statement ok +ABORT; BEGIN TRANSACTION + # Verify case-sensitivity of quoted vs. unquoted idents. statement ok SAVEPOINT "UpperCase" -statement error pq: SAVEPOINT "UpperCase" is in use +statement error pq: savepoint uppercase does not exist ROLLBACK TO SAVEPOINT UpperCase +query TB +SHOW SAVEPOINT STATUS +---- +UpperCase true + statement ok ABORT diff --git a/pkg/sql/logictest/testdata/logic_test/txn b/pkg/sql/logictest/testdata/logic_test/txn index d86fec6afa22..483c5d543fdc 100644 --- a/pkg/sql/logictest/testdata/logic_test/txn +++ b/pkg/sql/logictest/testdata/logic_test/txn @@ -695,146 +695,6 @@ ROLLBACK; DROP SEQUENCE s -# Wrong savepoint name moves the txn state from RestartWait to Aborted. -statement ok -BEGIN TRANSACTION; - SAVEPOINT cockroach_restart; - SELECT 1 - -query error pgcode 40001 restart transaction: crdb_internal.force_retry\(\): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry\(\) -SELECT crdb_internal.force_retry('1h':::INTERVAL) - -query T -SHOW TRANSACTION STATUS ----- -RestartWait - -statement error pq: SAVEPOINT "cockroach_restart" is in use -ROLLBACK TO SAVEPOINT bogus_name - -query T -SHOW TRANSACTION STATUS ----- -Aborted - -statement ok -ROLLBACK - -# General savepoints -statement ok -BEGIN TRANSACTION - -statement error SAVEPOINT not supported except for cockroach_restart -SAVEPOINT other - -statement ok -ROLLBACK - -statement ok -BEGIN TRANSACTION - -statement error SAVEPOINT not supported except for cockroach_restart -RELEASE SAVEPOINT other - -statement ok -ROLLBACK - -statement ok -BEGIN TRANSACTION - -statement error SAVEPOINT not supported except for cockroach_restart -ROLLBACK TO SAVEPOINT other - -statement ok -ROLLBACK - -# Savepoint must be first statement in a transaction. -statement ok -BEGIN TRANSACTION; UPSERT INTO kv VALUES('savepoint', 'true') - -statement error SAVEPOINT cockroach_restart needs to be the first statement in a transaction -SAVEPOINT cockroach_restart - -statement ok -ROLLBACK - -# Can rollback to a savepoint if no statements have been executed. -statement ok -BEGIN TRANSACTION; SAVEPOINT cockroach_restart - -statement ok -ROLLBACK TO SAVEPOINT cockroach_restart - -# Can do it twice in a row. -statement ok -ROLLBACK TO SAVEPOINT cockroach_restart - -# Can rollback after a transactional write, even from a non-error state. -statement ok -UPSERT INTO kv VALUES('savepoint', 'true') - -statement ok -ROLLBACK TO SAVEPOINT cockroach_restart - -statement ok -COMMIT - -# Because we rolled back, the 'savepoint' insert will not have been committed. -query I -SELECT count(*) FROM kv WHERE k = 'savepoint' ----- -0 - - -# Can ROLLBACK TO SAVEPOINT even from a non-retryable error. -statement ok -BEGIN TRANSACTION; SAVEPOINT cockroach_restart - -statement error pq: relation "bogus_name" does not exist -SELECT * from bogus_name - -query T -SHOW TRANSACTION STATUS ----- -Aborted - -statement ok -ROLLBACK TO SAVEPOINT cockroach_restart - -query T -SHOW TRANSACTION STATUS ----- -Open - -statement ok -ROLLBACK - - -# ROLLBACK TO SAVEPOINT in a txn without a SAVEPOINT. -statement ok -BEGIN - -statement error pgcode 3B000 savepoint cockroach_restart has not been used -ROLLBACK TO SAVEPOINT cockroach_restart - -statement ok -ROLLBACK - - -# ROLLBACK TO SAVEPOINT in an aborted txn without a SAVEPOINT. -statement ok -BEGIN - -statement error pq: relation "bogus_name" does not exist -SELECT * from bogus_name - -statement error pgcode 3B000 savepoint cockroach_restart has not been used -ROLLBACK TO SAVEPOINT cockroach_restart - -statement ok -ROLLBACK - - # Test READ ONLY/WRITE syntax. statement ok @@ -1026,14 +886,6 @@ SELECT crdb_internal.force_retry('1h':::INTERVAL) statement ok ROLLBACK -# Check that we don't crash when doing a release that wasn't preceded by a -# savepoint. -statement error pgcode 3B000 savepoint cockroach_restart has not been used -BEGIN; RELEASE SAVEPOINT cockroach_restart - -statement ok -ROLLBACK - # restore the default statement ok SET default_transaction_read_only = false diff --git a/pkg/sql/metric_test.go b/pkg/sql/metric_test.go index e91ddd4f17b5..188206b8eb80 100644 --- a/pkg/sql/metric_test.go +++ b/pkg/sql/metric_test.go @@ -313,8 +313,8 @@ func TestSavepointMetrics(t *testing.T) { if err != nil { t.Fatal(err) } - if _, err := txn.Exec("SAVEPOINT blah"); err == nil { - t.Fatal("expected an error but didn't get one") + if _, err := txn.Exec("SAVEPOINT blah"); err != nil { + t.Fatal(err) } if err := txn.Rollback(); err != nil { t.Fatal(err) diff --git a/pkg/sql/opt/exec/execbuilder/builder.go b/pkg/sql/opt/exec/execbuilder/builder.go index 130b71a86b41..57ef8d7fa1a7 100644 --- a/pkg/sql/opt/exec/execbuilder/builder.go +++ b/pkg/sql/opt/exec/execbuilder/builder.go @@ -63,6 +63,11 @@ type Builder struct { allowAutoCommit bool allowInsertFastPath bool + + // -- output -- + + // IsDDL is set to true if the statement contains DDL. + IsDDL bool } // New constructs an instance of the execution node builder using the diff --git a/pkg/sql/opt/exec/execbuilder/relational.go b/pkg/sql/opt/exec/execbuilder/relational.go index 815780025bd8..b1f8eda9385a 100644 --- a/pkg/sql/opt/exec/execbuilder/relational.go +++ b/pkg/sql/opt/exec/execbuilder/relational.go @@ -141,13 +141,17 @@ func (b *Builder) buildRelational(e memo.RelExpr) (execPlan, error) { var ep execPlan var err error - // This will set the system DB trigger for transactions containing - // schema-modifying statements that have no effect, such as - // `BEGIN; INSERT INTO ...; CREATE TABLE IF NOT EXISTS ...; COMMIT;` - // where the table already exists. This will generate some false schema - // cache refreshes, but that's expected to be quite rare in practice. isDDL := opt.IsDDLOp(e) if isDDL { + // Mark the statement as containing DDL for use + // in the SQL executor. + b.IsDDL = true + + // This will set the system DB trigger for transactions containing + // schema-modifying statements that have no effect, such as + // `BEGIN; INSERT INTO ...; CREATE TABLE IF NOT EXISTS ...; COMMIT;` + // where the table already exists. This will generate some false schema + // cache refreshes, but that's expected to be quite rare in practice. if err := b.evalCtx.Txn.SetSystemConfigTrigger(); err != nil { return execPlan{}, errors.WithSecondaryError( unimplemented.NewWithIssuef(26508, diff --git a/pkg/sql/parser/help_test.go b/pkg/sql/parser/help_test.go index acffd1f1370d..50b3b3f65ee8 100644 --- a/pkg/sql/parser/help_test.go +++ b/pkg/sql/parser/help_test.go @@ -298,6 +298,7 @@ func TestContextualHelp(t *testing.T) { {`SHOW TRANSACTION ISOLATION LEVEL ??`, `SHOW TRANSACTION`}, {`SHOW SYNTAX ??`, `SHOW SYNTAX`}, {`SHOW SYNTAX 'foo' ??`, `SHOW SYNTAX`}, + {`SHOW SAVEPOINT STATUS ??`, `SHOW SAVEPOINT`}, {`SHOW RANGE ??`, `SHOW RANGE`}, diff --git a/pkg/sql/parser/parse_test.go b/pkg/sql/parser/parse_test.go index 76e8e66682fc..0755d13a5a45 100644 --- a/pkg/sql/parser/parse_test.go +++ b/pkg/sql/parser/parse_test.go @@ -519,6 +519,8 @@ func TestParse(t *testing.T) { {`SHOW TRANSACTION STATUS`}, {`EXPLAIN SHOW TRANSACTION STATUS`}, + {`SHOW SAVEPOINT STATUS`}, + {`EXPLAIN SHOW SAVEPOINT STATUS`}, {`SHOW SYNTAX 'select 1'`}, {`EXPLAIN SHOW SYNTAX 'select 1'`}, diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index b8defdb63f28..f00f2f2e6274 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -782,6 +782,7 @@ func newNameFromStr(s string) *tree.Name { %type show_sequences_stmt %type show_session_stmt %type show_sessions_stmt +%type show_savepoint_stmt %type show_stats_stmt %type show_syntax_stmt %type show_tables_stmt @@ -3376,6 +3377,7 @@ show_stmt: | show_ranges_stmt // EXTEND WITH HELP: SHOW RANGES | show_range_for_row_stmt | show_roles_stmt // EXTEND WITH HELP: SHOW ROLES +| show_savepoint_stmt // EXTEND WITH HELP: SHOW SAVEPOINT | show_schemas_stmt // EXTEND WITH HELP: SHOW SCHEMAS | show_sequences_stmt // EXTEND WITH HELP: SHOW SEQUENCES | show_session_stmt // EXTEND WITH HELP: SHOW SESSION @@ -3828,6 +3830,17 @@ show_syntax_stmt: } | SHOW SYNTAX error // SHOW HELP: SHOW SYNTAX +// %Help: SHOW SAVEPOINT - display current savepoint properties +// %Category: Cfg +// %Text: SHOW SAVEPOINT STATUS +show_savepoint_stmt: + SHOW SAVEPOINT STATUS + { + /* SKIP DOC */ + $$.val = &tree.ShowSavepointStatus{} + } +| SHOW SAVEPOINT error // SHOW HELP: SHOW SAVEPOINT + // %Help: SHOW TRANSACTION - display current transaction properties // %Category: Cfg // %Text: SHOW TRANSACTION {ISOLATION LEVEL | PRIORITY | STATUS} diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go index 09e241df09be..9cbec4b7c172 100644 --- a/pkg/sql/plan.go +++ b/pkg/sql/plan.go @@ -446,6 +446,9 @@ const ( // planFlagImplicitTxn marks that the plan was run inside of an implicit // transaction. planFlagImplicitTxn + + // planFlagIsDDL marks that the plan contains DDL. + planFlagIsDDL ) func (pf planFlags) IsSet(flag planFlags) bool { diff --git a/pkg/sql/plan_opt.go b/pkg/sql/plan_opt.go index 33aaf6249576..9de3455c2c29 100644 --- a/pkg/sql/plan_opt.go +++ b/pkg/sql/plan_opt.go @@ -167,7 +167,8 @@ func (p *planner) makeOptimizerPlan(ctx context.Context) error { // Build the plan tree. root := execMemo.RootExpr() execFactory := makeExecFactory(p) - plan, err := execbuilder.New(&execFactory, execMemo, &opc.catalog, root, p.EvalContext()).Build() + bld := execbuilder.New(&execFactory, execMemo, &opc.catalog, root, p.EvalContext()) + plan, err := bld.Build() if err != nil { return err } @@ -175,6 +176,9 @@ func (p *planner) makeOptimizerPlan(ctx context.Context) error { result := plan.(*planTop) result.AST = stmt.AST result.flags = opc.flags + if bld.IsDDL { + result.flags.Set(planFlagIsDDL) + } cols := planColumns(result.plan) if stmt.ExpectedTypes != nil { diff --git a/pkg/sql/sem/tree/show.go b/pkg/sql/sem/tree/show.go index 95a441914137..326fc8e4848a 100644 --- a/pkg/sql/sem/tree/show.go +++ b/pkg/sql/sem/tree/show.go @@ -395,6 +395,15 @@ func (node *ShowTransactionStatus) Format(ctx *FmtCtx) { ctx.WriteString("SHOW TRANSACTION STATUS") } +// ShowSavepointStatus represents a SHOW SAVEPOINT STATUS statement. +type ShowSavepointStatus struct { +} + +// Format implements the NodeFormatter interface. +func (node *ShowSavepointStatus) Format(ctx *FmtCtx) { + ctx.WriteString("SHOW SAVEPOINT STATUS") +} + // ShowUsers represents a SHOW USERS statement. type ShowUsers struct { } diff --git a/pkg/sql/sem/tree/stmt.go b/pkg/sql/sem/tree/stmt.go index cbd26d219351..1df735e023d5 100644 --- a/pkg/sql/sem/tree/stmt.go +++ b/pkg/sql/sem/tree/stmt.go @@ -769,6 +769,14 @@ func (*ShowTransactionStatus) StatementTag() string { return "SHOW TRANSACTION S func (*ShowTransactionStatus) observerStatement() {} +// StatementType implements the Statement interface. +func (*ShowSavepointStatus) StatementType() StatementType { return Rows } + +// StatementTag returns a short string identifying the type of statement. +func (*ShowSavepointStatus) StatementTag() string { return "SHOW SAVEPOINT STATUS" } + +func (*ShowSavepointStatus) observerStatement() {} + // StatementType implements the Statement interface. func (*ShowUsers) StatementType() StatementType { return Rows } @@ -962,6 +970,7 @@ func (n *ShowRanges) String() string { return AsString(n) } func (n *ShowRangeForRow) String() string { return AsString(n) } func (n *ShowRoleGrants) String() string { return AsString(n) } func (n *ShowRoles) String() string { return AsString(n) } +func (n *ShowSavepointStatus) String() string { return AsString(n) } func (n *ShowSchemas) String() string { return AsString(n) } func (n *ShowSequences) String() string { return AsString(n) } func (n *ShowSessions) String() string { return AsString(n) } diff --git a/pkg/sql/testdata/savepoints b/pkg/sql/testdata/savepoints new file mode 100644 index 000000000000..43cab25933bb --- /dev/null +++ b/pkg/sql/testdata/savepoints @@ -0,0 +1,519 @@ +# This test exercises the savepoint state in the conn executor. + +subtest implicit_release_at_end + +# It's OK to leave savepoints open when the txn commits. +# This releases everything. +sql +BEGIN +SAVEPOINT foo +SAVEPOINT bar +SAVEPOINT baz +COMMIT +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.... (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##... foo +3: SAVEPOINT bar -- 0 rows +-- Open -> Open ###.. foo>bar +4: SAVEPOINT baz -- 0 rows +-- Open -> Open ####. foo>bar>baz +5: COMMIT -- 0 rows +-- Open -> NoTxn ##### (none) + +# Ditto rollbacks. +sql +BEGIN +SAVEPOINT foo +SAVEPOINT bar +SAVEPOINT baz +ROLLBACK +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.... (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##... foo +3: SAVEPOINT bar -- 0 rows +-- Open -> Open ###.. foo>bar +4: SAVEPOINT baz -- 0 rows +-- Open -> Open ####. foo>bar>baz +5: ROLLBACK -- 0 rows +-- Open -> NoTxn #.... (none) + +subtest end + +subtest savepoint_stack + +sql +BEGIN +SAVEPOINT foo +SAVEPOINT foo +SAVEPOINT bar +SAVEPOINT baz +ROLLBACK TO SAVEPOINT foo +SAVEPOINT baz +RELEASE SAVEPOINT foo +SAVEPOINT bar +RELEASE SAVEPOINT foo +COMMIT +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.......... (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##......... foo +3: SAVEPOINT foo -- 0 rows +-- Open -> Open ###........ foo>foo +4: SAVEPOINT bar -- 0 rows +-- Open -> Open ####....... foo>foo>bar +5: SAVEPOINT baz -- 0 rows +-- Open -> Open #####...... foo>foo>bar>baz +6: ROLLBACK TO SAVEPOINT foo -- 0 rows +-- Open -> Open ###........ foo>foo +7: SAVEPOINT baz -- 0 rows +-- Open -> Open ###...#.... foo>foo>baz +8: RELEASE SAVEPOINT foo -- 0 rows +-- Open -> Open ###...##... foo +9: SAVEPOINT bar -- 0 rows +-- Open -> Open ###...###.. foo>bar +10: RELEASE SAVEPOINT foo -- 0 rows +-- Open -> Open ###...####. (none) +11: COMMIT -- 0 rows +-- Open -> NoTxn ###...##### (none) + + +subtest end + +subtest savepoint_release_vs_rollback + +# A rollback keeps the savepoint active. +sql +BEGIN +SAVEPOINT foo +ROLLBACK TO SAVEPOINT foo +ROLLBACK TO SAVEPOINT foo +COMMIT +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.... (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##... foo +3: ROLLBACK TO SAVEPOINT foo -- 0 rows +-- Open -> Open ##... foo +4: ROLLBACK TO SAVEPOINT foo -- 0 rows +-- Open -> Open ##... foo +5: COMMIT -- 0 rows +-- Open -> NoTxn ##..# (none) + +# A release does not. +sql +BEGIN +SAVEPOINT foo +RELEASE SAVEPOINT foo +RELEASE SAVEPOINT foo +COMMIT +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.... (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##... foo +3: RELEASE SAVEPOINT foo -- 0 rows +-- Open -> Open ###.. (none) +4: RELEASE SAVEPOINT foo -- pq: savepoint foo does not exist +-- Open -> Aborted XXXXX (none) +5: COMMIT -- 0 rows +-- Aborted -> NoTxn #.... (none) + +subtest end + + +subtest rollback_after_sql_error + +# This should be supported. +# FIXME(knz): make this work + +sql +BEGIN +SAVEPOINT foo +SELECT nonexistent +ROLLBACK TO SAVEPOINT foo +SELECT 123 +COMMIT +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #..... (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##.... foo +3: SELECT nonexistent -- pq: column "nonexistent" does not exist +-- Open -> Aborted XXXXXX foo +4: ROLLBACK TO SAVEPOINT foo -- pq: unimplemented: ROLLBACK TO SAVEPOINT in aborted state +-- Aborted -> Aborted XXXXXX foo +5: SELECT 123 -- pq: current transaction is aborted, commands ignored until end of transaction block +-- Aborted -> Aborted XXXXXX foo +6: COMMIT -- 0 rows +-- Aborted -> NoTxn #..... (none) + +subtest end + +subtest rollback_after_dup_error + +# This should be supported. +# FIXME(knz): make this work + +sql +CREATE TABLE t(x INT UNIQUE) +INSERT INTO t(x) VALUES (1) +BEGIN +SAVEPOINT foo +INSERT INTO t(x) VALUES (1) +ROLLBACK TO SAVEPOINT foo +INSERT INTO t(x) VALUES (2) +COMMIT +---- +1: CREATE TABLE t(x INT UNIQUE) -- 0 rows +-- NoTxn -> NoTxn #....... (none) +2: INSERT INTO t(x) VALUES (1) -- 1 row +-- NoTxn -> NoTxn ##...... (none) +3: BEGIN -- 0 rows +-- NoTxn -> Open ###..... (none) +4: SAVEPOINT foo -- 0 rows +-- Open -> Open ####.... foo +5: INSERT INTO t(x) VALUES (1) -- pq: duplicate key value (x)=(1) violates unique constraint "t_x_key" +-- Open -> Aborted XXXXXXXX foo +6: ROLLBACK TO SAVEPOINT foo -- pq: unimplemented: ROLLBACK TO SAVEPOINT in aborted state +-- Aborted -> Aborted XXXXXXXX foo +7: INSERT INTO t(x) VALUES (2) -- pq: current transaction is aborted, commands ignored until end of transaction block +-- Aborted -> Aborted XXXXXXXX foo +8: COMMIT -- 0 rows +-- Aborted -> NoTxn ###..... (none) + +sql +DROP TABLE t +---- +1: DROP TABLE t -- 0 rows +-- NoTxn -> NoTxn # (none) + +subtest end + +subtest rollback_after_ddl + +# DDL under savepoints is fine as long as there is no rollback. +# Note: we do two DDL; the first one is there just to anchor +# the txn on the config range. The second DDL is the one +# exercised in the test. +sql +BEGIN; CREATE TABLE unused(x INT) +SAVEPOINT foo +CREATE TABLE t(x INT) +RELEASE SAVEPOINT foo +COMMIT +---- +1: BEGIN; CREATE TABLE unused(x INT) -- 0 rows +-- NoTxn -> Open #.... (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##... foo +3: CREATE TABLE t(x INT) -- 0 rows +-- Open -> Open ###.. foo +4: RELEASE SAVEPOINT foo -- 0 rows +-- Open -> Open ####. (none) +5: COMMIT -- 0 rows +-- Open -> NoTxn ##### (none) + +sql +DROP TABLE unused +DROP TABLE t +---- +1: DROP TABLE unused -- 0 rows +-- NoTxn -> NoTxn #. (none) +2: DROP TABLE t -- 0 rows +-- NoTxn -> NoTxn ## (none) + +# Rollback is unsupported after DDL for now. +# TODO(knz): Lift this limitation. + +sql +BEGIN; CREATE TABLE unused(x INT) +SAVEPOINT foo +CREATE TABLE t(x INT) +ROLLBACK TO SAVEPOINT foo +---- +1: BEGIN; CREATE TABLE unused(x INT) -- 0 rows +-- NoTxn -> Open #... (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##.. foo +3: CREATE TABLE t(x INT) -- 0 rows +-- Open -> Open ###. foo +4: ROLLBACK TO SAVEPOINT foo -- pq: unimplemented: ROLLBACK TO SAVEPOINT not yet supported after DDL statements +-- Open -> Aborted XXXX foo + +subtest end + +subtest unsupported_restart_under_savepoint + +# Today we don't support txn retries under a regular savepoint, +# in other words a retry can only retry the entire txn. +# This may become supported in the future. + +sql +BEGIN +SAVEPOINT foo +SAVEPOINT cockroach_restart +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.. (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##. foo +3: SAVEPOINT cockroach_restart -- pq: SAVEPOINT cockroach_restart cannot be nested +-- Open -> Aborted XXX foo + + +subtest end + +subtest invalid_uses + +sql +SAVEPOINT foo +ROLLBACK TO SAVEPOINT foo +RELEASE SAVEPOINT foo +---- +1: SAVEPOINT foo -- pq: there is no transaction in progress +-- NoTxn -> NoTxn #.. (none) +2: ROLLBACK TO SAVEPOINT foo -- pq: savepoint foo does not exist +-- NoTxn -> NoTxn ##. (none) +3: RELEASE SAVEPOINT foo -- pq: there is no transaction in progress +-- NoTxn -> NoTxn ### (none) + +sql +BEGIN +SAVEPOINT foo +RELEASE SAVEPOINT bar +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.. (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##. foo +3: RELEASE SAVEPOINT bar -- pq: savepoint bar does not exist +-- Open -> Aborted XXX foo + +sql +BEGIN +SAVEPOINT foo +ROLLBACK TO SAVEPOINT bar +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.. (none) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##. foo +3: ROLLBACK TO SAVEPOINT bar -- pq: savepoint bar does not exist +-- Open -> Aborted XXX foo + +subtest end + +subtest restart + +subtest restart/must_be_first_in_txn + +sql +CREATE TABLE t(x INT) +BEGIN +INSERT INTO t(x) VALUES (1) +SAVEPOINT cockroach_restart +---- +1: CREATE TABLE t(x INT) -- 0 rows +-- NoTxn -> NoTxn #... (none) +2: BEGIN -- 0 rows +-- NoTxn -> Open ##.. (none) +3: INSERT INTO t(x) VALUES (1) -- 1 row +-- Open -> Open ###. (none) +4: SAVEPOINT cockroach_restart -- pq: SAVEPOINT cockroach_restart needs to be the first statement in a transaction +-- Open -> Aborted XXXX (none) + +sql +DROP TABLE t +---- +1: DROP TABLE t -- 0 rows +-- NoTxn -> NoTxn # (none) + +subtest end + +subtest restart/restartwait_to_aborted + +# Wrong savepoint name moves the txn state from RestartWait to Aborted. +sql +BEGIN; SAVEPOINT cockroach_restart +SELECT 1 +SELECT crdb_internal.force_retry('1h':::INTERVAL) +ROLLBACK TO SAVEPOINT bogus_name +ROLLBACK +---- +1: BEGIN; SAVEPOINT cockroach_restart -- 0 rows +-- NoTxn -> Open #.... cockroach_restart(r) +2: SELECT 1 -- 1 row +-- Open -> Open ##... cockroach_restart(r) +3: SELECT crdb_internal.force_retry('1h':::INTERVAL) -- pq: restart transaction: crdb_internal.force_retry(): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry() +-- Open -> RestartWait XXXXX cockroach_restart(r) +4: ROLLBACK TO SAVEPOINT bogus_name -- pq: savepoint bogus_name does not exist +-- RestartWait -> Aborted XXXXX cockroach_restart(r) +5: ROLLBACK -- 0 rows +-- Aborted -> NoTxn #.... (none) + +subtest end + +subtest restart/rollback_after_error + +# Can ROLLBACK TO SAVEPOINT even from a non-retryable error. +# Note: we're aiming for this to work for non-restart +# savepoints too. + +sql +BEGIN; SAVEPOINT cockroach_restart +SELECT * FROM bogus_name +ROLLBACK TO SAVEPOINT cockroach_restart +ROLLBACK +---- +1: BEGIN; SAVEPOINT cockroach_restart -- 0 rows +-- NoTxn -> Open #... cockroach_restart(r) +2: SELECT * FROM bogus_name -- pq: relation "bogus_name" does not exist +-- Open -> Aborted XXXX cockroach_restart(r) +3: ROLLBACK TO SAVEPOINT cockroach_restart -- 0 rows +-- Aborted -> Open #... cockroach_restart(r) +4: ROLLBACK -- 0 rows +-- Open -> NoTxn #... (none) + +subtest end + +subtest restart/release_without_savepoint + +sql +BEGIN +RELEASE SAVEPOINT cockroach_restart +ROLLBACK +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.. (none) +2: RELEASE SAVEPOINT cockroach_restart -- pq: savepoint cockroach_restart does not exist +-- Open -> Aborted XXX (none) +3: ROLLBACK -- 0 rows +-- Aborted -> NoTxn #.. (none) + +subtest end + +subtest restart/rollback_without_savepoint + +# ROLLBACK TO SAVEPOINT in an open txn without a SAVEPOINT. +sql +BEGIN +ROLLBACK TO SAVEPOINT cockroach_restart +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #. (none) +2: ROLLBACK TO SAVEPOINT cockroach_restart -- pq: savepoint cockroach_restart does not exist +-- Open -> Aborted XX (none) + +# ROLLBACK TO SAVEPOINT in an aborted txn without a SAVEPOINT. +sql +BEGIN +SELECT * FROM bogus_name +ROLLBACK TO SAVEPOINT cockroach_restart +---- +1: BEGIN -- 0 rows +-- NoTxn -> Open #.. (none) +2: SELECT * FROM bogus_name -- pq: relation "bogus_name" does not exist +-- Open -> Aborted XXX (none) +3: ROLLBACK TO SAVEPOINT cockroach_restart -- pq: savepoint cockroach_restart does not exist +-- Aborted -> Aborted XXX (none) + +subtest end + +subtest restart/rollbacks + +sql +CREATE TABLE t(x INT); +BEGIN; SAVEPOINT cockroach_restart +ROLLBACK TO SAVEPOINT cockroach_restart +ROLLBACK TO SAVEPOINT cockroach_restart +INSERT INTO t(x) VALUES (1) +ROLLBACK TO SAVEPOINT cockroach_restart +COMMIT +---- +1: CREATE TABLE t(x INT); -- 0 rows +-- NoTxn -> NoTxn #...... (none) +2: BEGIN; SAVEPOINT cockroach_restart -- 0 rows +-- NoTxn -> Open ##..... cockroach_restart(r) +3: ROLLBACK TO SAVEPOINT cockroach_restart -- 0 rows +-- Open -> Open ##..... cockroach_restart(r) +4: ROLLBACK TO SAVEPOINT cockroach_restart -- 0 rows +-- Open -> Open ##..... cockroach_restart(r) +5: INSERT INTO t(x) VALUES (1) -- 1 row +-- Open -> Open ##..#.. cockroach_restart(r) +6: ROLLBACK TO SAVEPOINT cockroach_restart -- 0 rows +-- Open -> Open ##..... cockroach_restart(r) +7: COMMIT -- 0 rows +-- Open -> NoTxn ##....# (none) + +sql +DROP TABLE t +---- +1: DROP TABLE t -- 0 rows +-- NoTxn -> NoTxn # (none) + +subtest end + + +subtest restart/savepoint_under_restart + +sql +BEGIN; SAVEPOINT cockroach_restart +SAVEPOINT foo +SAVEPOINT bar +ROLLBACK TO SAVEPOINT foo +SELECT crdb_internal.force_retry('1s') +ROLLBACK TO SAVEPOINT cockroach_restart +SELECT 123 +COMMIT +---- +1: BEGIN; SAVEPOINT cockroach_restart -- 0 rows +-- NoTxn -> Open #....... cockroach_restart(r) +2: SAVEPOINT foo -- 0 rows +-- Open -> Open ##...... cockroach_restart(r)>foo +3: SAVEPOINT bar -- 0 rows +-- Open -> Open ###..... cockroach_restart(r)>foo>bar +4: ROLLBACK TO SAVEPOINT foo -- 0 rows +-- Open -> Open ##...... cockroach_restart(r)>foo +5: SELECT crdb_internal.force_retry('1s') -- pq: restart transaction: crdb_internal.force_retry(): TransactionRetryWithProtoRefreshError: forced by crdb_internal.force_retry() +-- Open -> RestartWait XXXXXXXX cockroach_restart(r)>foo +6: ROLLBACK TO SAVEPOINT cockroach_restart -- 0 rows +-- RestartWait -> Open #....... cockroach_restart(r) +7: SELECT 123 -- 1 row +-- Open -> Open #.....#. cockroach_restart(r) +8: COMMIT -- 0 rows +-- Open -> NoTxn #.....## (none) + +subtest end + +subtest restart/all_savepoints_disabled + +# Under "force_savepoint_restart", every savepoint +# is a restart savepoint. + +sql +SET force_savepoint_restart = true +BEGIN; SAVEPOINT foo +SAVEPOINT bar +---- +1: SET force_savepoint_restart = true -- 0 rows +-- NoTxn -> NoTxn #.. (none) +2: BEGIN; SAVEPOINT foo -- 0 rows +-- NoTxn -> Open ##. foo(r) +3: SAVEPOINT bar -- pq: SAVEPOINT cockroach_restart cannot be nested +-- Open -> Aborted XXX foo(r) + +sql +SET force_savepoint_restart = false +---- +1: SET force_savepoint_restart = false -- 0 rows +-- NoTxn -> NoTxn # (none) + +subtest end + +subtest end diff --git a/pkg/sql/txn_restart_test.go b/pkg/sql/txn_restart_test.go index 18646e2c1fcf..5950834e162e 100644 --- a/pkg/sql/txn_restart_test.go +++ b/pkg/sql/txn_restart_test.go @@ -1449,12 +1449,6 @@ func TestRollbackToSavepointFromUnusualStates(t *testing.T) { } } - // ROLLBACK TO SAVEPOINT with a wrong name - _, err := sqlDB.Exec("ROLLBACK TO SAVEPOINT foo") - if !testutils.IsError(err, "SAVEPOINT not supported except for cockroach_restart") { - t.Fatalf("unexpected error: %v", err) - } - tx, err := sqlDB.Begin() if err != nil { t.Fatal(err) diff --git a/pkg/sql/txn_state.go b/pkg/sql/txn_state.go index 963d6f94d45a..94457715738e 100644 --- a/pkg/sql/txn_state.go +++ b/pkg/sql/txn_state.go @@ -105,10 +105,8 @@ type txnState struct { // stateAborted. txnAbortCount *metric.Counter - // activeRestartSavepointName stores the name of the active - // top-level restart savepoint, or is empty if no top-level restart - // savepoint is active. - activeRestartSavepointName tree.Name + // savepointEnv stores the stack of savepoints. + savepointEnv savepointEnv } // txnType represents the type of a SQL transaction.