Handle the case where RELEASE SAVEPOINT fails with a retry

e2e/newenemy/newenemy_test.go

@@ -110,7 +110,7 @@ func TestNoNewEnemy(t *testing.T) {
 
 	t.Log("filling with data to span multiple ranges")
 	rand.Seed(time.Now().UnixNano())
-	fill(vulnerableSpiceDb[0].Client().V0().ACL(), 4000, 100)
+	fill(require, vulnerableSpiceDb[0].Client().V0().ACL(), 4000, 100)
 
 	const sampleSize = 5
 	samples := make([]int, sampleSize)
@@ -249,29 +249,25 @@ func BenchmarkConflictingTupleWrites(b *testing.B) {
 	require.NoError(b, spicedb.Connect(ctx, os.Stdout))
 
 	// fill with tuples to ensure we span multiple ranges
-	fill(spicedb[0].Client().V0().ACL(), 2000, 100)
+	fill(require.New(b), spicedb[0].Client().V0().ACL(), 2000, 100)
 
 	b.ResetTimer()
 
 	checkNoNewEnemy(ctx, b, spicedb, b.N)
 }
 
-func fill(client v0.ACLServiceClient, fillerCount, batchSize int) {
+func fill(require *require.Assertions, client v0.ACLServiceClient, fillerCount, batchSize int) {
 	directs, excludes := generateTuples(fillerCount)
 	for i := 0; i < fillerCount/batchSize; i++ {
 		fmt.Println("filling ", i*batchSize, "to", (i+1)*batchSize)
 		_, err := client.Write(testCtx, &v0.WriteRequest{
 			Updates: excludes[i*batchSize : (i+1)*batchSize],
 		})
-		if err != nil {
-			fmt.Println(err)
-		}
+		require.NoError(err)
 		_, err = client.Write(testCtx, &v0.WriteRequest{
 			Updates: directs[i*batchSize : (i+1)*batchSize],
 		})
-		if err != nil {
-			fmt.Println(err)
-		}
+		require.NoError(err)
 	}
 }
 

internal/datastore/crdb/tx.go

@@ -66,8 +66,25 @@ func execute(ctx context.Context, conn conn, txOptions pgx.TxOptions, fn transac
 	defer func() {
 		retryHistogram.Observe(float64(i))
 	}()
+
+	releasedFn := func(tx pgx.Tx) error {
+		if err := fn(tx); err != nil {
+			return err
+		}
+
+		// RELEASE acts like COMMIT in CockroachDB. We use it since it gives us an
+		// opportunity to react to retryable errors, whereas tx.Commit() doesn't.
+
+		// RELEASE SAVEPOINT itself can fail, in which case the entire
+		// transaction needs to be retried
+		if _, err := tx.Exec(ctx, "RELEASE SAVEPOINT cockroach_restart"); err != nil {
+			return err
+		}
+		return nil
+	}
+
 	for i = 0; i < maxRetries; i++ {
-		if err = fn(tx); err != nil {
+		if err = releasedFn(tx); err != nil {
 			if !retriable(err) {
 				return err
 			}
@@ -76,12 +93,7 @@ func execute(ctx context.Context, conn conn, txOptions pgx.TxOptions, fn transac
 			}
 			continue
 		}
-
-		// RELEASE acts like COMMIT in CockroachDB. We use it since it gives us an
-		// opportunity to react to retryable errors, whereas tx.Commit() doesn't.
-		if _, err = tx.Exec(ctx, "RELEASE SAVEPOINT cockroach_restart"); err == nil {
-			return nil
-		}
+		return nil
 	}
 	return errors.New(errReachedMaxRetry)
 }