feat: inline BeginTransaction with first statement

[ko3a4ok]

Add inline BeginTransaction for the first read or executeSql request in a transaction.

Each transaction doesn't call beginTransanction before its start. Instead, all creation transaction parameters are passed within the first read or executeSql request.
In case of a transaction restart, beginTransaction is called explicitly.

[olavloite]

Would you mind adding a test that verifies the following:

1. A query is the first statement in a read/write transaction. A transaction ID is successfully returned by initial request.
2. One or more PartialResultSets are returned by the stream, with (at least) one of them returning a resume token.
3. The stream fails halfway with an UNAVAILABLE error and the stream is restarted with a resume token.

Step 3 should use the transaction ID that was returned by step 1, and not include a BeginTransaction option.

(https://github.com/googleapis/nodejs-spanner/pull/1253/files is a very old implementation for the same. Some of the tests there might be re-usable.)

[ko3a4ok]

Added. thanks a lot for this comment: it helped to find a bug in the code

[olavloite]

This does not take into account that a transaction could in theory have multiple requests in flight at the same time. If a transaction starts out with sending two SELECT statements to the backend, it might very well be that the first that is sent has not yet returned a transaction id before the second is being sent. That will cause both requests to include a BeginTransaction option.

Consider the following test case (the getRowCountFromStreamingSql function is defined in test/Spanner.ts):

    it('should only inline one begin transaction', async () => {
      const database = newTestDatabase();
      await database.runTransactionAsync(async tx => {
        const rowCount1 = getRowCountFromStreamingSql(tx, {sql: selectSql});
        const rowCount2 = getRowCountFromStreamingSql(tx, {sql: selectSql});
        await Promise.all([rowCount1, rowCount2]);
        await tx.commit();
      });
      await database.close();

      let request = spannerMock.getRequests().find(val => {
        return (val as v1.ExecuteSqlRequest).sql;
      }) as v1.ExecuteSqlRequest;
      assert.ok(request, 'no ExecuteSqlRequest found');
      assert.deepStrictEqual(request.transaction!.begin!.readWrite, {});
      assert.strictEqual(request.sql, selectSql);

      request = spannerMock
        .getRequests()
        .slice()
        .reverse()
        .find(val => {
          return (val as v1.ExecuteSqlRequest).sql;
        }) as v1.ExecuteSqlRequest;
      assert.ok(request, 'no ExecuteSqlRequest found');
      assert.strictEqual(request.sql, selectSql);
      assert.ok(request.transaction!.id, 'TransactionID is not set.');
      const beginTxnRequest = spannerMock.getRequests().find(val => {
        return (val as v1.BeginTransactionRequest).options?.readWrite;
      }) as v1.BeginTransactionRequest;
      assert.ok(!beginTxnRequest, 'beginTransaction was called');
    });

[ko3a4ok]

You're absolutely right. Added a lock that prevents a multiple inline begin transaction at the same time. Thanks!

[surbhigarg92]

What would happen if the call fails?
As per the design we should make an explicit begin transaction call if the call fails.

cc: @olavloite

[ko3a4ok]

if the call fails, the transaction is created explicitly with "BeginTransaction" rpc call.
It's covered in the test: "should use beginTransaction on retry".

[asthamohta]

Is this done at the backend or do we explicitly call begin transaction from the client side?

[ko3a4ok]

from the client side

[asthamohta]

Should this rather be else if(this._options.readWrite)

[ko3a4ok]

yes, fixed, thanks

[surbhigarg92]

@ko3a4ok Can you please fix the failing test cases ?

[surbhigarg92]

@ko3a4ok We should also remove the transaction.begin call at line and add the appropriate test cases for the same.

[ko3a4ok]

@ko3a4ok We should also remove the transaction.begin call at line and add the appropriate test cases for the same.

@surbhigarg92, unfortunately, if we just remove it, the explicit beginTransaction call won't be called on the retry. I updated to call it only after the first attempt. Please let me know if it makes sense, or is it better to move the call transaction.begin() somewhere here.

[surbhigarg92]

@ko3a4ok We should also remove the transaction.begin call at line and add the appropriate test cases for the same.

@surbhigarg92, unfortunately, if we just remove it, the explicit beginTransaction call won't be called on the retry. I updated to call it only after the first attempt. Please let me know if it makes sense, or is it better to move the call transaction.begin() somewhere here.

This looks fine. Only thing I am wondering is that with this implementation, we are calling transaction.begin() in all possible failure cases.
@olavloite Do you think there will be any particular error case where we wan't to skip calling transaction.begin()

[ko3a4ok]

@ko3a4ok I believe we are missing 2 use cases for Inline BeginTransaction implementation.

1. Mutation - Consider a transaction which contains only mutation operations. In that case, first rpc in the transaction will be commit transaction. To handle this scenario , we would need to make an explicit begin transaction rpc.
2. ExecuteBatchDML - Transaction needs to be inlined for ExecuteBatchDML operation as well. Special handling is required for this use case.

ExecuteBatchDml is also slightly odd since either the call itself can fail, or it can return success with a non-OK Status in the ExecuteBatchDmlResponse. In the latter case, there may still be one or more ResultSets returned, with the first containing a transaction ID in its ResultSetMetadata. The upshot is that the client should check ExecuteBatchDmlResponse::result_set for a transaction prior to considering ExecuteBatchDmlResponse::status.

Regarding 1.: in this case, we should not create any transaction at all because CommitRequest will include singleUseTransaction parameter, and transactionId will be empty. This is how it works now, and inlined begin transaction shouldn't affect this implementation.

Regarding 2.: I completely forgot about that, thanks for bringing this up. Added in 2caaa0f

[gcf-merge-on-green]

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.