feat: classify retryable transaction exceptions#10162
feat: classify retryable transaction exceptions#10162memleakd wants to merge 2 commits intocodeigniter4:4.8from
Conversation
Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
|
Thanks for working on this. The goal makes sense. It's useful to flag transaction errors that are worth retrying, like deadlocks and serialization failures. However, I wonder if this should follow the same approach we recently added with Instead of adding a public method like try {
$result = $db->transException(true)->transaction(static function ($db) {
// ...
});
} catch (RetryableTransactionException $e) {
// Retry the whole transaction
}This seems doable because the drivers already inspect native database codes when creating Overall, this looks useful, especially the conservative list of retryable codes. My main suggestion is about the public API shape. I would prefer named exception over |
- Replace the public retryable transaction classifier with a named exception - Classify retryable driver errors through the database exception factory - Update docs and tests for exception-based retry handling Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com>
|
Thanks a lot for pointing this out. That makes total sense, and I agree it's a better direction. I missed the previous I refactored the PR around that idea. The drivers now classify the known retryable transaction errors and throw One thing I noticed while doing this is that prepared queries still have separate exception paths, similar to the existing unique-constraint behavior. I kept that out of this PR so it stays focused, but I'd be happy to work on prepared-query consistency as a follow-up after this one. |
| $exception = $e->getCode() === 1062 | ||
| ? new UniqueConstraintViolationException($e->getMessage(), $e->getCode(), $e) | ||
| : new DatabaseException($e->getMessage(), $e->getCode(), $e); | ||
| : $this->createDatabaseException($e->getMessage(), $e->getCode(), $e); |
There was a problem hiding this comment.
One thing I think can be improved is, let $this->createDatabaseException handle all types of exception including UniqueConstraintViolation. Similar to isRetryableTransactionErrorCode, there could be a protected function like isUniqueConstraintViolation(int|string $code): bool and this function will be used by createDatabaseException function and it returns UniqueConstraintViolationException for that particular error code. In this way, we can avoid this ternary operator in all connection implementations, and in future, if we need to add more refined exceptions, we can do so just in BaseConnection without messy ternary operators
Also, please update the PR description as well. |
Description
This PR proposes adding
isRetryableTransactionException()to database connections.The goal is to give CodeIgniter a small, driver-aware way to recognize transaction failures that are reasonable candidates for a whole-transaction retry, such as deadlocks and serialization failures.
This is useful on its own for applications that already want to implement their own retry policy:
It is also intended as groundwork for a follow-up proposal to add opt-in retry support to the closure-based
transaction()helper. Establishing this driver classification policy first keeps that later retry proposal smaller: which database errors CodeIgniter considers retryable can be discussed separately from retry control flow, attempt counts, delay/backoff behavior, and callback side-effect concerns.The classifier is intentionally conservative. It only includes errors that represent transaction-level concurrency failures where retrying the whole transaction is a reasonable default.
This PR does not retry anything automatically. It only gives applications and future framework APIs a common way to ask: "is this database exception one of the transaction failures the framework considers retryable?"
API Shape Feedback
One intentional question in this PR is whether this classifier should be public now.
I proposed it as
ConnectionInterface::isRetryableTransactionException()because it has standalone value for applications and packages that already want to implement their own retry policy, and it gives future framework retry behavior a shared driver-aware classification point.That said, if the team would rather avoid exposing this as public API before automatic transaction retry support is discussed, I am happy to adjust the PR so the driver-specific classification stays internal/protected for now, and leave the public surface to a later
transaction()retry proposal.Feedback on this API shape would be very welcome.
Changes
ConnectionInterface::isRetryableTransactionException().BaseConnection.Driver Policy
Included by default:
121340001,40P0151205,396060,8177Not included by default: lock timeouts, unique constraint violations, resource-busy errors, or SQLite extended busy codes. Those can be valid retry cases in some applications, but they are more policy-dependent.
References: MySQL, PostgreSQL, SQLite, SQL Server deadlocks, SQL Server 3960, Oracle ORA-00060, Oracle ORA-08177.
Checklist: