[3.0] Reports failed queries on PostgreSQL instead of swallowing them

[albertlast]

Description

A failing query on PostgreSQL goes nowhere. query() ends with:

$this->last_result = @pg_query($connection, $db_string);

// Debugging.
if (DebugUtils::isDebugEnabled()) {
	self::$cache[self::$count]['t'] = microtime(true) - $st;
}

return $this->last_result;

false comes back, the warning is suppressed, and that is the end of it. Nothing is written to log_errors, nothing is shown, and the request carries on as if the statement had run. Callers that ask to opt out of error reporting by passing db_error_skip — there are around thirty of them, in the installer, the migrations, Unread, Fulltext and elsewhere — have been handing that flag to code that could not have honoured it either way.

This is not deliberate. 04b4ad1 took the block out of both database APIs in Dec 2023. 44c6b13 ("Show query errors") put it back for MySQL in Mar 2024. PostgreSQL was never given the same treatment, so the two APIs have disagreed about what a failed query means for the last two years.

This ports that MySQL block verbatim, so both behave alike again.

The aborted-transaction case that is specific to PostgreSQL is already handled: a failed statement poisons the rest of the transaction, but error_insert() rolls an open transaction back before it writes the log row, so the error still gets recorded.

What it exposes

This is the point of the change, but it is worth being explicit: turning silence into errors makes existing breakage visible, and there was some. Running a forum with this applied and exercising 31 pages plus posting, starting topics, sending PMs, searching, marking read and a full recount surfaced exactly two failing queries, both real bugs that had been quietly doing nothing:

Failing query	Effect	Fix
Draft::saveToDatabase(), SET type = <unix timestamp> into a tinyint	every edit to an existing draft was silently thrown away	#9339
Config::updateModSettings(), SET value = value + 1 on a text column	totalMessages, totalTopics, totalMembers, unapprovedMembers never moved	#9340

Both should land before this one. On their own they are invisible; with this change they become a Database Error page, the second one on every post. With them in place, the sweep above is clean.

Two more pre-existing faults turned up alongside them. Neither is a query failure and neither is affected by this change, but they are worth recording:

• Calendar::show() calls ErrorHandler::fatalLang('calendar_off', false, 403), where the third parameter is $sprintf and wants an array. Any hit on ?action=calendar with the calendar disabled is a TypeError rather than the intended 403.
• Sources/PersonalMessage/PM.php uses UserDataset::Minimal and UserDataset::None at lines 1607, 1670 and 1917 but never imports SMF\UserDataset, so it resolves to SMF\PersonalMessage\UserDataset. Sending a PM fatals with "Class not found" on any database.

Testing

Docker environment, PostgreSQL 17.

• A query that fails now produces the same Database Error page MySQL gives, naming the caller — ERROR: smallint out of range, File: /var/www/html/Sources/Draft.php, Line: 704 — and logs a row in log_errors with error_type = database.
• With [3.0] Stops drafts from silently discarding every change after the first save #9339 and [3.0] Makes the settings counters increment on PostgreSQL too #9340 applied, 31 pages plus post, new topic, PM send, search, mark all read and Forum Maintenance → recount all complete with an empty error log.
• vendor/bin/phpunit — 108 tests, 157 assertions, OK.

Issues References (Fixes|Related|Closes)

Related to #9339, #9340

[live627]

Could you remove the following four lines?

SMF/Sources/Db/APIs/PostgreSQL.php

Lines 684 to 687 in fcbc3cb

	if ($connection === null && $this->connection === null) {
	return '';
	}

Those null checks aren't necessary:

https://stackoverflow.com/a/2950415/4710434

Removing them also helps keep the MySQL and PostgreSQL database files as similar as possible, making them easier to maintain.

[albertlast]

Both done, thanks.

error() is now identical to the MySQL one. You're right that the null check was dead weight — null instanceof \PgSql\Connection is already false, so the instanceof test on its own returns '' in exactly the same cases.

Verified on PostgreSQL 17:

• A failing query with db_error_skip still comes back false quietly, and $this->error() with no argument returns the same message $this->error($this->connection) did.
• Without the flag it reaches the Database Error page and writes a database row to log_errors naming the calling file and line.
• 108 PHPUnit tests pass; a sweep of the usual pages logs nothing.