Add a reusable SqlRetryPolicy with per-DBMS transient-error classification - #590
Open
Yaraslaut wants to merge 4 commits into
Open
Add a reusable SqlRetryPolicy with per-DBMS transient-error classification#590Yaraslaut wants to merge 4 commits into
Yaraslaut wants to merge 4 commits into
Conversation
…ssification SqlBackup had a working retry/backoff policy, but it was private to the backup engine: nothing outside it could retry a deadlock-losing transaction or a dropped connection. Introduce SqlRetryPolicy, which pairs a SqlRetrySettings descriptor (budget, initial delay, multiplier, per-delay cap, cumulative-delay deadline) with a SqlRetryClassifier deciding which errors are worth another attempt. The classifier is a per-DBMS extension point reached through the new SqlQueryFormatter::RetryOps(), mirroring AdvisoryLockOps(): PostgreSQL keys off dedicated SQLSTATEs (55P03, 57P0x, 53300), SQL Server off native codes (1205 deadlock victim, 1222 lock timeout, the Azure transient set), SQLite off the driver message text, all on top of the shared ODBC classes 08/40 and HYT00/01. Decide() is pure — no clock, no I/O, no hidden state — so every branch is reachable from a unit test; Execute()/TryExecute() are thin drivers over it, with the sleeper injected so tests need not sleep through a real backoff. SqlBackup migrates onto the shared type: RetrySettings becomes an alias of SqlRetrySettings, RetryAction an alias of SqlRetryAction, and the detail helpers plus the remaining hand-rolled loops delegate to the policy. Field names, defaults and progress-message wording are unchanged. Refs #554 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
…tabase The retry policy landed in the previous commit but had no test file. This adds one, and registers it in the test CMakeLists. The policy is deliberately free of I/O and of any clock - an error is a value and the sleeper is injected - so every case runs as a pure function of its inputs, with no database and no real waiting. That is what makes the per-DBMS classification testable at all: provoking a genuine Azure SQL throttle or a PostgreSQL serialization failure on demand is not something a unit test can arrange.
…icy tests CI's clang-tidy job would have rejected the new test file on two counts, both fixed at the source rather than suppressed: - `MakeError` took (sqlState, nativeCode, message), and the project sets bugprone-easily-swappable-parameters.MinimumLength to 3, so exactly that shape is flagged - a caller can transpose the two strings silently. The helper now takes only the SQLSTATE and native code; the nine cases that need a driver message spell out the SqlErrorInfo aggregate with designated initializers, which names each field and cannot be transposed. - FlakyOperation mixed a public `calls` with private state, which cppcoreguidelines-non-private-member-variables-in-classes reports (its IgnoreClassesWithAllMemberVariablesBeingPublic exemption does not apply to a mixed class). It is now private with a Calls() accessor. Also applies clang-format, which the file had drifted from on one line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
tests
Core API
Query Formatter
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Two failures on this branch, both in the feature commit rather than the tests: - `Check documentation coverage` rejected `@ref SqlException` in SqlRetryPolicy.hpp: `@ref` needs a target Doxygen has indexed, and `SqlException` carries no doc comment, so the reference could not resolve. The project already refers to it as `@c SqlException` elsewhere (Utils.hpp); this now matches that. - `Check C++ style` found SqlBackup/Common.hpp unformatted around `RetryOnTransientError`. clang-format applied. No behaviour change. Suite green on sqlite3 (1426 cases). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #554
What & why
Transient database failures — a dropped connection, a deadlock victim, a serialization failure, a lock timeout — are retryable; a unique-constraint violation or a syntax error is not. Which SQLSTATE means which is dialect-specific:
40001is a serialization failure on PostgreSQL, while SQL Server signals a deadlock victim through native code1205under a genericHY000, and SQLite reportsSQLITE_BUSYin the driver's message text.This adds
SqlRetryPolicywith a per-DBMS classifier, so callers express intent (retry what is worth retrying) rather than hard-coding SQLSTATE tables at every call site.Design
The policy is deliberately free of I/O and of any clock: an error is a value, and the sleeper is injected. That is what makes the classification testable at all — provoking a genuine Azure SQL throttle or a PostgreSQL serialization failure on demand is not something a unit test can arrange. Every test case therefore runs as a pure function of its inputs, with no database and no real waiting.
Testing
24 test cases / 129 assertions covering each dialect's classifier (transient and non-transient SQLSTATEs, so a classifier cannot pass by saying "yes" to everything), the backoff schedule, retry exhaustion, and the
Executecontrol flow.clang-debug(PEDANTIC + ASan + UBSan)sqlite3,mssql2022(Docker),postgres(Docker 16.4) — 1426 cases eachNOLINT)The two clang-tidy fixes are their own commit:
MakeError's three parameters trippedbugprone-easily-swappable-parameters(the project setsMinimumLength: 3), so it now takes two and the cases needing a driver message spell out the aggregate with designated initializers; andFlakyOperation's publiccallsnext to private state trippedcppcoreguidelines-non-private-member-variables-in-classes, so it is private with aCalls()accessor.Compilers: Clang only, locally —
gcc-releaseis Linux-gated and unavailable on this macOS host; CI's GCC legs cover it.