feat: delay transaction start option #2462

olavloite · 2023-05-23T08:25:08Z

Adds an opt-in to delay the actual start of a read/write transaction until the first write operation. This reduces lock contention and can reduce latency for read/write transactions that execute (many) read operations before any write operations, at the expense of a lower transaction isolation level. Typical workloads that benefit from this option are ORMs (e.g. Hibernate). The application must be able to handle the lower isolation level.

This feature will automatically propagate into clients that use the Connection API, such as the JDBC driver and PGAdapter.

Adds an opt-in to delay the actual start of a read/write transaction until the first write operation. This reduces lock contention and can reduce latency for read/write transactions that execute (many) read operations before any write operations, at the expense of a lower transaction isolation level. Typical workloads that benefit from this option are ORMs (e.g. Hibernate). The application must be able to handle the lower isolation level.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

thiagotnunes · 2023-05-31T01:35:19Z

...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java

@@ -225,13 +237,22 @@ public boolean isReadOnly() {
  }

  @Override
-  void checkValidTransaction() {
+  void checkValidTransaction(ParsedStatement statement) {


nit: checkValidationTransaction mutates state within the transaction (it did before as well with the future), so perhaps it is a good chance to rename the method? (very nitty, so feel free to ignore)

That's indeed a good point. Renamed to checkOrCreateValidTransaction.

thiagotnunes · 2023-05-31T01:37:08Z

...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java

@@ -622,7 +651,8 @@ public void onSuccess(long[] result) {}
  @Override
  public ApiFuture<Void> writeAsync(Iterable<Mutation> mutations) {
    Preconditions.checkNotNull(mutations);
-    checkValidTransaction();
+    // We actually don't need a transaction yet, as mutations are buffered until commit.
+    checkValidTransaction(null);


nit: can we avoid passing null here? Do we need this call to set the transactionStarted or can we avoid this call?

We need to set the transactionStarted, and we need to verify that the transaction is in a valid state, so we can't skip it altogether. Also; the method takes a ParsedStatement as an input argument so it can check the type of statement, and we don't have a SQL statement that is equivalent with writing/buffering mutations.

What we can however do is slightly rearrange the methods so we have a separate method only for verifying the valid state of the transaction and marking it as started, but skipping the creation of the underlying transaction. I've changed it to that.

thiagotnunes · 2023-05-31T01:39:40Z

...le-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ReadWriteTransaction.java

    state = UnitOfWorkState.COMMITTING;
    commitResponseFuture = SettableApiFuture.create();
    ApiFuture<Void> res;
-    if (retryAbortsInternally) {
+    // Check if this transaction actually needs to commit anything.
+    if (txContextFuture == null) {


Just double checking my understanding: checkValidTransaction(COMMIT_STATEMENT) will initialize the txContextFuture here, so it won't be null for blind writes, correct?

Correct, although it will only be initialized for blind writes if the transaction actually has any mutations. So a transaction that does not do anything except calling Commit will be a no-op (it already was).

thiagotnunes · 2023-05-31T01:40:45Z

...-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/AbstractMockServerTest.java

@@ -114,6 +114,8 @@ public abstract class AbstractMockServerTest {
      Statement.of("INSERT INTO TEST (ID, NAME) VALUES (1, 'test aborted')");
  public static final Statement INSERT_RETURNING_STATEMENT =
      Statement.of("INSERT INTO TEST (ID, NAME) VALUES (1, 'test aborted') THEN RETURN *");
+  public static final Statement PG_INSERT_RETURNING_STATEMENT =


Unrelated to delayed transactions?

I added this so we could include tests that execute DML with a RETURNING clause in both dialects. The reason that it is relevant is that these statements are executed using the executeQuery method, and we should ensure that these statements trigger the start of the underlying transaction, even though they are executed through the executeQuery method.

So this statement is referenced in the DelayTransactionStartUntilFirstWriteMockServerTest

thiagotnunes · 2023-05-31T01:44:47Z

.../com/google/cloud/spanner/connection/DelayTransactionStartUntilFirstWriteMockServerTest.java

+  }
+
+  @Test
+  public void testSingleQuery() {


nit: rename to something a bit more clear. It seems we test that we never commit nor rollback, because there are no writes (thus we are using a singleUse read only transaction?)

I renamed the tests in this class to more clearly reflect that they test transactions with a single query, ... etc.

thiagotnunes · 2023-05-31T01:45:59Z

.../com/google/cloud/spanner/connection/DelayTransactionStartUntilFirstWriteMockServerTest.java

+
+        assertEquals(2, mockSpanner.countRequestsOfType(ExecuteSqlRequest.class));
+        assertFalse(mockSpanner.getRequestsOfType(ExecuteSqlRequest.class).get(0).hasTransaction());
+        assertFalse(mockSpanner.getRequestsOfType(ExecuteSqlRequest.class).get(1).hasTransaction());


Why does the second query has a transaction? I would expect both of them to be using a singleUse (because there were no writes)?

I think you misread the assertion... Or I'm missing something. The line says assertFalse(... hasTransaction()).

So your assumption is right. Both are single-use read-only.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

🤖 I have created a release *beep* *boop* --- ## [6.43.0](https://togithub.com/googleapis/java-spanner/compare/v6.42.3...v6.43.0) (2023-06-07) ### Features * Delay transaction start option ([#2462](https://togithub.com/googleapis/java-spanner/issues/2462)) ([f1cbd16](https://togithub.com/googleapis/java-spanner/commit/f1cbd168a7e5f48206cdfc2d782835cf7ccb8b0d)) * Make administrative request retries optional ([#2476](https://togithub.com/googleapis/java-spanner/issues/2476)) ([ee6548c](https://togithub.com/googleapis/java-spanner/commit/ee6548cfa511d6efc99f508290ed0b1ce025a4cc)) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.11.0 ([#2486](https://togithub.com/googleapis/java-spanner/issues/2486)) ([82400d5](https://togithub.com/googleapis/java-spanner/commit/82400d5576c3ffe08ff6bb94d8b1a307e2f41662)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).

product-auto-label bot added size: xl Pull request size is extra large. api: spanner Issues related to the googleapis/java-spanner API. labels May 23, 2023

olavloite added 3 commits May 24, 2023 16:02

fix: clirr check

2c39555

fix: only create tx manager if needed

9db90fa

test: add integration tests

0dbbf3c

olavloite marked this pull request as ready for review May 26, 2023 10:32

olavloite requested review from a team as code owners May 26, 2023 10:32

olavloite requested review from thiagotnunes and arpan14 May 26, 2023 10:32

olavloite and others added 2 commits May 26, 2023 12:35

test: skip concurrency tests on emulator

4fb23cc

🦉 Updates from OwlBot post-processor

438e736

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

thiagotnunes approved these changes May 31, 2023

View reviewed changes

olavloite and others added 2 commits June 1, 2023 13:45

Merge branch 'main' into delay-transaction-start

d828f64

🦉 Updates from OwlBot post-processor

fc0f7ed

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

arpan14 approved these changes Jun 1, 2023

View reviewed changes

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2023

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 2, 2023

olavloite merged commit f1cbd16 into main Jun 2, 2023
20 of 21 checks passed

olavloite deleted the delay-transaction-start branch June 2, 2023 14:18

release-please bot mentioned this pull request Jun 2, 2023

chore(main): release 6.43.0 #2478

Merged

olavloite mentioned this pull request Aug 21, 2023

feat: [WIP] support read-committed isolation level #2173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: delay transaction start option #2462

feat: delay transaction start option #2462

olavloite commented May 23, 2023 •

edited

thiagotnunes May 31, 2023

olavloite Jun 1, 2023

thiagotnunes May 31, 2023

olavloite Jun 1, 2023

thiagotnunes May 31, 2023

olavloite Jun 1, 2023

thiagotnunes May 31, 2023

olavloite Jun 1, 2023

thiagotnunes May 31, 2023

olavloite Jun 1, 2023

thiagotnunes May 31, 2023

olavloite Jun 1, 2023

feat: delay transaction start option #2462

feat: delay transaction start option #2462

Conversation

olavloite commented May 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite commented May 23, 2023 •

edited