sql: add a retry loop for stmts in READ COMMITTED txns by rafiss · Pull Request #107044 · cockroachdb/cockroach

rafiss · 2023-07-18T07:21:27Z

This new loop retries individual statements inside an explicit READ
COMMITTED transaction. This is possible because each statement in a READ
COMMITTED transaction has a different read timestamp. The conn executor
already has retry logic for all implicit transactions, and we continue to
use those when possible.

A session setting controls how many retries will be performed for a statement
inside of an explicit READ COMMITTED transaction.

Release note (sql change): Added the
max_retries_for_read_committed session variable. It
defaults to 10, and determines the number of times an individual
statement in an explicit READ COMMITTED transaction will be retried
if it encounters a retriable transaction error.

cockroach-teamcity · 2023-07-18T07:21:40Z

This change is

fqazi

minus CI not being happy

Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

chengxiong-ruan · 2023-07-18T21:40:54Z

pkg/sql/vars.go

+	// CockroachDB extension. Configures the maximum number of automatic retries
+	// to perform for statements in explicit READ COMMITTED transactions that
+	// see a transaction retry error.
+	`max_retries_for_read_committed_transactions`: {


how about a cluster setting for this purpose?

i lean towards session variable, since cluster settings are generally intended for usage by operators, and require higher permissions on the cluster. also, in this case, it is reasonable that different workloads may want different values here, since it depends on how much contention they see.

Xiang-Gu

Good work!

michae2

Nice work!

Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @fqazi, @nvanbenschoten, and @rafiss)

pkg/sql/conn_executor_exec.go line 963 at r2 (raw file):

	if ex.state.mu.txn.IsOpen() &&
		ex.state.mu.txn.IsoLevel() == isolation.ReadCommitted &&
		!ex.implicitTxn() &&

Do we also want to defer to the state machine retry for the first statement of explicit transactions?

pkg/sql/conn_executor_exec.go line 980 at r2 (raw file):

	}

	maxExecCount := 1

Rather than going through this loop codepath in every case, I think it would be cleaner to put the retry loop in a separate conn executor function that wraps dispatchToExecutionEngine and is only called for read committed transactions. That will do two things: (a) get rid of some of the conditional logic, and (b) add a function to the stack that will be visible when debugging which could help us know we're in a read committed transaction.

pkg/sql/conn_executor_exec.go line 987 at r2 (raw file):

	}

	for attemptNum := 0; attemptNum < maxExecCount; attemptNum++ {

Does this loop need some kind of (randomized) backoff? For example, I think two update statements could both conflict with each other and both need to retry repeatedly.

pkg/sql/conn_executor_exec.go line 1005 at r2 (raw file):

				break
			}
			if !errIsRetriable(maybeRetriableErr) || attemptNum == maxRetries {

Does errIsRetriable return true for any errors that need a full transaction retry (rather than just a statement retry)? If so, we might be performing extra useless statement retries in those cases, when we should go straight to a transaction-level retry.

pkg/sql/vars.go line 2034 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i lean towards session variable, since cluster settings are generally intended for usage by operators, and require higher permissions on the cluster. also, in this case, it is reasonable that different workloads may want different values here, since it depends on how much contention they see.

bikeshedding: I understand that "_transactions" refers to the fact that the whole transaction is at read-committed isolation, but find it a little confusing for this setting. I think it would be clearer to leave it off or change it to "_statements".

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @fqazi, and @nvanbenschoten)

pkg/sql/conn_executor_exec.go line 1019 at r4 (raw file):

			}
			res.SetError(nil)
			ex.state.mu.txn.PrepareForRetry(ctx)

we should wait for kv: remove TxnCoordSender.PrepareRetryableError, rationalize ManualRestart to complete.

rafiss · 2023-08-10T06:49:58Z

For some reason I can't type into Reviewable, but replying here:

Do we also want to defer to the state machine retry for the first statement of explicit transactions?

I don't think so - the state machine doesn't do retries if the BEGIN; and first statement are sent in two different commands. But in this case, we would want to.

Rather than going through this loop codepath in every case, I think it would be cleaner to put the retry loop in a separate conn executor function that wraps dispatchToExecutionEngine and is only called for read committed transactions. That will do two things: (a) get rid of some of the conditional logic, and (b) add a function to the stack that will be visible when debugging which could help us know we're in a read committed transaction.

Very great idea. Done, and hopefully it's simpler to read now.

Does errIsRetriable return true for any errors that need a full transaction retry (rather than just a statement retry)? If so, we might be performing extra useless statement retries in those cases, when we should go straight to a transaction-level retry.

Great catch. I've now rebased on top of #105161 so that I can check with TxnMustRestartFromBeginning.

bikeshedding: I understand that "_transactions" refers to the fact that the whole transaction is at read-committed isolation, but find it a little confusing for this setting. I think it would be clearer to leave it off or change it to "_statements".

I just removed "_transactions" from the end now.

rafiss · 2023-08-17T03:18:29Z

tftrs!

bors r+

craig · 2023-08-17T03:38:33Z

Build failed:

Bazel Essential CI (Cockroach)

This test ensures that we do not do per-statement retries for READ COMMITTED transactions if results were already sent to the client. Release note: None

rafiss · 2023-08-17T04:03:16Z

bors r+

craig · 2023-08-17T04:21:42Z

Build failed:

Bazel Essential CI (Cockroach)

rafiss · 2023-08-17T04:23:10Z

flake was:

   > [internal] load metadata for registry.access.redhat.com/ubi8/ubi-minimal:latest:
  ------
  Dockerfile:1
  --------------------
     1 | >>> FROM registry.access.redhat.com/ubi8/ubi-minimal
     2 |     ARG fips_enabled
     3 |
  --------------------
  ERROR: failed to solve: registry.access.redhat.com/ubi8/ubi-minimal: failed to do request: Head "https://registry.access.redhat.com/v2/ubi8/ubi-minimal/manifests/latest": read tcp 10.142.0.162:46278->23.47.144.132:443: read: connection reset by peer
  + remove_files_on_exit

bors r+

craig · 2023-08-17T04:49:45Z

Build failed:

Bazel Essential CI (Cockroach)

rafiss · 2023-08-17T04:51:20Z

bors r+

craig · 2023-08-17T05:35:08Z

Build succeeded:

Bazel Essential CI (Cockroach)

147682: sql: increase observability of automatic statement retries r=yuzefovich a=michae2 In #107044 we added statement-level automatic retries to the conn executor. These retries are used under Read Committed isolation to try to prevent retryable errors from escaping to the application. This PR adds more observability to statement-level retries, in some cases bringing it up to par with transaction-level retries. See individual commits for details. Informs: #145377 Release note: None Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>

In cockroachdb#107044 we added a retry loop for individual statements executing under Read Committed isolation. We chose 10 as the default retry limit, but testing has shown that this is too low for high-contention workloads. (By comparison, automatic transaction retries in conn_fsm are unlimited.) Increase the default limit to 100. Informs: cockroachdb#145377 Release note (sql change): This change increases the default value for the `max_retries_for_read_committed` session variable from 10 to 100. Testing has shown that some high-contention workloads running under Read Committed isolation benefit from more statement retries.

rafiss force-pushed the retry-read-committed branch from 1d1d2d9 to 0b0b287 Compare July 18, 2023 16:21

rafiss marked this pull request as ready for review July 18, 2023 16:21

rafiss requested review from a team as code owners July 18, 2023 16:21

rafiss requested review from michae2 and nvb July 18, 2023 16:21

rafiss force-pushed the retry-read-committed branch from 0b0b287 to 633add0 Compare July 18, 2023 17:12

fqazi approved these changes Jul 18, 2023

View reviewed changes

rafiss force-pushed the retry-read-committed branch from 633add0 to 9c30984 Compare July 18, 2023 21:27

chengxiong-ruan reviewed Jul 18, 2023

View reviewed changes

Xiang-Gu approved these changes Jul 20, 2023

View reviewed changes

michae2 reviewed Jul 20, 2023

View reviewed changes

rafiss commented Jul 25, 2023

View reviewed changes

michae2 mentioned this pull request Jul 26, 2023

sql: add implicit SELECT FOR SHARE locking to FK checks #105857

Merged

rafiss force-pushed the retry-read-committed branch from 9c30984 to 0c74392 Compare August 10, 2023 06:44

rafiss requested a review from a team as a code owner August 10, 2023 06:44

rafiss force-pushed the retry-read-committed branch from 0c74392 to f4f17c6 Compare August 10, 2023 07:20

rafiss mentioned this pull request Aug 10, 2023

kv: support recovery from retry error with partial rollback under RC #105161

Merged

rafiss force-pushed the retry-read-committed branch from f4f17c6 to 2e3268d Compare August 11, 2023 00:39

rafiss requested review from a team as code owners August 11, 2023 00:39

rafiss requested review from rhu713 and removed request for a team August 11, 2023 00:39

rafiss force-pushed the retry-read-committed branch 4 times, most recently from f81d19e to 7d1895f Compare August 14, 2023 15:04

rafiss force-pushed the retry-read-committed branch from 4e7f87e to 6be2709 Compare August 17, 2023 03:15

sql: add a test for not doing retries if results were sent

0e57591

This test ensures that we do not do per-statement retries for READ COMMITTED transactions if results were already sent to the client. Release note: None

rafiss force-pushed the retry-read-committed branch from 6be2709 to 0e57591 Compare August 17, 2023 04:00

craig bot merged commit 0333b78 into cockroachdb:master Aug 17, 2023

cockroach-teamcity mentioned this pull request Aug 18, 2023

PR #107044 - sql: add a retry loop for stmts in READ COMMITTED txns cockroachdb/docs#17695

Closed

rafiss deleted the retry-read-committed branch August 18, 2023 05:32

rafiss mentioned this pull request Nov 7, 2023

sql: observability for number of per-statement retries under READ COMMITTED #113986

Closed

michae2 mentioned this pull request Jun 3, 2025

sql: increase observability of automatic statement retries #147682

Merged

Conversation

rafiss commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jul 18, 2023

Uh oh!

fqazi left a comment

Choose a reason for hiding this comment

Uh oh!

chengxiong-ruan Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

rafiss Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

Xiang-Gu left a comment

Choose a reason for hiding this comment

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss commented Aug 10, 2023

Uh oh!

rafiss commented Aug 17, 2023

Uh oh!

craig bot commented Aug 17, 2023

Uh oh!

rafiss commented Aug 17, 2023

Uh oh!

craig bot commented Aug 17, 2023

Uh oh!

rafiss commented Aug 17, 2023

Uh oh!

craig bot commented Aug 17, 2023

Uh oh!

rafiss commented Aug 17, 2023

Uh oh!

craig bot commented Aug 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rafiss commented Jul 18, 2023 •

edited

Loading