Repository navigation
Bug 2091129 - Bugzilla::DB pings MySQL before every query (about 200 pings per show_bug.cgi request) - #2821
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the request-scoped database-handle caching change.
No open issues 🎉
What changed in this PR
Reduces repeated database connection checks in Bugzilla by reusing checked handles instead of consulting DBIx::Connector before every query.
Changes:
- Caches database handles per request, rechecking after five seconds, a process ID change, or handle inactivity.
- Adds tests for handle reuse, request boundaries, interval expiry, simulated process ID changes, and reconnection after disconnect.
| File | Description |
|---|---|
t/db-dbh-check.t |
Tests connection-check frequency and handle revalidation. |
Bugzilla/DB.pm |
Replaces delegated handle access with request-scoped caching and periodic checks. |
Review effort: Balanced
| $cache->{refaddr $self} = {dbh => $dbh, pid => $PID, checked => $now}; | ||
| return $dbh; | ||
| } | ||
| has [qw(dsn user pass attrs)] => (is => 'ro', required => 1); |
There was a problem hiding this comment.
Nit: should there be a blank line here, or should sub dbh move below the has declarations so the attributes stay grouped together?
There was a problem hiding this comment.
Moved sub dbh (along with DBH_CHECK_INTERVAL and its comment) below the has/around declarations in 5db4dc0, so the attributes are grouped together.
| use constant DBH_CHECK_INTERVAL => 5; | ||
|
|
||
| # DBIx::Connector->dbh pings the server on every call, and every proxied DBI | ||
| # method below calls dbh(), so a single page used to send hundreds of pings. |
There was a problem hiding this comment.
The comment above the DBI proxy methods (lines 86-87, DBIx::Connector->dbh has to be called each time we need a DBI handle to ensure the connection is alive) now says the opposite of this change.
Could it be updated, for example with something like "We can't use handles() as dbh() has to re-check the connection (see dbh above)"? As you want, its a detail ;)
Xzzz
left a comment
Xzzz
left a comment
There was a problem hiding this comment.
All good to me, thanks!
Just a suggestion on a comment line, but not so important :)
Addresses review feedback: keeps the Moo has declarations grouped together.
No description provided.