Flexible retry backoff policies by MattWhelan · Pull Request #182 · cockroachdb/cockroach-go

MattWhelan · 2025-06-09T23:17:08Z

Extends the existing retry limit mechanism to allow for delays between retries. Includes implementations for fixed delays and exponential backoff.

cockroach-teamcity · 2025-06-09T23:17:16Z

This change is

dikshant · 2025-06-10T20:58:05Z

We haven’t implemented this because cockroachdb-go uses save point based retries and because of savepoint-based retries, a transaction can continue holding locks in between retries. So that means with exponential backoff, you'll be holding locks for longer, thus putting the app at even greater risk of contention.

If you could show us that this is not the case with backoffs, we will consider adding this! Thanks!

MattWhelan · 2025-06-10T22:13:21Z

We haven’t implemented this because cockroachdb-go uses save point based retries and because of savepoint-based retries, a transaction can continue holding locks in between retries. So that means with exponential backoff, you'll be holding locks for longer, thus putting the app at even greater risk of contention.

If you could show us that this is not the case with backoffs, we will consider adding this! Thanks!

I just pushed 9dda97b. Could you take a look at that? I'm not sure what all the implications are of separating the database transaction lifecycle from the Tx object, but aside from that, I think it's sound, and avoids the problem with holding locks during backoff.

rafiss · 2025-07-03T21:26:07Z

+}
+
+// Vargo converts a go-retry style Delay provider into a RetryPolicy
+func Vargo(fn func() VargoBackoff) RetryPolicy {


I'm wondering if we could think of a different name to go with here. Users of cockroach-go who don't already know about the sethvargo/go-retry library may be confused to see this.

I'm open to suggestions. I was trying to build an explicit integration without adding a transitive dependency.

@sravotto , how about we rename this function Vargo -> ExternalBackoffPolicy.

I think the name would be a bit more discoverable this way.

rafiss · 2025-07-03T21:36:55Z

+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)
+			}
+			if restartErr := tx.Exec(ctx, "BEGIN"); restartErr != nil {


This seems like it could have issues. Some libraries may not like it if we use the transaction after rolling it back. For example, I see this in the pgx code where it returns an error if you try to use a transaction that was already rolled back: https://github.com/jackc/pgx/blob/ecc9203ef42fbba50507e773901b5aead75288ef/tx.go#L205-L224

It returns an error if you use a transaction object that has the closed flag set. Since we're not doing that, it ought to be ok, I would expect.

Extends the existing retry limit mechanism to allow for delays between retries. Includes implementations for fixed delays and exponential backoff.

rafiss · 2025-10-21T15:27:41Z

 // WithMaxRetries configures context so that ExecuteTx retries tx specified
 // number of times when encountering retryable errors.
-// Setting retries to 0 will retry indefinitely.
+// Setting retries to 0 will not retry: the transaction will be tried only once.


I think we should try to avoid breaking the API here. This changes the behavior of WithMaxRetries(ctx, 0) from "retry indefinitely" to "no retries," which would affect any existing code relying on this.

Maybe we could add a separate WithNoRetries() function, and change something about the internal representation so that the outward facing API remains the same.

Agree. Done.

rafiss · 2025-10-21T15:30:55Z

+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)
+			}
+			if restartErr := tx.Exec(ctx, "BEGIN"); restartErr != nil {


This still doesn't seem valid to me -- if we reuse the same tx object here, it will not succeed when sending BEGIN right after a ROLLBACK. (see https://github.com/jackc/pgx/blob/ecc9203ef42fbba50507e773901b5aead75288ef/tx.go#L205-L224)

My understanding is that we not calling tx.Rollback, but rather tx.Exec(ctx, "ROLLBACK"). Please check the comments in the code, and let me know if that makes sense.

rafiss · 2025-10-21T15:32:28Z

-			return newTxnRestartError(rollbackErr, err)
+		// We have a retryable error. Check the retry policy.
+		delay, retryErr := retryFunc(err)
+		if delay > 0 && retryErr == nil {


I don't follow this. Why are we doing a retry if retryErr is nil?

I added documentation on the RetryFunc function definition to clarify the semantics. Implementation of RetryFunc may return errors (typically MaxRetriesExceededError) to stop the retry process.

rafiss · 2025-10-21T15:35:04Z

+		if delay > 0 && retryErr == nil {
+			// We don't want to hold locks while waiting for a backoff, so restart the entire transaction
+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)


This is the wrong error to return here -- as per this line, when this error is displayed, it will say that there was an issue with ROLLBACK TO SAVEPOINT, which is not what this line is doing:

cockroach-go/crdb/error.go

Line 70 in c787987

const msgPattern = "restarting txn failed. ROLLBACK TO SAVEPOINT " +

(This is also the wrong error to return on lines 98 and 101.)

Done. Passing the operation to newTxnRestartError.

rafiss · 2025-10-21T15:37:11Z

+		}
+	}
+}
+


I think we should also have tests for edge cases like zero BaseDelay, negative RetryLimit, and MaxDelay < BaseDelay.

rafiss · 2025-10-21T15:38:07Z

-		if rollbackErr := tx.Exec(ctx, "ROLLBACK TO SAVEPOINT cockroach_restart"); rollbackErr != nil {
-			return newTxnRestartError(rollbackErr, err)
+		// We have a retryable error. Check the retry policy.
+		delay, retryErr := retryFunc(err)


before we enter the retry logic, we should check for context cancellation:

if err := ctx.Err(); err != nil { return err }

rafiss · 2025-10-21T15:39:23Z

+	NewRetry() RetryFunc
+}
+
+type LimitBackoffRetryPolicy struct {


This is public-facing so should have a comment explaining what it is and how to use it.

rafiss · 2025-10-21T15:39:57Z

+}
+
+// ExpBackoffRetryPolicy implements RetryPolicy using an exponential backoff with optional
+// saturation.


I don't follow what "saturation" means here. Can we write this comment to explain more how to use this?

Done, reworded comment.

rafiss · 2025-10-21T15:42:02Z

+	actualDelays := make([]time.Duration, 0, len(expectedDelays))
+	rf := policy.NewRetry()
+	for {
+		delay, err := rf(nil)


We're always passing a nil error here -- can we add tests that check the behavior with a non-nil error?

Document LimitBackoffRetryPolicy, ExpBackoffRetryPolicy, and Vargo adapter with detailed examples. Preserve backward compatibility by making WithMaxRetries(ctx, 0) mean unlimited retries (original behavior). Add WithNoRetries() for disabling retries and introduce sentinel constants. Enhance RetryFunc documentation to clarify return value semantics and add additional test cases.

sravotto

Thanks for the review. Please take another look (will squash the commits once we are done).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @sean-)

sravotto · 2025-10-23T18:06:34Z

-		if rollbackErr := tx.Exec(ctx, "ROLLBACK TO SAVEPOINT cockroach_restart"); rollbackErr != nil {
-			return newTxnRestartError(rollbackErr, err)
+		// We have a retryable error. Check the retry policy.
+		delay, retryErr := retryFunc(err)


sravotto · 2025-10-23T18:06:34Z

-			return newTxnRestartError(rollbackErr, err)
+		// We have a retryable error. Check the retry policy.
+		delay, retryErr := retryFunc(err)
+		if delay > 0 && retryErr == nil {


I added documentation on the RetryFunc function definition to clarify the semantics. Implementation of RetryFunc may return errors (typically MaxRetriesExceededError) to stop the retry process.

sravotto · 2025-10-23T18:06:35Z

+		if delay > 0 && retryErr == nil {
+			// We don't want to hold locks while waiting for a backoff, so restart the entire transaction
+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)


Done. Passing the operation to newTxnRestartError.

sravotto · 2025-10-23T18:06:35Z

+			if restartErr := tx.Exec(ctx, "ROLLBACK"); restartErr != nil {
+				return newTxnRestartError(restartErr, err)
+			}
+			if restartErr := tx.Exec(ctx, "BEGIN"); restartErr != nil {


My understanding is that we not calling tx.Rollback, but rather tx.Exec(ctx, "ROLLBACK"). Please check the comments in the code, and let me know if that makes sense.

sravotto · 2025-10-23T18:06:36Z

+	NewRetry() RetryFunc
+}
+
+type LimitBackoffRetryPolicy struct {


sravotto · 2025-10-23T18:06:36Z

+}
+
+// ExpBackoffRetryPolicy implements RetryPolicy using an exponential backoff with optional
+// saturation.


Done, reworded comment.

sravotto · 2025-10-23T18:06:37Z

+	actualDelays := make([]time.Duration, 0, len(expectedDelays))
+	rf := policy.NewRetry()
+	for {
+		delay, err := rf(nil)


sravotto · 2025-10-23T18:06:37Z

+		}
+	}
+}
+


sravotto · 2025-10-23T18:06:38Z

 // WithMaxRetries configures context so that ExecuteTx retries tx specified
 // number of times when encountering retryable errors.
-// Setting retries to 0 will retry indefinitely.
+// Setting retries to 0 will not retry: the transaction will be tried only once.


Agree. Done.

rafiss

Thanks for addressing those comments.

I gave one final pass, and the only remaining comments I have are renames and simple check for handling infinite retries. I'll mark this as approved, so feel free to merge after addressing those comments.

rafiss · 2025-12-04T03:47:59Z

+}
+
+// Vargo converts a go-retry style Delay provider into a RetryPolicy
+func Vargo(fn func() VargoBackoff) RetryPolicy {


@sravotto , how about we rename this function Vargo -> ExternalBackoffPolicy.

I think the name would be a bit more discoverable this way.

…icies Update ExpBackoffRetryPolicy to handle RetryLimit consistently with LimitBackoffRetryPolicy: 0 now means unlimited retries, negative values mean no retries. Rename vargoAdapter to ExternalBackoff-related names for clarity and consistency with the public API. i Add comprehensive test coverage for NoRetries, UnlimitedRetries, and negative RetryLimit values with ExpBackoffRetryPolicy.

sravotto · 2025-12-04T15:49:37Z

Thanks for the review.

MattWhelan force-pushed the backoff branch 2 times, most recently from 3034fdf to d7039d1 Compare June 10, 2025 15:32

MattWhelan requested a review from sean- June 10, 2025 16:01

MattWhelan force-pushed the backoff branch from d7039d1 to 4f09d74 Compare June 10, 2025 16:33

MattWhelan requested a review from rafiss June 23, 2025 20:05

rafiss reviewed Jul 3, 2025

View reviewed changes

Matt Whelan added 2 commits October 21, 2025 14:33

Flexible retry backoff policies

0a27be5

Extends the existing retry limit mechanism to allow for delays between retries. Includes implementations for fixed delays and exponential backoff.

Avoid SAVEPOINT with backoff

d408129

sravotto force-pushed the backoff branch from 79348da to d408129 Compare October 21, 2025 12:33

rafiss requested changes Oct 21, 2025

View reviewed changes

sravotto force-pushed the backoff branch from 782ff75 to 93bb226 Compare October 23, 2025 17:16

sravotto reviewed Oct 23, 2025

View reviewed changes

rafiss approved these changes Dec 4, 2025

View reviewed changes

sravotto force-pushed the backoff branch from 2483c43 to b4539af Compare December 4, 2025 15:31

sravotto merged commit 319b7f9 into cockroachdb:master Dec 4, 2025
3 checks passed

Conversation

MattWhelan commented Jun 9, 2025

Uh oh!

cockroach-teamcity commented Jun 9, 2025

Uh oh!

dikshant commented Jun 10, 2025

Uh oh!

MattWhelan commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sravotto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment