[EXPOSED-46] Add a possibility to set a delay for the repetition attempts

[mgrati]

In the project I am working on, we use the Serializable isolation level in our transactions. When sending multiple queries at the same time, we often have the following error

• com.impossibl.postgres.jdbc.PGSQLSimpleException: could not serialize access due to read/write dependencies among transactions
• or com.impossibl.postgres.jdbc.PGSQLSimpleException: could not serialize access due to concurrent update

For this type of error, the setting of a number of repetition attempts should solve the issue, and the query should be executed correctly in the second or third attempt. However, depending on the number of queries sent simultaneously, we are often left with some queries that finish all the repetition attempts unsuccessfully since they are immediately launched.

To make sure we minimise the occurrence of this problem. I propose in this pull request to add two parameters minRepetitionDelay and maxRepetitionDelay to the DatabaseConfig. if set, the repetition attempt is launched with a random delay time between the minRepetitionDelay and maxRepetitionDelay.

[bog-walk]

@mgrati Thank you for submitting a PR for your feature request.

A question so we can better understand:
The proposed change involves 2 new parameters that are used to calculate a random delay within the range set by the parameters.

• What is the advantage of the delay being randomised, as opposed to introducing a single parameter defaultRepetitionDelay? With this change, if a user wants to explicitly set a delay value, they'd have to provide equal duplicate arguments to avoid the first conditional branch.

[mgrati]

@bog-walk In our case, it's to maximize our chance for the repeated transactions to actually pass.

Since we can have multiple transactions that can be refused at the same time due to the same reason (due to the transaction isolation level, which is serializable in our case) we have more chance for the repeated transaction to pass if they are repeated each after a different time rather than all at the same time, hence the randomized delay.

[e5l]

Could you tell me how it will work with newSuspendTransaction?

[mgrati]

I tried to do some tests using newSuspendedTransaction with the repetition delay. Here's what I found :

• newSuspendedTransaction calls suspendedTransactionAsyncInternal which does not handle transaction repetitions

try {
        transaction.statement().apply {
            if (shouldCommit) transaction.commit()
        }
    } catch (e: SQLException) {
        handleSQLException(e, transaction, 1)
        throw e

• if newSuspendedTransaction is called within a normal transaction, then the transactions are repeated with the calculated delay.

Is there something else I should check?

[e5l]

As a user, I would expect that both transaction and newSuspendTransaction had the same retry policy

[mgrati]

Hi. Sorry I couldn't find the time to check your response until today.

I don't have any experience working with coroutines but I tried to implement the same retry policy in newSuspendTransaction as the one in transaction.

Unfortunately, I couldn't make it work : When implementing the while loop to handle retries, I got the following error in suspendedTransactionAsyncInternal, Which I don't really understand:

Type mismatch: inferred type is Deferred<Unit> but Deferred<T> was expected

I didn't pursue this further because for me the scope of this pull request is to be able to set a delay whenever I have a repetition attempt. Which is currently only handled in transaction.

I think adding repetition handling in newSuspendTransaction can be added in new separate issue. What do you think?

[e5l]

Sure, let's move it to the separate issue

[e5l]

please check the comments

[e5l]

As a user, I would expect that both transaction and newSuspendTransaction had the same retry policy

[bog-walk]

@mgrati Thanks for all the work on this PR.

So that we can move forward to merging, could you please perform gradle task apiDump, as per these instructions, and push a fresh commit with the updated .api files. That should allow the build checks to pass.

[bog-walk]

@mgrati Thanks for the quick response. I should have made it clear that a rebase from main would be needed to get all the existing api files before updating them with apiDump, my apologies. Would you be willing to do that to resolve the branch conflicts?

[mgrati]

@bog-walk Done. The branch conflicts are now resolved.

[bog-walk]

@mgrati One last request that I'd do myself but I can't push to your branch.

A new api file was somehow added to the exposed-tests module even though the apiChecker is setup to ignore that module. Could you please remove the new exposed-tests/api folder and its new file? The final diff should have only 7 existing files changed.

If you run the task apiCheck after doing so, it should run successfully, so you won't have to run apiDump again. Hopefully I won't have to bother you again after this, so thanks for your patience 👍