commit 46f46553c52cea776c5ed3e1563a51cc595aa0b5
parent 9e9a920f98d84a6556a86c2275df6b4ca31d3d10
Author: Christian Grothoff <christian@grothoff.org>
Date: Fri, 7 Aug 2026 00:12:21 +0200
fix compiler warning
Diffstat:
2 files changed, 111 insertions(+), 11 deletions(-)
diff --git a/src/backenddb/start.c b/src/backenddb/start.c
@@ -118,9 +118,43 @@ TALER_MERCHANTDB_commit (
PREPARE (pg,
"commit",
"COMMIT");
- qs = GNUNET_PQ_eval_prepared_non_select (pg->conn,
- "commit",
- params);
+ {
+ PGresult *result;
+
+ result = GNUNET_PQ_exec_prepared (pg->conn,
+ "commit",
+ params);
+ qs = GNUNET_PQ_eval_result (pg->conn,
+ "commit",
+ result);
+ /* PostgreSQL accepts COMMIT on a transaction it has already aborted: it
+ silently rolls the transaction back and answers PGRES_COMMAND_OK with the
+ command tag ROLLBACK. GNUNET_PQ_eval_result() cannot tell that apart from
+ a real commit, so every caller took the success path for a transaction
+ whose writes are gone. Report it as a soft error instead: sessions run
+ SERIALIZABLE, so the overwhelmingly likely cause is an unhandled
+ 40001/40P01 for which retrying is right; a caller with a retry loop gives
+ up after MAX_RETRIES and fails visibly, one without fails immediately.
+
+ The *success* path deliberately keeps returning exactly what
+ GNUNET_PQ_eval_prepared_non_select() returned before. That function is
+ GNUNET_PQ_exec_prepared() + GNUNET_PQ_eval_result() plus a
+ strtol(PQcmdTuples()) step that only runs when the status is already
+ GNUNET_DB_STATUS_SUCCESS_NO_RESULTS, and PQcmdTuples() on a COMMIT command
+ tag is the empty string -- so that step yields 0, which is
+ GNUNET_DB_STATUS_SUCCESS_NO_RESULTS again. */
+ if ( (0 <= qs) &&
+ (NULL != result) &&
+ (0 == strcmp ("ROLLBACK",
+ PQcmdStatus (result))) )
+ {
+ GNUNET_log (GNUNET_ERROR_TYPE_WARNING,
+ "Transaction `%s' was rolled back by the database instead of committed; a previous statement must have failed without being handled\n",
+ pg->transaction_name);
+ qs = GNUNET_DB_STATUS_SOFT_ERROR;
+ }
+ PQclear (result);
+ }
pg->transaction_name = NULL;
return qs;
}
diff --git a/src/backenddb/test_merchantdb.c b/src/backenddb/test_merchantdb.c
@@ -923,10 +923,6 @@ test_get_instance_auth_null (const struct InstanceData *instance)
{
struct TALER_MERCHANTDB_InstanceAuthSettings ias;
char sql[256];
- struct GNUNET_PQ_ExecuteStatement es[] = {
- GNUNET_PQ_make_execute (sql),
- GNUNET_PQ_EXECUTE_STATEMENT_END
- };
GNUNET_snprintf (sql,
sizeof (sql),
@@ -935,10 +931,17 @@ test_get_instance_auth_null (const struct InstanceData *instance)
" ,auth_salt=NULL"
" WHERE merchant_id='%s'",
instance->instance.id);
- TEST_COND_RET_ON_FAIL (GNUNET_OK ==
- GNUNET_PQ_exec_statements (pg->conn,
- es),
- "Failed to NULL out the instance authentication\n");
+ {
+ struct GNUNET_PQ_ExecuteStatement es[] = {
+ GNUNET_PQ_make_execute (sql),
+ GNUNET_PQ_EXECUTE_STATEMENT_END
+ };
+
+ TEST_COND_RET_ON_FAIL (GNUNET_OK ==
+ GNUNET_PQ_exec_statements (pg->conn,
+ es),
+ "Failed to NULL out the instance authentication\n");
+ }
memset (&ias,
42,
sizeof (ias));
@@ -10635,6 +10638,68 @@ test_mfa_challenges (void)
/**
+ * Test that COMMIT on a transaction PostgreSQL has already aborted is not
+ * reported as a success -- and that a clean COMMIT still returns exactly
+ * GNUNET_DB_STATUS_SUCCESS_NO_RESULTS, which is what every caller that
+ * compares the result against 0 relies on.
+ *
+ * @return 0 on success
+ */
+static int
+test_commit_on_aborted_transaction (void)
+{
+ struct GNUNET_PQ_ExecuteStatement mk[] = {
+ GNUNET_PQ_make_execute ("CREATE TABLE test_commit_rollback (x INT)"),
+ GNUNET_PQ_EXECUTE_STATEMENT_END
+ };
+ struct GNUNET_PQ_ExecuteStatement bad[] = {
+ GNUNET_PQ_make_execute ("SELECT 1/0"),
+ GNUNET_PQ_EXECUTE_STATEMENT_END
+ };
+ struct GNUNET_PQ_ExecuteStatement chk[] = {
+ GNUNET_PQ_make_execute (
+ "DO $$ BEGIN"
+ " IF EXISTS (SELECT FROM pg_class"
+ " WHERE relname='test_commit_rollback')"
+ " THEN RAISE EXCEPTION 'table survived a rollback';"
+ " END IF; END $$"),
+ GNUNET_PQ_EXECUTE_STATEMENT_END
+ };
+
+ TEST_COND_RET_ON_FAIL (GNUNET_OK ==
+ TALER_MERCHANTDB_start (pg,
+ "test-commit-aborted"),
+ "Failed to start transaction\n");
+ TEST_COND_RET_ON_FAIL (GNUNET_OK ==
+ GNUNET_PQ_exec_statements (pg->conn,
+ mk),
+ "Failed to create the scratch table\n");
+ /* provoke an error; from here on the transaction is doomed */
+ TEST_COND_RET_ON_FAIL (GNUNET_OK !=
+ GNUNET_PQ_exec_statements (pg->conn,
+ bad),
+ "Division by zero unexpectedly succeeded\n");
+ /* before the fix this returned GNUNET_DB_STATUS_SUCCESS_NO_RESULTS (0) */
+ TEST_COND_RET_ON_FAIL (0 >
+ TALER_MERCHANTDB_commit (pg),
+ "COMMIT of an aborted transaction reported success\n");
+ /* ...and the table is indeed gone, so 'success' would have been a lie */
+ TEST_COND_RET_ON_FAIL (GNUNET_OK ==
+ GNUNET_PQ_exec_statements (pg->conn,
+ chk),
+ "Scratch table survived the rollback\n");
+ TEST_COND_RET_ON_FAIL (GNUNET_OK ==
+ TALER_MERCHANTDB_start (pg,
+ "test-commit-clean"),
+ "Failed to start transaction\n");
+ TEST_COND_RET_ON_FAIL (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS ==
+ TALER_MERCHANTDB_commit (pg),
+ "COMMIT changed its return value on success\n");
+ return 0;
+}
+
+
+/**
* Function that runs all tests.
*
* @return 0 on success, 1 otherwise.
@@ -10658,6 +10723,7 @@ run_tests (void)
TEST_RET_ON_FAIL (test_inventory_deleted_webhook ());
TEST_RET_ON_FAIL (test_statistics ());
TEST_RET_ON_FAIL (test_mfa_challenges ());
+ TEST_RET_ON_FAIL (test_commit_on_aborted_transaction ());
return 0;
}
