sql: SAVEPOINT cockroach_restart can be erroneously called after having run other SQL statements when using DistSQL
#15012
Assignees
Labels
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Milestone
Comments
andreimatei
added
the
C-bug
|
@andreimatei I think this has been fixed, correct? |
|
Incorrect, situation looks the same to me. |
|
Please note that some users currently require being able to make metadata requests against virtual tables before issuing the savepoint. Any solution to this should preserve this behavior. |
I don't think this is currently supported though. "Make metadata requests" requires KV operations, which in turn engages the retry logic. |
craig bot
pushed a commit
that referenced
this issue
Nov 13, 2018
31971: sql: Allow restart savepoint to be renamed r=bobvawter a=bobvawter 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. Co-authored-by: Bob Vawter <bob@cockroachlabs.com>
|
@knz has this been fixed? |
|
yes correct I fixed that. thanks for noticing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We enforce that SAVEPOINT is the first statement in a txn (or in a retry of a txn) by looking at
client.Txn.CommandCount(). If the previous statements usedDistSQL, thatCommandCountis not necessarily updated.We should get rid of
CommandCountand track the status of the txn exclusively at the sql level.The text was updated successfully, but these errors were encountered: