refactor: Use modern way to obtain pgsql size#965
Conversation
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de> AI-assisted: Claude Code (Claude Sonnet 4.6)
There was a problem hiding this comment.
Pull request overview
Refactors PostgreSQL database size retrieval in DatabaseStatistics to avoid splitting database names containing dots and to simplify the query, addressing the reported failure in issue #556 and reducing risk of invalid result handling.
Changes:
- Replace manual
pg_proc/pg_databaselookups (and dot-splitting of db name) with a directpg_database_size(:dbname)query. - Use a bound parameter for the database name when querying PostgreSQL size.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $database = $this->config->getSystemValue('dbname'); | ||
| $sql = 'SELECT pg_database_size(:dbname) as size'; | ||
| $result = $this->connection->executeQuery($sql, ['dbname' => $database]); | ||
| $database_size = $result->fetchOne(); |
There was a problem hiding this comment.
Consider using getSystemValueString('dbname') here (as already done for dbtype in getDatabaseStatistics()) so the parameter passed to pg_database_size() is guaranteed to be a string. getSystemValue() returns mixed, and a non-string value could cause unexpected parameter binding/SQL errors at runtime and may reintroduce static-analysis warnings.
| case 'pgsql': | ||
| $sql = "SELECT proname | ||
| FROM pg_proc | ||
| WHERE proname = 'pg_database_size'"; | ||
| $result = $this->connection->executeQuery($sql); | ||
| $row = $result->fetch(); | ||
| $database = $this->config->getSystemValue('dbname'); | ||
| $sql = 'SELECT pg_database_size(:dbname) as size'; | ||
| $result = $this->connection->executeQuery($sql, ['dbname' => $database]); | ||
| $database_size = $result->fetchOne(); | ||
| $result->closeCursor(); | ||
| if ($row['proname'] === 'pg_database_size') { | ||
| $database = $this->config->getSystemValue('dbname'); | ||
| if (strpos($database, '.') !== false) { | ||
| [$database, ] = explode('.', $database); | ||
| } | ||
| $sql = "SELECT oid | ||
| FROM pg_database | ||
| WHERE datname = '$database'"; | ||
| $result = $this->connection->executeQuery($sql); | ||
| $row = $result->fetch(); | ||
| $result->closeCursor(); | ||
| $oid = $row['oid']; | ||
| $sql = 'SELECT pg_database_size(' . $oid . ') as size'; | ||
| $result = $this->connection->executeQuery($sql); | ||
| $row = $result->fetch(); | ||
| $result->closeCursor(); | ||
| $database_size = $row['size']; | ||
| } | ||
| break; |
There was a problem hiding this comment.
The PGSQL databaseSize() branch is the core behavior change to fix #556 (db names containing dots) but there’s no automated test asserting the generated SQL/parameters or that dotted DB names are handled correctly. Adding a focused PHPUnit test for the pgsql path (mocking IConfig + IDBConnection/Result) would help prevent regressions.
Fix #556
And resolves the psalm warnings about Database, Sharing and Storage statistics by switching from fetch to fetchOne to unblock #952