From 1ed61fcca81797172151832c6e94e78299771d5f Mon Sep 17 00:00:00 2001 From: Bob Vawter Date: Mon, 29 Oct 2018 11:27:14 -0400 Subject: [PATCH] sql: Allow restart savepoint to be renamed Currently, the only allowable savepoint names are those beginning with `cockroach_restart` which triggers a Cockroach-specific transaction-retry flow. This creates compatibility issues with certain integration scenarios (e.g. apps using Spring ORM) that can take advantage of restartable transactions, but which assume than an arbitrary savepoint identifier may be used. This patch introduces a new session variable `force_savepoint_restart` which allows the user to customize the savepoint name to any valid identifier, while always triggering a restartable transaction. Telemetry: * Unimplemented-feature error now includes reference to #10735. * `sql.force_savepoint_restart` event recorded when enabled. Resolves #30588 (customize `SAVEPOINT` name) Relates to #15012 (possible change for detecting "empty" transactions) Release note (sql change): Users can customize the auto-retry savepoint name used by the `SAVEPOINT` command by setting the `force_savepoint_restart` session variable. For example: `SET force_savepoint_restart=true; BEGIN; SAVEPOINT foo` will now function as desired. This session variable may also be supplied as part of a connection string to support existing code that assumes that arbitrary savepoint names may be used. Release note (sql change): The names supplied to a `SAVEPOINT` command are now properly treated as SQL identifiers. That is `SAVEPOINT foo` and `SAVEPOINT FOO` are now equivalent statements. --- pkg/sql/conn_executor_exec.go | 71 ++++++++++++++++--- pkg/sql/exec_util.go | 4 ++ .../testdata/logic_test/manual_retry | 69 +++++++++++++++++- .../logictest/testdata/logic_test/pg_catalog | 3 + .../logictest/testdata/logic_test/show_source | 1 + pkg/sql/logictest/testdata/logic_test/txn | 16 ++--- .../logictest/testdata/planner_test/explain | 10 +-- pkg/sql/opt/exec/execbuilder/testdata/explain | 10 +-- pkg/sql/parser/parse_test.go | 3 +- pkg/sql/parser/sql.y | 6 +- pkg/sql/pgwire/pgerror/errors.go | 9 +++ pkg/sql/sem/tree/txn.go | 28 ++------ pkg/sql/sessiondata/session_data.go | 6 ++ pkg/sql/txn_restart_test.go | 2 +- pkg/sql/txn_state.go | 4 ++ pkg/sql/vars.go | 23 +++++- 16 files changed, 208 insertions(+), 57 deletions(-) diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index dea5c46f3756..e0f0a3506e56 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -41,9 +41,12 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/tracing" ) +// RestartSavepointName is the only savepoint ident that we accept. +const RestartSavepointName string = "cockroach_restart" + var errSavepointNotUsed = pgerror.NewErrorf( pgerror.CodeSavepointExceptionError, - "savepoint %s has not been used", tree.RestartSavepointName) + "savepoint %s has not been used", RestartSavepointName) // execStmt executes one statement by dispatching according to the current // state. Returns an Event to be passed to the state machine, or nil if no @@ -221,7 +224,7 @@ func (ex *connExecutor) execStmtInOpenState( return ev, payload, nil case *tree.ReleaseSavepoint: - if err := tree.ValidateRestartCheckpoint(s.Savepoint); err != nil { + if err := ex.validateSavepointName(s.Savepoint); err != nil { return makeErrEvent(err) } if !ex.machine.CurState().(stateOpen).RetryIntent.Get() { @@ -239,29 +242,40 @@ func (ex *connExecutor) execStmtInOpenState( return ev, payload, nil case *tree.Savepoint: - if err := tree.ValidateRestartCheckpoint(s.Name); err != nil { + // Ensure that the user isn't trying to run BEGIN; SAVEPOINT; SAVEPOINT; + if ex.state.activeSavepointName != "" { + err := fmt.Errorf("SAVEPOINT may not be nested") + return makeErrEvent(err) + } + if err := ex.validateSavepointName(s.Name); err != nil { return makeErrEvent(err) } - // We want to disallow SAVEPOINTs to be issued after a transaction has + // 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. + // 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 meta := ex.state.mu.txn.GetTxnCoordMeta(ctx) if meta.CommandCount > 0 { err := fmt.Errorf("SAVEPOINT %s needs to be the first statement in a "+ - "transaction", tree.RestartSavepointName) + "transaction", RestartSavepointName) return makeErrEvent(err) } + ex.state.activeSavepointName = 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 case *tree.RollbackToSavepoint: - if err := tree.ValidateRestartCheckpoint(s.Savepoint); err != nil { + if err := ex.validateSavepointName(s.Savepoint); err != nil { return makeErrEvent(err) } if !os.RetryIntent.Get() { return makeErrEvent(errSavepointNotUsed) } + ex.state.activeSavepointName = "" res.ResetStmtType((*tree.Savepoint)(nil)) return eventTxnRestart{}, nil /* payload */, nil @@ -573,6 +587,7 @@ func (ex *connExecutor) checkTableTwoVersionInvariant(ctx context.Context) error func (ex *connExecutor) commitSQLTransaction( ctx context.Context, stmt tree.Statement, ) (fsm.Event, fsm.EventPayload) { + ex.state.activeSavepointName = "" isRelease := false if _, ok := stmt.(*tree.ReleaseSavepoint); ok { isRelease = true @@ -595,6 +610,7 @@ func (ex *connExecutor) commitSQLTransaction( // rollbackSQLTransaction executes a ROLLBACK statement: the KV transaction is // rolled-back and an event is produced. func (ex *connExecutor) rollbackSQLTransaction(ctx context.Context) (fsm.Event, fsm.EventPayload) { + ex.state.activeSavepointName = "" if err := ex.state.mu.txn.Rollback(ctx); err != nil { log.Warningf(ctx, "txn rollback failed: %s", err) } @@ -1073,6 +1089,7 @@ func (ex *connExecutor) execStmtInAbortedState( ev, payload := ex.rollbackSQLTransaction(ctx) return ev, payload } + ex.state.activeSavepointName = "" // Note: Postgres replies to COMMIT of failed txn with "ROLLBACK" too. res.ResetStmtType((*tree.RollbackTransaction)(nil)) @@ -1082,22 +1099,36 @@ func (ex *connExecutor) execStmtInAbortedState( // 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 string + 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 err := tree.ValidateRestartCheckpoint(spName); err != nil { + // If the user issued a SAVEPOINT in the abort state, validate + // as though there were no active savepoint. + if !isRollback { + ex.state.activeSavepointName = "" + } + if err := ex.validateSavepointName(spName); err != nil { 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.activeSavepointName = "" + } else { + ex.state.activeSavepointName = spName + } if !(inRestartWait || ex.machine.CurState().(stateAborted).RetryIntent.Get()) { ev := eventNonRetriableErr{IsCommit: fsm.False} @@ -1354,3 +1385,25 @@ func (ex *connExecutor) incrementStmtCounter(stmt Statement) { ex.server.StatementCounters.incrementCount(stmt.AST) } } + +// 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.activeSavepointName != "" { + if savepoint == ex.state.activeSavepointName { + return nil + } + return pgerror.NewErrorf(pgerror.CodeInvalidSavepointSpecificationError, + `SAVEPOINT %q is in use`, tree.ErrString(&ex.state.activeSavepointName)) + } + if !ex.sessionData.ForceSavepointRestart && !strings.HasPrefix(string(savepoint), RestartSavepointName) { + return pgerror.UnimplementedWithIssueHintError(10735, + "SAVEPOINT not supported except for "+RestartSavepointName, + "Retryable transactions with arbitrary SAVEPOINT names can be enabled "+ + "with SET force_savepoint_restart=true") + } + return nil +} diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 63954eeadf88..b34693424859 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -1618,6 +1618,10 @@ func (m *sessionDataMutator) SetDistSQLMode(val sessiondata.DistSQLExecMode) { m.data.DistSQLMode = val } +func (m *sessionDataMutator) SetForceSavepointRestart(val bool) { + m.data.ForceSavepointRestart = val +} + func (m *sessionDataMutator) SetLookupJoinEnabled(val bool) { m.data.LookupJoinEnabled = val } diff --git a/pkg/sql/logictest/testdata/logic_test/manual_retry b/pkg/sql/logictest/testdata/logic_test/manual_retry index 4527fad2f831..ed952303ad78 100644 --- a/pkg/sql/logictest/testdata/logic_test/manual_retry +++ b/pkg/sql/logictest/testdata/logic_test/manual_retry @@ -28,8 +28,13 @@ ROLLBACK TO SAVEPOINT cockroach_restart # wait until the transaction is at least 1 second sleep 1s +# Ensure that ident case rules are used. +statement error pq: unimplemented: SAVEPOINT not supported except for cockroach_restart +SAVEPOINT "COCKROACH_RESTART" + +# Ensure that ident case rules are used. statement ok -SAVEPOINT cockroach_restart +SAVEPOINT COCKROACH_RESTART query I SELECT crdb_internal.force_retry('500ms':::INTERVAL) @@ -77,3 +82,65 @@ query I SELECT id FROM t ---- 1 + +subtest rename_savepoint + +query T +show session force_savepoint_restart +---- +off + +statement ok +SET force_savepoint_restart = true + +query T +show session force_savepoint_restart +---- +on + +# We can now use anything that we want. +statement ok +BEGIN TRANSACTION; SAVEPOINT something_else; COMMIT + +# Ensure that we can't mix-and-match names. +statement ok +BEGIN TRANSACTION; SAVEPOINT foo + +statement error pq: SAVEPOINT "foo" is in use +ROLLBACK TO SAVEPOINT bar + +# Verify we're doing the right thing for non-quoted idents. +statement ok +ROLLBACK TO SAVEPOINT FOO + +# Verify use of quoted idents. +statement ok +SAVEPOINT "Foo Bar" + +statement error pq: SAVEPOINT "Foo Bar" is in use +ROLLBACK TO SAVEPOINT FooBar + +# Verify case-sensitivity of quoted idents. +statement error pq: SAVEPOINT "Foo Bar" is in use +ROLLBACK TO SAVEPOINT "foo bar" + +statement ok +ROLLBACK TO SAVEPOINT "Foo Bar" + +# Verify case-sensitivity of quoted vs. unquoted idents. +statement ok +SAVEPOINT "UpperCase" + +statement error pq: SAVEPOINT "UpperCase" is in use +ROLLBACK TO SAVEPOINT UpperCase + +statement ok +ABORT + +statement ok +RESET force_savepoint_restart + +query T +show session force_savepoint_restart +---- +off diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index 6929ead53bd5..b86d65f64a5d 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -1340,6 +1340,7 @@ experimental_force_zigzag_join off NULL NULL NULL experimental_serial_normalization rowid NULL NULL NULL string experimental_vectorize off NULL NULL NULL string extra_float_digits 0 NULL NULL NULL string +force_savepoint_restart off NULL NULL NULL string integer_datetimes on NULL NULL NULL string intervalstyle postgres NULL NULL NULL string max_index_keys 32 NULL NULL NULL string @@ -1383,6 +1384,7 @@ experimental_force_zigzag_join off NULL user NULL off experimental_serial_normalization rowid NULL user NULL rowid rowid experimental_vectorize off NULL user NULL off off extra_float_digits 0 NULL user NULL 0 2 +force_savepoint_restart off NULL user NULL off off integer_datetimes on NULL user NULL on on intervalstyle postgres NULL user NULL postgres postgres max_index_keys 32 NULL user NULL 32 32 @@ -1422,6 +1424,7 @@ experimental_force_zigzag_join NULL NULL NULL NULL NULL experimental_serial_normalization NULL NULL NULL NULL NULL experimental_vectorize NULL NULL NULL NULL NULL extra_float_digits NULL NULL NULL NULL NULL +force_savepoint_restart NULL NULL NULL NULL NULL integer_datetimes NULL NULL NULL NULL NULL intervalstyle NULL NULL NULL NULL NULL max_index_keys NULL NULL NULL NULL NULL diff --git a/pkg/sql/logictest/testdata/logic_test/show_source b/pkg/sql/logictest/testdata/logic_test/show_source index 7a0ac7578db0..36147944aa88 100644 --- a/pkg/sql/logictest/testdata/logic_test/show_source +++ b/pkg/sql/logictest/testdata/logic_test/show_source @@ -39,6 +39,7 @@ experimental_force_zigzag_join off experimental_serial_normalization rowid experimental_vectorize off extra_float_digits 0 +force_savepoint_restart off integer_datetimes on intervalstyle postgres max_index_keys 32 diff --git a/pkg/sql/logictest/testdata/logic_test/txn b/pkg/sql/logictest/testdata/logic_test/txn index 5c2f71f28efd..ad415714dd1d 100644 --- a/pkg/sql/logictest/testdata/logic_test/txn +++ b/pkg/sql/logictest/testdata/logic_test/txn @@ -661,7 +661,7 @@ SHOW TRANSACTION STATUS ---- RestartWait -statement error SAVEPOINT not supported except for COCKROACH_RESTART +statement error pq: SAVEPOINT "cockroach_restart" is in use ROLLBACK TO SAVEPOINT bogus_name query T @@ -676,7 +676,7 @@ ROLLBACK statement ok BEGIN TRANSACTION -statement error SAVEPOINT not supported except for COCKROACH_RESTART +statement error SAVEPOINT not supported except for cockroach_restart SAVEPOINT other statement ok @@ -685,7 +685,7 @@ ROLLBACK statement ok BEGIN TRANSACTION -statement error SAVEPOINT not supported except for COCKROACH_RESTART +statement error SAVEPOINT not supported except for cockroach_restart RELEASE SAVEPOINT other statement ok @@ -694,7 +694,7 @@ ROLLBACK statement ok BEGIN TRANSACTION -statement error SAVEPOINT not supported except for COCKROACH_RESTART +statement error SAVEPOINT not supported except for cockroach_restart ROLLBACK TO SAVEPOINT other statement ok @@ -704,7 +704,7 @@ ROLLBACK statement ok BEGIN TRANSACTION; UPSERT INTO kv VALUES('savepoint', 'true') -statement error SAVEPOINT COCKROACH_RESTART needs to be the first statement in a transaction +statement error SAVEPOINT cockroach_restart needs to be the first statement in a transaction SAVEPOINT cockroach_restart statement ok @@ -766,7 +766,7 @@ ROLLBACK statement ok BEGIN -statement error pgcode 3B000 savepoint COCKROACH_RESTART has not been used +statement error pgcode 3B000 savepoint cockroach_restart has not been used ROLLBACK TO SAVEPOINT cockroach_restart statement ok @@ -780,7 +780,7 @@ 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 +statement error pgcode 3B000 savepoint cockroach_restart has not been used ROLLBACK TO SAVEPOINT cockroach_restart statement ok @@ -980,7 +980,7 @@ 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 +statement error pgcode 3B000 savepoint cockroach_restart has not been used BEGIN; RELEASE SAVEPOINT cockroach_restart statement ok diff --git a/pkg/sql/logictest/testdata/planner_test/explain b/pkg/sql/logictest/testdata/planner_test/explain index 64d07fc102b7..2ac8e482b730 100644 --- a/pkg/sql/logictest/testdata/planner_test/explain +++ b/pkg/sql/logictest/testdata/planner_test/explain @@ -171,7 +171,7 @@ render · · └── filter · · │ filter variable = 'database' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW TIME ZONE @@ -180,7 +180,7 @@ render · · └── filter · · │ filter variable = 'timezone' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW DEFAULT_TRANSACTION_ISOLATION @@ -189,7 +189,7 @@ render · · └── filter · · │ filter variable = 'default_transaction_isolation' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW TRANSACTION ISOLATION LEVEL @@ -198,7 +198,7 @@ render · · └── filter · · │ filter variable = 'transaction_isolation' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW TRANSACTION PRIORITY @@ -207,7 +207,7 @@ render · · └── filter · · │ filter variable = 'transaction_priority' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW COLUMNS FROM foo diff --git a/pkg/sql/opt/exec/execbuilder/testdata/explain b/pkg/sql/opt/exec/execbuilder/testdata/explain index 33405ea35627..4309474052ce 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/explain +++ b/pkg/sql/opt/exec/execbuilder/testdata/explain @@ -162,7 +162,7 @@ render · · └── filter · · │ filter variable = 'database' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW TIME ZONE @@ -171,7 +171,7 @@ render · · └── filter · · │ filter variable = 'timezone' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW DEFAULT_TRANSACTION_ISOLATION @@ -180,7 +180,7 @@ render · · └── filter · · │ filter variable = 'default_transaction_isolation' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW TRANSACTION ISOLATION LEVEL @@ -189,7 +189,7 @@ render · · └── filter · · │ filter variable = 'transaction_isolation' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW TRANSACTION PRIORITY @@ -198,7 +198,7 @@ render · · └── filter · · │ filter variable = 'transaction_priority' └── values · · -· size 2 columns, 35 rows +· size 2 columns, 36 rows query TTT EXPLAIN SHOW COLUMNS FROM foo diff --git a/pkg/sql/parser/parse_test.go b/pkg/sql/parser/parse_test.go index 59dd88dffe47..8225eb409efd 100644 --- a/pkg/sql/parser/parse_test.go +++ b/pkg/sql/parser/parse_test.go @@ -51,7 +51,8 @@ func TestParse(t *testing.T) { {`BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE, PRIORITY HIGH, READ WRITE`}, {`COMMIT TRANSACTION`}, {`ROLLBACK TRANSACTION`}, - {"SAVEPOINT foo"}, + {`SAVEPOINT foo`}, + {`SAVEPOINT "foo bar"`}, {`CREATE DATABASE a`}, {`EXPLAIN CREATE DATABASE a`}, diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index c651767a49db..bb037bd25f60 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -4922,7 +4922,7 @@ opt_set_data: release_stmt: RELEASE savepoint_name { - $$.val = &tree.ReleaseSavepoint{Savepoint: $2} + $$.val = &tree.ReleaseSavepoint{Savepoint: tree.Name($2)} } | RELEASE error // SHOW HELP: RELEASE @@ -4955,7 +4955,7 @@ resume_stmt: savepoint_stmt: SAVEPOINT name { - $$.val = &tree.Savepoint{Name: $2} + $$.val = &tree.Savepoint{Name: tree.Name($2)} } | SAVEPOINT error // SHOW HELP: SAVEPOINT @@ -5026,7 +5026,7 @@ rollback_stmt: ROLLBACK opt_to_savepoint { if $2 != "" { - $$.val = &tree.RollbackToSavepoint{Savepoint: $2} + $$.val = &tree.RollbackToSavepoint{Savepoint: tree.Name($2)} } else { $$.val = &tree.RollbackTransaction{} } diff --git a/pkg/sql/pgwire/pgerror/errors.go b/pkg/sql/pgwire/pgerror/errors.go index 09ff0e581c66..09e96146bd7b 100644 --- a/pkg/sql/pgwire/pgerror/errors.go +++ b/pkg/sql/pgwire/pgerror/errors.go @@ -204,6 +204,15 @@ func UnimplementedWithIssueDetailError(issue int, detail, msg string) error { return err.SetHintf("See: https://github.com/cockroachdb/cockroach/issues/%d", issue) } +// UnimplementedWithIssueHintError constructs an error with the given +// message, hint, and a link to the passed issue. Recorded as "#" +// in tracking. +func UnimplementedWithIssueHintError(issue int, msg, hint string) error { + err := NewErrorWithDepthf(1, CodeFeatureNotSupportedError, "unimplemented: %s", msg) + err.InternalCommand = fmt.Sprintf("#%d", issue) + return err.SetHintf("%s\nSee: https://github.com/cockroachdb/cockroach/issues/%d", hint, issue) +} + const unimplementedErrorHint = `This feature is not yet implemented in CockroachDB. Please check https://github.com/cockroachdb/cockroach/issues to check diff --git a/pkg/sql/sem/tree/txn.go b/pkg/sql/sem/tree/txn.go index 0ebe258b7e2d..50544e4fd0cb 100644 --- a/pkg/sql/sem/tree/txn.go +++ b/pkg/sql/sem/tree/txn.go @@ -16,7 +16,6 @@ package tree import ( "fmt" - "strings" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" ) @@ -177,50 +176,35 @@ func (node *RollbackTransaction) Format(ctx *FmtCtx) { ctx.WriteString("ROLLBACK TRANSACTION") } -// RestartSavepointName is the only savepoint name that we accept, modulo -// capitalization. -const RestartSavepointName string = "COCKROACH_RESTART" - -// ValidateRestartCheckpoint checks that a checkpoint name is our magic restart -// value. -// 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 ValidateRestartCheckpoint(savepoint string) error { - if !strings.HasPrefix(strings.ToUpper(savepoint), RestartSavepointName) { - return pgerror.NewErrorf(pgerror.CodeFeatureNotSupportedError, "SAVEPOINT not supported except for %s", RestartSavepointName) - } - return nil -} - // Savepoint represents a SAVEPOINT statement. type Savepoint struct { - Name string + Name Name } // Format implements the NodeFormatter interface. func (node *Savepoint) Format(ctx *FmtCtx) { ctx.WriteString("SAVEPOINT ") - ctx.WriteString(node.Name) + node.Name.Format(ctx) } // ReleaseSavepoint represents a RELEASE SAVEPOINT statement. type ReleaseSavepoint struct { - Savepoint string + Savepoint Name } // Format implements the NodeFormatter interface. func (node *ReleaseSavepoint) Format(ctx *FmtCtx) { ctx.WriteString("RELEASE SAVEPOINT ") - ctx.WriteString(node.Savepoint) + node.Savepoint.Format(ctx) } // RollbackToSavepoint represents a ROLLBACK TO SAVEPOINT statement. type RollbackToSavepoint struct { - Savepoint string + Savepoint Name } // Format implements the NodeFormatter interface. func (node *RollbackToSavepoint) Format(ctx *FmtCtx) { ctx.WriteString("ROLLBACK TRANSACTION TO SAVEPOINT ") - ctx.WriteString(node.Savepoint) + node.Savepoint.Format(ctx) } diff --git a/pkg/sql/sessiondata/session_data.go b/pkg/sql/sessiondata/session_data.go index 712149e4f748..a5c5f88590ec 100644 --- a/pkg/sql/sessiondata/session_data.go +++ b/pkg/sql/sessiondata/session_data.go @@ -82,6 +82,12 @@ type SessionData struct { DurationAdditionMode duration.AdditionMode // Vectorize enables automatic planning of vectorized operators. Vectorize bool + // ForceSavepointRestart overrides the default SAVEPOINT behavior + // for compatibility with certain ORMs. When this flag is set, + // the savepoint name will no longer be compared against the magic + // identifier `cockroach_restart` in order use a restartable + // transaction. + ForceSavepointRestart bool } // DataConversionConfig contains the parameters that influence diff --git a/pkg/sql/txn_restart_test.go b/pkg/sql/txn_restart_test.go index f13a7b562cd8..bb45d1091884 100644 --- a/pkg/sql/txn_restart_test.go +++ b/pkg/sql/txn_restart_test.go @@ -1608,7 +1608,7 @@ 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") { + if !testutils.IsError(err, "SAVEPOINT not supported except for cockroach_restart") { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/sql/txn_state.go b/pkg/sql/txn_state.go index 66d03c7cb445..8fb999203e83 100644 --- a/pkg/sql/txn_state.go +++ b/pkg/sql/txn_state.go @@ -107,6 +107,10 @@ type txnState struct { // txnAbortCount is incremented whenever the state transitions to // stateAborted. txnAbortCount *metric.Counter + + // activeSavepointName stores the name of the active savepoint, + // or is empty if no savepoint is active. + activeSavepointName tree.Name } // txnType represents the type of a SQL transaction. diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index eeb3f70ce28f..e79e1179a4ea 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -24,6 +24,7 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/build" + "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" @@ -417,7 +418,26 @@ var varGen = map[string]sessionVar{ }, GlobalDefault: func(sv *settings.Values) string { return "0" }, }, - + // CockroachDB extension. See docs on SessionData.ForceSavepointRestart. + // https://github.com/cockroachdb/cockroach/issues/30588 + `force_savepoint_restart`: { + Get: func(evalCtx *extendedEvalContext) string { + return formatBoolAsPostgresSetting(evalCtx.SessionData.ForceSavepointRestart) + }, + GetStringVal: makeBoolGetStringValFn("force_savepoint_restart"), + Set: func(_ context.Context, m *sessionDataMutator, val string) error { + b, err := parsePostgresBool(val) + if err != nil { + return err + } + if b { + telemetry.Count("sql.force_savepoint_restart") + } + m.SetForceSavepointRestart(b) + return nil + }, + GlobalDefault: globalFalse, + }, // See https://www.postgresql.org/docs/10/static/runtime-config-preset.html `integer_datetimes`: makeReadOnlyVar("on"), @@ -433,7 +453,6 @@ var varGen = map[string]sessionVar{ return fmt.Sprintf("%d", evalCtx.NodeID) }, }, - // CockroachDB extension (inspired by MySQL). // See https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_sql_safe_updates `sql_safe_updates`: {