feat: Fixing and refactoring transaction retry logic in dbapi. Also adding interceptors support for testing

[ankiaga]

We found couple of bugs in the existing retry logic, the major one being b/315807641
Other one being assuming tuple to be a list (line 316 in old connection.py) and comparing which would never work.
These issues were not caught as there are no proper tests for these.

So this PR is fixing those issues. Also the current code was not very easy to understand so this PR also refactor the retry logic from connection.py to transaction_helper.py along with the fixes. Tests from test_connection.py have also moved to test_transaction_helper.py along with many new tests (mainly on batch statements) added

Also added interceptors support so we can test retry logic in system tests. Added system tests for the same

[olavloite]

Would you mind adding a description to the PR what the faulty behavior was that you found, and how you fixed it? The PR is relatively big and complex, so it would be good to have some context.

Also: After an initial pass, it is very clear that this feature does not have a good enough test coverage. There are some big problems with the current implementation (probably much of it was already there), and the fact that none of the tests have caught that is a clear sign that it is not tested well enough.

[olavloite]

My general impression is that we need a more extensive test suite for transaction retries. Tests that I would like to see are:

1. Tests that show that a retry fails if the underlying data has changed.
2. Tests that show that a retry succeeds if the underlying data has remained unchanged. The test should include a query that selects data from a table that uses all data types that are supported by Cloud Spanner. This will show that the checksum calculation works correctly for all data types.
3. Tests that verify that the same retry as in test 2 fails if one of the columns in the result has changed. This also verifies that the checksum calculation is 'correct' for all data types in the sense that it produces a different outcome if a value has changed.
4. (Potentially in a separate PR) A test that shows that a Batch DML that contains N elements and that successfully executes M=N-X of the DML statements and returns an error for statement number M+1, is correctly handled. That is: During a retry, both the M update counts + the error must be the same for the retry to be successful, and otherwise it should fail.
5. (Potentially in a separate PR) A test that shows that a fetchxxx call that returns an Aborted error is correctly handled. 'Correctly' in this case means either:
   5.1. Propagates the Aborted error to the user without retrying
   5.2. OR: Internally retries the transaction and continues the cursor from the same point.

[ankiaga]

My general impression is that we need a more extensive test suite for transaction retries. Tests that I would like to see are:

1. Tests that show that a retry fails if the underlying data has changed.
2. Tests that show that a retry succeeds if the underlying data has remained unchanged. The test should include a query that selects data from a table that uses all data types that are supported by Cloud Spanner. This will show that the checksum calculation works correctly for all data types.
3. Tests that verify that the same retry as in test 2 fails if one of the columns in the result has changed. This also verifies that the checksum calculation is 'correct' for all data types in the sense that it produces a different outcome if a value has changed.
4. (Potentially in a separate PR) A test that shows that a Batch DML that contains N elements and that successfully executes M=N-X of the DML statements and returns an error for statement number M+1, is correctly handled. That is: During a retry, both the M update counts + the error must be the same for the retry to be successful, and otherwise it should fail.
5. (Potentially in a separate PR) A test that shows that a fetchxxx call that returns an Aborted error is correctly handled. 'Correctly' in this case means either:
   5.1. Propagates the Aborted error to the user without retrying
   5.2. OR: Internally retries the transaction and continues the cursor from the same point.

Thanks for spending time and figuring out all tests missing here. As discussed created a bug https://buganizer.corp.google.com/issues/319404848 for this

[olavloite]

Thanks for your patience and perseverance on this 👍