Add Inject Retry Errors On Commit variable

[lyang24]

fixes #86370

Release note (sql change): The session variable
inject_retry_errors_on_commit_enabled has been added. When this is true, any
COMMIT statement will return a transaction retry
error if it is run inside of an explicit transaction. The errors will
continue until inject_retry_errors_on_commit_enabled
is set to false. The purpose of this setting is to allow developers to
test their transaction retry logic.

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

• @rafiss (commented on add session variable to emulate retryable exceptions on commit #86370)

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[lyang24]

I believe tests are flakey how do i rerun failed test?

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[lyang24]

Hi @rafiss I mimicked your previous pr can you please some feedbacks when you have time thanks!

[rafiss]

thanks for your contribution! this is the right direction, and i've left a few comments

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lyang24 and @rytaft)

---

-- commits line 2 at r1:
can you add a release note to the commit message. it should say:

Release note (sql change): The session variable
inject_retry_errors_on_commit_enabled has been added. When this is true, any
COMMIT statement will return a transaction retry
error if it is run inside of an explicit transaction. If the client
retries the transaction using the special cockroach_restart SAVEPOINT,
then after the 3rd error, the transaction will proceed as normal.
Otherwise, the errors will continue until inject_retry_errors_enabled
is set to false. The purpose of this setting is to allow developers to
test their transaction retry logic.

---

pkg/sql/conn_executor_exec.go line 939 at r1 (raw file):

		GetIdleLatency(ex.statsCollector.PreviousPhaseTimes())

	isSetOrShow := ast.StatementTag() == "SET" || ast.StatementTag() == "SHOW"

i think this should be changed so that the error is only injected if ast.StatementTag() == "COMMIT"

---

pkg/sql/conn_executor_exec.go line 945 at r1 (raw file):

				ctx, "injected by `inject_retry_errors_on_commit_enabled` session variable")
			return ex.makeErrEvent(retryErr, ast)
		}

similar to the other place where we inject an error, i think this should be changed to be like:

	if ex.state.mu.txn.Epoch() < ex.state.lastEpoch+numTxnRetryErrors {
		retryErr := ex.state.mu.txn.GenerateForcedRetryableError(
			ctx, "injected by `inject_retry_errors_on_commit_enabled` session variable")
		return ex.makeErrEvent(retryErr, ast)
	} else {
		ex.state.lastEpoch = ex.state.mu.txn.Epoch()
	}

this allows the automatic SAVEPOINT retry logic to work (see my suggestion for a test below)

---

pkg/sql/conn_executor_exec.go line 1278 at r1 (raw file):

		// numTxnRetryErrors is the number of times an error will be injected if
		// the transaction is retried using SAVEPOINTs.
		const numTxnRetryErrors = 3

along with my above comment, can you move the initialization of numTxnRetryErrors so that is can be shared between the two functions?

---

pkg/sql/conn_executor_test.go line 1531 at r1 (raw file):

	_, err := db.Exec("SET inject_retry_errors_on_commit_enabled = 'true'")
	require.NoError(t, err)

could you add another test cases that uses savepoints, similar to the with_savepoints subtest in TestInjectRetryErrors?

and could you add another where both inject_retry_errors_on_commit_enabled and inject_retry_errors_enabled are on?

---

pkg/sql/conn_executor_test.go line 1553 at r1 (raw file):

			err = tx.Commit()
			if attemptCount >= 5 {
				require.NoError(t, err)

nit: add a break here. there's no need to keep running the loop after this

---

pkg/sql/conn_executor_test.go line 1560 at r1 (raw file):

				// We should not expect a rollback on commit errors.
			}
		}

add an assertion here for require.Equal(t, 5, txRes)

---

pkg/sql/conn_executor_test.go line 1563 at r1 (raw file):

	})

	t.Run("insert_outside_of_txn", func(t *testing.T) {

you can remove this insert_outside_of_txn test. it's not needed for explicit transactions

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[lyang24]

Hi @rafiss I had some trouble on how to implement retry with save points on commit injected errors. I looked at the code logic even though the errors returned on commit is retryable, the getRewindTxnCapability will always return canAutoRetry = false and according to the state machine the transaction will be moved to no transaction state, rollback to savepoint seems to be impossible in this case.
crdb demo example:

[email protected]:26257/movr  OPEN> commit;                                                                                                                                                                                                                    
ERROR: restart transaction: TransactionRetryWithProtoRefreshError: injected by `inject_retry_errors_on_commit_enabled` session variable
SQLSTATE: 40001
HINT: See: https://www.cockroachlabs.com/docs/v21.2/transaction-retry-error-reference.html
[email protected]:26257/movr>

[rafiss]

Thanks for your explanation! I see the issue now - the auto-retry logic for SAVEPOINTs would only work if the error is injected during the RELEASE TO SAVEPOINT command. However, injecting the error at that point is a bit more complicated.

For now, you can remove the test_injection_failure_on_commit_with_savepoints test you added, and the rest of your change should be good.

[lyang24]

Hi @rafiss, I have remove the savepoint test and all ci checks looks good do you mind to take another look?

[lyang24]

Hi @rafiss I have fixed all the comments do you mind to take another look?

[rafiss]

thanks for your contribution! i will merge this now

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)