From 1974f924eb2c4c9aea2638100d71fa1cace9dc17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Wed, 14 Sep 2022 07:49:09 +0200 Subject: [PATCH 1/2] fix: retries of updates in the Connection API ignored analyze mode Retries of read/write transactions in the Connection API did not respect the analyze mode of update statements. This would cause updates to be executed using AnalyzeMode.NORMAL during retries, regardless of what was used during the initial attempt. Fixes #2009 --- .../connection/ReadWriteTransaction.java | 6 +-- .../spanner/connection/RetriableUpdate.java | 13 ++++++- .../cloud/spanner/connection/AbortedTest.java | 38 +++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java index 93d38f17935..968ef357c24 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java @@ -444,7 +444,7 @@ private ApiFuture internalExecuteUpdateAsync( options); } createAndAddRetriableUpdate( - update, updateCount.getRowCountExact(), options); + update, analyzeMode, updateCount.getRowCountExact(), options); return updateCount; } catch (AbortedException e) { throw e; @@ -712,9 +712,9 @@ private void createAndAddFailedQuery( } private void createAndAddRetriableUpdate( - ParsedStatement update, long updateCount, UpdateOption... options) { + ParsedStatement update, AnalyzeMode analyzeMode, long updateCount, UpdateOption... options) { if (retryAbortsInternally) { - addRetryStatement(new RetriableUpdate(this, update, updateCount, options)); + addRetryStatement(new RetriableUpdate(this, update, analyzeMode, updateCount, options)); } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/RetriableUpdate.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/RetriableUpdate.java index 3818bdd739c..011bed69cbc 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/RetriableUpdate.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/RetriableUpdate.java @@ -31,18 +31,21 @@ final class RetriableUpdate implements RetriableStatement { private final ReadWriteTransaction transaction; private final ParsedStatement statement; + private final AnalyzeMode analyzeMode; private final long updateCount; private final UpdateOption[] options; RetriableUpdate( ReadWriteTransaction transaction, ParsedStatement statement, + AnalyzeMode analyzeMode, long updateCount, UpdateOption... options) { Preconditions.checkNotNull(transaction); Preconditions.checkNotNull(statement); this.transaction = transaction; this.statement = statement; + this.analyzeMode = analyzeMode; this.updateCount = updateCount; this.options = options; } @@ -54,7 +57,15 @@ public void retry(AbortedException aborted) throws AbortedException { transaction .getStatementExecutor() .invokeInterceptors(statement, StatementExecutionStep.RETRY_STATEMENT, transaction); - newCount = transaction.getReadContext().executeUpdate(statement.getStatement(), options); + if (analyzeMode == null || analyzeMode == AnalyzeMode.NONE) { + newCount = transaction.getReadContext().executeUpdate(statement.getStatement(), options); + } else { + newCount = + transaction + .getReadContext() + .analyzeUpdate(statement.getStatement(), analyzeMode.getQueryAnalyzeMode()) + .getRowCountExact(); + } } catch (AbortedException e) { // Just re-throw the AbortedException and let the retry logic determine whether another try // should be executed or not. diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java index 274f6d27304..96416c3e8d5 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbortedTest.java @@ -26,6 +26,7 @@ import com.google.cloud.Timestamp; import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult; +import com.google.cloud.spanner.ReadContext.QueryAnalyzeMode; import com.google.cloud.spanner.ResultSet; import com.google.cloud.spanner.SpannerException; import com.google.cloud.spanner.Statement; @@ -37,9 +38,11 @@ import com.google.spanner.v1.CommitRequest; import com.google.spanner.v1.ExecuteBatchDmlRequest; import com.google.spanner.v1.ExecuteSqlRequest; +import com.google.spanner.v1.ExecuteSqlRequest.QueryMode; import io.grpc.Status; import io.grpc.StatusRuntimeException; import java.util.Collections; +import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -299,6 +302,41 @@ public void testRetryUsesTags() { assertEquals(2L, commitRequestCount); } + @Test + public void testRetryUsesAnalyzeModeForUpdate() { + mockSpanner.putStatementResult( + StatementResult.query(SELECT_COUNT_STATEMENT, SELECT_COUNT_RESULTSET_BEFORE_INSERT)); + mockSpanner.putStatementResult(StatementResult.update(INSERT_STATEMENT, 0)); + try (ITConnection connection = createConnection()) { + assertEquals( + 0L, connection.analyzeUpdate(INSERT_STATEMENT, QueryAnalyzeMode.PLAN).getRowCountExact()); + + mockSpanner.abortNextStatement(); + connection.executeQuery(SELECT_COUNT_STATEMENT); + + mockSpanner.putStatementResult(StatementResult.update(INSERT_STATEMENT, 1)); + assertEquals(1L, connection.executeUpdate(INSERT_STATEMENT)); + + connection.commit(); + } + // 5 requests because: + // 1. Analyze INSERT + // 2. Execute SELECT COUNT(*) (Aborted) + // 3. Analyze INSERT (retry) + // 4. Execute SELECT COUNT(*) (retry) + // 5. Execute INSERT + assertEquals(5, mockSpanner.countRequestsOfType(ExecuteSqlRequest.class)); + List requests = mockSpanner.getRequestsOfType(ExecuteSqlRequest.class); + assertEquals(QueryMode.PLAN, requests.get(0).getQueryMode()); + assertEquals(QueryMode.NORMAL, requests.get(1).getQueryMode()); + + // This used NORMAL because of https://github.com/googleapis/java-spanner/issues/2009. + assertEquals(QueryMode.PLAN, requests.get(2).getQueryMode()); + + assertEquals(QueryMode.NORMAL, requests.get(3).getQueryMode()); + assertEquals(QueryMode.NORMAL, requests.get(4).getQueryMode()); + } + ITConnection createConnection(TransactionRetryListener listener) { ITConnection connection = super.createConnection(ImmutableList.of(), ImmutableList.of(listener)); From 581ef182833f4aff2cee365d7505556a1bce53c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Wed, 14 Sep 2022 13:12:46 +0200 Subject: [PATCH 2/2] fix: remove null check and force not-null --- .../cloud/spanner/connection/RetriableUpdate.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/RetriableUpdate.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/RetriableUpdate.java index 011bed69cbc..68568a4428e 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/RetriableUpdate.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/RetriableUpdate.java @@ -41,11 +41,9 @@ final class RetriableUpdate implements RetriableStatement { AnalyzeMode analyzeMode, long updateCount, UpdateOption... options) { - Preconditions.checkNotNull(transaction); - Preconditions.checkNotNull(statement); - this.transaction = transaction; - this.statement = statement; - this.analyzeMode = analyzeMode; + this.transaction = Preconditions.checkNotNull(transaction); + this.statement = Preconditions.checkNotNull(statement); + this.analyzeMode = Preconditions.checkNotNull(analyzeMode); this.updateCount = updateCount; this.options = options; } @@ -57,7 +55,7 @@ public void retry(AbortedException aborted) throws AbortedException { transaction .getStatementExecutor() .invokeInterceptors(statement, StatementExecutionStep.RETRY_STATEMENT, transaction); - if (analyzeMode == null || analyzeMode == AnalyzeMode.NONE) { + if (analyzeMode == AnalyzeMode.NONE) { newCount = transaction.getReadContext().executeUpdate(statement.getStatement(), options); } else { newCount =