sql: misc savepoint fixes #46415

knz · 2020-03-23T11:56:00Z

Fixes #46372

Release justification: bug fixes and low-risk updates to new functionality

These commits correct a few gaps in the recent implementation.

cockroach-teamcity · 2020-03-23T11:56:10Z

This change is

andreimatei

LGTM mod the comment about doing the ManualRestart in the Aborted state too

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @knz)

pkg/sql/conn_executor_savepoints.go, line 207 at r1 (raw file):

	// cockroach_restart using ignored seqnum lists so we need to
	// restart the txn the "old way".
	// TODO(knz): Remove this check in v20.2 and only keep the 'else'

nit: I'd get rid of the TODO. It's implied by all the version gates.

pkg/sql/conn_executor_savepoints.go, line 209 at r1 (raw file):

	// TODO(knz): Remove this check in v20.2 and only keep the 'else'
	// clause, which is the generic rollback code.
	if entry.kvToken.Initial() && !ex.server.cfg.Settings.Version.IsActive(ctx, clusterversion.VersionSavepoints) {

long line

pkg/sql/conn_executor_savepoints.go, line 210 at r1 (raw file):

	// clause, which is the generic rollback code.
	if entry.kvToken.Initial() && !ex.server.cfg.Settings.Version.IsActive(ctx, clusterversion.VersionSavepoints) {
		// Bump the epoch but keep the kv txn.

nit: I'd nix the "keep the kv txn" part. not "keeping the kv txn" is not something we do in related circumstances.

pkg/sql/conn_executor_savepoints.go, line 211 at r1 (raw file):

	if entry.kvToken.Initial() && !ex.server.cfg.Settings.Version.IsActive(ctx, clusterversion.VersionSavepoints) {
		// Bump the epoch but keep the kv txn.
		ex.state.mu.txn.ManualRestart(ctx, hlc.Timestamp{})

We need to do the same in the aborted state, don't we? To cover the case where we got there through a non-retriable error.

pkg/sql/conn_executor_savepoints.go, line 223 at r3 (raw file):

}

func (ex *connExecutor) checkRollbackValidity(

pls add a comment to this method

pkg/sql/conn_executor_savepoints.go, line 226 at r3 (raw file):

	ctx context.Context, s *tree.RollbackToSavepoint, entry *savepoint,
) (ev fsm.Event, payload fsm.EventPayload, ok bool) {
	if ex.extraTxnState.numDDL > entry.numDDL {

nit: invert this condition to return early and unindent the rest

In mixed v19.2/v20.1 clusters, the v19.2 nodes don't know what to do with the ignored seqnum list, so we can't let v20.1 nodes use the savepoint code just yet. This patch adds the corresponding cluster version check. Release justification: bug fixes and low-risk updates to new functionality Release note: None

See issue cockroachdb#46414 Because lease acquisition must operate at a high prio than schema changes, and SQL "HIGH PRIORITY" operates at the same prio as lease acquisition, we can't let ROLLBACK TO SAVEPOINT after schema change in HIGH PRIORITY txns. Release justification: fixes for high-priority or high-severity bugs in existing functionality Release note (sql change): ROLLBACK TO SAVEPOINT (for either regular savepoint or "restart savepoints" defined with `cockroach_restart`) causes a "feature not supported" error after a DDL statement in a HIGH PRIORITY transaction, in order to avoid a transaction deadlock. See issue cockroachdb#46414 for details.

Prior to this patch, the two checks on whether ROLLBACK TO SAVEPOINT was allowed were only ran when the txn was in the open state (no error). This patch complements this by ensuring the checks are also ran when the txn is in the aborted state. Release justification: bug fixes and low-risk updates to new functionality Release note: None

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)

pkg/sql/conn_executor_savepoints.go, line 207 at r1 (raw file):