-
Notifications
You must be signed in to change notification settings - Fork 48
Add retry mechanism when connecting to Oracle #1303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 2 commits
eb15869
adb67e9
a81bf28
21e7013
0f411a6
f4e1767
b2c0820
8429910
a30e06f
60c3fa5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ on: | |
| pull_request: | ||
| push: | ||
| schedule: | ||
| - cron: '0 0/2 * * *' | ||
| - cron: '0 0/8 * * *' | ||
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
|
|
@@ -120,6 +120,14 @@ jobs: | |
| image: gvenzl/${{ matrix.type == 'Phpunit Lowest' && 'oracle-free:slim-faststart' || 'oracle-xe:18-slim-faststart' }} | ||
| env: | ||
| ORACLE_PASSWORD: atk4_pass | ||
| APP_USER: atk4_test_user | ||
| APP_USER_PASSWORD: atk4_user_pass | ||
| # Provide healthcheck script options for startup | ||
| options: >- | ||
| --health-cmd healthcheck.sh | ||
| --health-interval 10s | ||
| --health-timeout 5s | ||
| --health-retries 10 | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
@@ -246,9 +254,10 @@ jobs: | |
| - name: "Run tests: Oracle - PDO (only for coverage or cron)" | ||
| if: (success() || failure()) && (env.LOG_COVERAGE || github.event_name == 'schedule') | ||
| env: | ||
| DB_DSN: "pdo_oci:dbname=oracle/free" | ||
| DB_USER: system | ||
| DB_PASSWORD: atk4_pass | ||
| # connection to portable database | ||
| DB_DSN: "pdo_oci:dbname=oracle/${{ matrix.type == 'Phpunit Lowest' && 'freepdb1' || 'xepdb1' }}" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issues are mostly/solely present with "lowest". Ie. the lowest/oldest deps or the newest db. I would be happy to have this explained - is this because of the newest/never db and did some db limit changed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #1303 (comment)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I trigged about 1000 pipelines with all Oracle versions and I have found:
The retry solution might be the only one. I want to still do some more experiments. Also, have the 322/502 200/320 limits changed with version 23.3 -> 23.4? If not, what changed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well. That could be true about 23.3 and 23.4 as from 23.4 Oracle introduced AI Vector Search & Data Types - Version 23.4 officially introduced the native VECTOR data type and AI vector search features, which were not fully packaged or active in 23.3 developer builds. I can't guarantee that this AI answer is correct, but that's approximately that. But anyway, even if any of these solutions work, I still think that retry middleware is good to have as a fallback mechanism. No long retries, just try few times in quite short intervals and that's it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I cannot agree with the last paragraph. If 1 concurrent connection can exceed the total connections limit of 200, there is something very wrong with the database. If there is a possibility of fixing this by some Oracle configuration, I might prefer it. I mean of course not by raising the concurrency to sky...
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that would be perfect to fix it by Oracle settings of course. P.S. Also Oracle uses like maybe 50 connections internally for its own services. So it's not 200 free connections at a start. |
||
| DB_USER: atk4_test_user | ||
| DB_PASSWORD: atk4_user_pass | ||
| NLS_LANG: AMERICAN_AMERICA.AL32UTF8 | ||
| run: | | ||
| php -d opcache.enable_cli=1 vendor/bin/phpunit --exclude-group none $(if [ -n "$LOG_COVERAGE" ]; then echo --coverage-text; else echo --no-coverage; fi) --fail-on-warning --fail-on-risky $(if vendor/bin/phpunit --version | grep -q '^PHPUnit 9\.'; then echo -v; else echo --fail-on-notice --fail-on-deprecation --display-notices --display-deprecations --display-warnings --display-errors --display-incomplete --display-skipped; fi) | ||
|
|
@@ -257,9 +266,10 @@ jobs: | |
| - name: "Run tests: Oracle - OCI8" | ||
| if: success() || failure() | ||
| env: | ||
| DB_DSN: "oci8:dbname=oracle/free" | ||
| DB_USER: system | ||
| DB_PASSWORD: atk4_pass | ||
| # connection to portable database | ||
| DB_DSN: "oci8:dbname=oracle/${{ matrix.type == 'Phpunit Lowest' && 'freepdb1' || 'xepdb1' }}" | ||
| DB_USER: atk4_test_user | ||
| DB_PASSWORD: atk4_user_pass | ||
| NLS_LANG: AMERICAN_AMERICA.AL32UTF8 | ||
| run: | | ||
| php -d opcache.enable_cli=1 vendor/bin/phpunit --exclude-group none $(if [ -n "$LOG_COVERAGE" ]; then echo --coverage-text; else echo --no-coverage; fi) --fail-on-warning --fail-on-risky $(if vendor/bin/phpunit --version | grep -q '^PHPUnit 9\.'; then echo -v; else echo --fail-on-notice --fail-on-deprecation --display-notices --display-deprecations --display-warnings --display-errors --display-incomplete --display-skipped; fi) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Atk4\Data\Persistence\Sql\Oracle; | ||
|
|
||
| use Doctrine\DBAL\Driver; | ||
| use Doctrine\DBAL\Driver\Connection; | ||
| use Doctrine\DBAL\Driver\Middleware; | ||
| use Doctrine\DBAL\Driver\Middleware\AbstractDriverMiddleware; | ||
| use Doctrine\DBAL\Driver\OCI8\Exception\ConnectionFailed; | ||
|
|
||
| /** | ||
| * Retries transient Oracle listener failures during connection establishment. | ||
| * | ||
| * Oracle may temporarily return ORA-12516 when the listener has not yet | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure? IIRC this issue is present even with very few requests. What is the real connection count limit? (and btw. this is already an issue with MySQL, but limit of small number of connections instead of strict 1 is enough and reliable)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Current limits were: About 60-90 sessions can be already used internally by Oracle background services. You can see more stats here https://github.com/atk4/data/actions/runs/31742995074/job/94590708915
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also you can check step "Stress test Oracle connections" in this job https://github.com/atk4/data/actions/runs/31723291851/job/94525337653
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The proper solution might be to release the connection better. It might be even a bug in php driver. Does this happen with pdo_oci as well as oci8? A solution worth to consider might be even to force to release the connection using destructor.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a bug or something in php. |
||
| * released a handler after a connection has been closed. Retrying after | ||
| * a short delay allows the listener to refresh its handler state. | ||
| */ | ||
| class RetryConnectionMiddleware implements Middleware | ||
| { | ||
| #[\Override] | ||
| public function wrap(Driver $driver): Driver | ||
| { | ||
| return new class($driver) extends AbstractDriverMiddleware { | ||
| // ORA-12516: TNS:listener could not find available handler with matching protocol stack | ||
| private const RETRY_ERROR_CODES = [12516]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Discord you mentioned https://dbamarco.wordpress.com/2023/07/20/suspicous-ora-12516-by-oracle-scan-listener/ . I want to check if this desctribe the issue. In the chat, he wrote they increased the connection limit... Did you see this issue in production somewhen?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Increasing connection limit is simplest workaround. Of course if you have 1000000 connections limit, then you'll not hit it while testing and all should be fine. But normally there shouldn't be such high limit. I've tested that all here in last few days. It's really that - in case of this failure just retry in 10-100ms (rarely a bit more - up till about 0.5s) and listener cache is updated, connections are freed and tests continue. |
||
| private const RETRY_COUNT = 8; | ||
| private const RETRY_INTERVAL_BASE_MS = 10; | ||
| private const RETRY_INTERVAL_MAX_MS = 1000; | ||
|
|
||
| #[\Override] | ||
| public function connect( | ||
| #[\SensitiveParameter] | ||
| array $params | ||
| ): Connection { | ||
| for ($attempt = 0;; ++$attempt) { | ||
| try { | ||
| return parent::connect($params); | ||
| } catch (ConnectionFailed $e) { // @phpstan-ignore catch.internalClass | ||
| if ( | ||
| !in_array($e->getCode(), self::RETRY_ERROR_CODES, true) | ||
| || $attempt >= self::RETRY_COUNT | ||
| ) { | ||
| throw $e; | ||
| } | ||
|
|
||
| $timeoutMs = min( | ||
| self::RETRY_INTERVAL_BASE_MS * (2 ** $attempt), | ||
| self::RETRY_INTERVAL_MAX_MS | ||
| ); | ||
|
|
||
| usleep($timeoutMs * 1000); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| } | ||
| } | ||





There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github CI is dummy, all services are started one by one and all healtchecks are busy waited before tests. If a service starts slow, it will slowdown the CI.
Thanks and I will check this on the separate PRs. In past, I opted out for the healtchecks as the tests on other DBs were realiably enough without having to busy wait.
Please do not merge this PR yet, I want to land the other changes first and verify if retry mechanism is the only solution, I personally do not like it but on the other side Oracle is very ugly software and it might be the best working solution.