sql: add a retry loop for stmts in READ COMMITTED txns #107044

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.

nvanbenschoten

You should be able to rebase this on master now.

Reviewed 10 of 13 files at r17, 5 of 5 files at r18, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @michae2, @rafiss, and @rhu713)

pkg/sql/conn_executor_exec.go line 1523 at r14 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

one thought i had is if it would make sense to reuse the autoRetryCounter that we already use the for the conn_executor state machine retries. it would be a slight abuse of the variable, but it actually doesn't seem incorrect to me, and it would be the quickest way to plug into any existing observability that shows retries.

This makes sense to me. I don't think we want to consider this a "transaction retry" and so we will also want to introduce some "statement retry" specific observability, but in existing cases where we are willing to broaden the definitions (e.g. "auto-retry"), it feels reasonable to group this in.

pkg/sql/conn_executor_exec.go line 1560 at r18 (raw file):

			// flushed and sent back to the client already. In that case, we can't
			// retry the statement.
			res.SetError(errors.Wrapf(

Do we have a test that exercises this case?

pkg/sql/tests/read_committed_test.go line 58 at r18 (raw file):

				// the second read committed write begins, the read committed scans
				// will have finished.
				if readCommittedWriteCount.Load() == 2 {

Instead of re-loading, do we want to use the result of the Add? Right now, this is racy.

108728: build: add basic infrastructure for remote execution with EngFlow r=rail a=rickystewart Add the `--config engflow` which sets some appropriate configurations for building against our engflow cluster, and set some other metadata. Also bump some test sizes and shard counts to get everything working. Epic: [CRDB-8308](https://cockroachlabs.atlassian.net/browse/CRDB-8308) Release note: None 108817: sql: simplifiy tracking of injected txn retry errors r=rafiss a=rafiss Rather than using the txn epoch, we can just track how many errors were injected. This lets us have a bit more control over how many errors to inject, without having to rely on how the KV layer handles different types of transaction retries. informs: #100145 split off from: #107044 Release note: None Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>

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.

Release note: None

rafiss

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

pkg/sql/conn_executor_exec.go line 1560 at r18 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we have a test that exercises this case?

added a test

pkg/sql/tests/read_committed_test.go line 58 at r18 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Instead of re-loading, do we want to use the result of the Add? Right now, this is racy.

good point; fixed

nvanbenschoten

We might also want one person from SQL to sign off on the changes as well.

Reviewed 21 of 25 files at r20, 2 of 2 files at r21, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @michae2, @rafiss, and @rhu713)

michae2

Very nice! 🍾

I left some comments about tests that you can ignore.

Reviewed 10 of 13 files at r17, 25 of 25 files at r19, 2 of 25 files at r20, 1 of 2 files at r21, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan, @rafiss, and @rhu713)

pkg/sql/conn_executor_test.go line 1442 at r20 (raw file):

	params := base.TestServerArgs{}

	var readCommittedStmtRetries int

Does this also need to be an atomic?

pkg/sql/tests/read_committed_test.go line 49 at r20 (raw file):

	filterFunc := func(ctx context.Context, ba *kvpb.BatchRequest) *kvpb.Error {
		if ba.Txn == nil || ba.Txn.IsoLevel != isolation.ReadCommitted {

I'm worried that if we ever add internal queries running at ReadCommitted isolation they will cause this test to flake because we're looking for any Put at RC isolation. I don't think we need to make it bulletproof now, but maybe a comment would help someone fix the hypothetical flake one day.

rafiss

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

pkg/sql/conn_executor_test.go line 1442 at r20 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Does this also need to be an atomic?

hmm it might be safer. will do

pkg/sql/tests/read_committed_test.go line 49 at r20 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I'm worried that if we ever add internal queries running at ReadCommitted isolation they will cause this test to flake because we're looking for any Put at RC isolation. I don't think we need to make it bulletproof now, but maybe a comment would help someone fix the hypothetical flake one day.

i think i can easily change this so it only counts writes to the table in this test

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)

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 nvanbenschoten 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 requested a review from a team as a code owner August 15, 2023 21:54

rafiss mentioned this pull request Aug 15, 2023

sql: simplifiy tracking of injected txn retry errors #108817

Merged

nvanbenschoten reviewed Aug 16, 2023

View reviewed changes

rafiss added 2 commits August 16, 2023 15:16

sql/tests: add a test for WriteTooOldError under READ COMMITTED

6ac84c4

Release note: None

rafiss force-pushed the retry-read-committed branch from 6c37b4c to 4e7f87e Compare August 16, 2023 20:10

rafiss commented Aug 16, 2023

View reviewed changes

nvanbenschoten approved these changes Aug 16, 2023

View reviewed changes

michae2 approved these changes Aug 16, 2023

View reviewed changes

rafiss commented Aug 16, 2023

View reviewed changes

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
6 of 7 checks passed

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

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add a retry loop for stmts in READ COMMITTED txns #107044

sql: add a retry loop for stmts in READ COMMITTED txns #107044

rafiss commented Jul 18, 2023 •

edited

cockroach-teamcity commented Jul 18, 2023

fqazi left a comment

chengxiong-ruan Jul 18, 2023

rafiss Jul 19, 2023

Xiang-Gu left a comment

michae2 left a comment

rafiss left a comment

rafiss commented Aug 10, 2023

nvanbenschoten left a comment

rafiss left a comment

nvanbenschoten left a comment

michae2 left a comment

rafiss left a comment

rafiss commented Aug 17, 2023

craig bot commented Aug 17, 2023

rafiss commented Aug 17, 2023

craig bot commented Aug 17, 2023

rafiss commented Aug 17, 2023

craig bot commented Aug 17, 2023

rafiss commented Aug 17, 2023

craig bot commented Aug 17, 2023

sql: add a retry loop for stmts in READ COMMITTED txns #107044

sql: add a retry loop for stmts in READ COMMITTED txns #107044

Conversation

rafiss commented Jul 18, 2023 • edited

cockroach-teamcity commented Jul 18, 2023

fqazi left a comment

Choose a reason for hiding this comment

chengxiong-ruan Jul 18, 2023

Choose a reason for hiding this comment

rafiss Jul 19, 2023

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

michae2 left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss commented Aug 10, 2023

nvanbenschoten left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

michae2 left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss commented Aug 17, 2023

craig bot commented Aug 17, 2023

rafiss commented Aug 17, 2023

craig bot commented Aug 17, 2023

rafiss commented Aug 17, 2023

craig bot commented Aug 17, 2023

rafiss commented Aug 17, 2023

craig bot commented Aug 17, 2023

rafiss commented Jul 18, 2023 •

edited