fix(pgsql): surface mid-transaction backend errors instead of silently replaying

[renecannao]

Summary

Fixes a silent data-consistency hazard in the PostgreSQL code path: when a backend connection broke mid-transaction (e.g. postmaster SIGKILL, backend pg_terminate_backend), ProxySQL would silently replay the failed statement on a fresh backend connection as autocommit. The client's next COMMIT then landed on a connection with no open transaction and PostgreSQL emitted WARNING: there is no transaction in progress — which the application never saw.

Two commits:

1. 68c76eb42 fix(pgsql): mark transaction state unknown when backend connection is broken
   In compute_unknown_transaction_status(), force unknown_transaction_status = true whenever is_connected() == false. libpq's PQtransactionStatus() returns PQTRANS_UNKNOWN on a broken connection regardless of whether a transaction was active (no cached SERVER_STATUS_IN_TRANS equivalent to MySQL's), so without this branch IsActiveTransaction() wrongly reports false and the retry guard in handler_minus1_ClientLibraryError() doesn't trip.

2. 8e509d371 test(pgsql): regression guard for silent in-transaction retry fix
   New TAP test pgsql-retry_guard_in_txn_on_broken_backend-t that reproduces the bug: BEGIN + SELECT pg_sleep(3) inside the tx, terminate the backend mid-sleep via a direct superuser connection, assert the client sees PGRES_FATAL_ERROR. Pre-fix this returns PGRES_TUPLES_OK (silent replay).

Why the branch was previously commented out

The is_connected() == false branch was introduced in 65fc633cf (Jun 2024), then commented out in 24fecc1f6 (Jun 2025, "Add PostgreSQL extended query (prepared statement) support") alongside changes to the PQTRANS_UNKNOWN switch case. The surrounding work points to a workaround for transient states libpq can report during pipeline-mode error recovery. This fix is strictly narrower than that 2025 disable: it restores only the disconnect branch (PQstatus() != CONNECTION_OK, which is not a pipeline transient) and leaves the PQTRANS_UNKNOWN default: preservation intact, so pipeline-mode behavior is unchanged.

MySQL is not affected: the MySQL protocol caches SERVER_STATUS_IN_TRANS in mysql->server_status from the last OK packet and that bit survives the connection breaking.

Test plan

• make debug && make build_tap_test_debug
• cd test/infra/docker-pgsql16-single && ./docker-compose-init.bash
• cd test/tap/tests && ./pgsql-retry_guard_in_txn_on_broken_backend-t — passes on this branch, fails on v3.0 tip (regression guard works both directions).
• Pipeline-mode sanity pass: ./pgsql-extended_query_protocol_test-t, ./pgsql-transaction_state_comprehensive-t, ./pgsql-reg_test_5352_prepared_statement_refcount_race-t — confirms the narrower restore does not re-open whatever 24fecc1f6 was working around.
• Repro from the field: empirically exercised on the v3.0_pgsql_blogs branch via the unplanned-primary-failure lab — pre-fix reported zero errors on a 90 s workload across a SIGKILL of the primary postmaster; post-fix reports exactly eight errors (one per in-flight worker) and a sub-second failover window once the synthetic detection delay is backed out.

Summary by CodeRabbit

• New Features

  • Preserve client connections when a PostgreSQL backend dies mid-transaction: clients receive SQLSTATE 25P02 errors/notices and can recover with ROLLBACK/COMMIT semantics.

• Bug Fixes

  • Ensure transaction state is marked unknown on disconnected backends so mid-transaction backend deaths produce fatal errors rather than silent success.

• Tests

  • Added multiple TAP regression tests covering backend-kill, extended-query, and recovery flows; increased test data to reduce flakiness and updated unit test build flags.

• Chores

  • Enhanced CI lint workflow and improved test-group/source validation.

[coderabbitai]

📝 Walkthrough

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 39.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(pgsql): surface mid-transaction backend errors instead of silently replaying' directly summarizes the main objective of the PR: preventing silent replaying of failed statements when a backend breaks mid-transaction by instead surfacing the error to the client.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/pgsql-active-tx-on-broken-conn

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request addresses a critical data consistency issue in the PostgreSQL module where statements were silently retried on a new connection if the backend connection failed during an active transaction. By forcing the transaction status to unknown when a connection is lost, the retry logic correctly identifies the active transaction state and prevents unsafe re-execution. A new regression test has been implemented to verify this behavior and ensure that such failures are correctly propagated to the client as fatal errors. I have no feedback to provide.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp`:
- Line 75: The test currently only emits a diagnostic for the expected
"ROLLBACK" recovery and does not assert it actually succeeded; replace the
diagnostic that prints "ROLLBACK" with a TAP assertion that the ROLLBACK command
executed successfully (e.g. use ok(...) or is(...) on the function that runs
SQL/commands in the test so it verifies
exec("ROLLBACK")/run_sql("ROLLBACK")/txn->rollback() returned success), and do
the same replacement for the second occurrence around lines 168-174; keep the
assertion message like "ROLLBACK recovery after broken backend" so it clearly
documents the check.
- Around line 111-141: Pre-open the direct superuser connection with
openConnection and store the PGconn* (e.g. PGconn* direct) before spawning the
killer thread so the 500ms sleep only controls when pg_terminate_backend is
sent, not connection setup; then start the killer thread which uses that
pre-opened direct connection to run snprintf + PQexec for "SELECT
pg_terminate_backend(%d)" (using backend_pid) and sets kill_delivered, and
ensure PQclear/PQfinish still run (either in the killer thread or after joining)
to avoid leaks; keep the PQexec(cli, "SELECT pg_sleep(3)") call as the in-flight
query under test.
- Line 41: Remove the unused POSIX include directive "<unistd.h>" from the test;
verify no uses of POSIX symbols (search for usleep, sleep, close, getpid, read,
write, pipe, fork, exec) remain in the test code before deleting the include,
and then delete the `#include` <unistd.h> line (no other code changes required).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 60ec2520-cb18-45a8-a199-f836e2309bb4

📥 Commits

Reviewing files that changed from the base of the PR and between 872479a and 8e509d3.

📒 Files selected for processing (2)

• lib/PgSQL_Connection.cpp
• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
• GitHub Check: CI-builds / builds (ubuntu22,-tap)
• GitHub Check: CI-builds / builds (debian12,-dbg)
• GitHub Check: run / trigger
• GitHub Check: claude-review

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{cpp,h,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via #ifdef PROXYSQLGENAI, #ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters

Files:

• lib/PgSQL_Connection.cpp
• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

lib/**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

One class per file typically in lib/ directory

Files:

• lib/PgSQL_Connection.cpp

test/tap/tests/*-t.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/

Files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

🧠 Learnings (6)

📚 Learning: 2026-04-01T21:27:03.216Z

Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-01-20T09:34:19.124Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to test/tap/tests/*-t.cpp : Test files should follow naming pattern `test_*.cpp` or `*-t.cpp` in `test/tap/tests/`

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-01-20T07:40:34.938Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

🪛 Clang (14.0.6)

test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

[error] 41-41: 'unistd.h' file not found

(clang-diagnostic-error)

🔇 Additional comments (1)

lib/PgSQL_Connection.cpp (1)

1311-1320: LGTM — disconnected backends now block unsafe retry.

Setting unknown_transaction_status = true before consulting PQtransactionStatus() preserves the retry guard when libpq can only report an unknown state on a broken connection.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Remove the unused POSIX header flagged by static analysis.

<unistd.h> is not used in this test, and the analyzer reports it as unavailable. Dropping it avoids a portability/build failure without changing behavior.

🧹 Proposed fix

-#include <unistd.h>
 `#include` <cstdlib>

Verify the include has no symbol usage before removing it:

#!/bin/bash
# Expect: only the include line, with no sleep/usleep/close/getpid/etc. usage.
rg -n '#include <unistd\.h>|\b(usleep|sleep|close|getpid|read|write|pipe|fork|exec)\b' test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

🧰 Tools

🪛 Clang (14.0.6)

[error] 41-41: 'unistd.h' file not found

(clang-diagnostic-error)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp` at line 41,
Remove the unused POSIX include directive "<unistd.h>" from the test; verify no
uses of POSIX symbols (search for usleep, sleep, close, getpid, read, write,
pipe, fork, exec) remain in the test code before deleting the include, and then
delete the `#include` <unistd.h> line (no other code changes required).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Assert ROLLBACK recovery instead of only logging it.

The test summary says recovery is verified with ROLLBACK, but the current code only emits a diagnostic and would still pass that step if ROLLBACK failed.

✅ Proposed fix

-    plan(6);
+    plan(7);
@@
     // Recover: ROLLBACK clears the failed transaction on a pool-reusable
     // connection. A follow-up SELECT 1 should succeed.
     r = PQexec(cli, "ROLLBACK");
-    diag("post-kill ROLLBACK status: %s", PQresStatus(PQresultStatus(r)));
+    ExecStatusType rollback_st = PQresultStatus(r);
+    ok(rollback_st == PGRES_COMMAND_OK,
+       "ROLLBACK after backend kill succeeded (%s)", PQresStatus(rollback_st));
     PQclear(r);
 
     r = PQexec(cli, "SELECT 1");

Also applies to: 168-174

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp` at line 75,
The test currently only emits a diagnostic for the expected "ROLLBACK" recovery
and does not assert it actually succeeded; replace the diagnostic that prints
"ROLLBACK" with a TAP assertion that the ROLLBACK command executed successfully
(e.g. use ok(...) or is(...) on the function that runs SQL/commands in the test
so it verifies exec("ROLLBACK")/run_sql("ROLLBACK")/txn->rollback() returned
success), and do the same replacement for the second occurrence around lines
168-174; keep the assertion message like "ROLLBACK recovery after broken
backend" so it clearly documents the check.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Avoid timing flake in the kill window.

Opening the direct PostgreSQL connection inside the killer thread after the 500ms delay can push pg_terminate_backend() past the 3s pg_sleep() on slow CI, causing a false regression failure. Pre-open the direct connection before starting the in-flight query so the delay controls only the kill timing.

🧪 Proposed hardening

     // Fire the kill from a separate thread so it lands while the main thread
     // is blocked inside PQexec(SELECT pg_sleep). We use a DIRECT superuser
     // connection to the primary (not ProxySQL) so the kill path is unrelated
     // to the code path under test.
+    PGconn* direct = openConnection(cl.pgsql_server_host, cl.pgsql_server_port,
+                                    cl.pgsql_server_username, cl.pgsql_server_password,
+                                    "PG-direct (superuser)");
     std::atomic<bool> kill_delivered(false);
     std::thread killer([&]() {
         // Let the main thread enter pg_sleep before we fire.
         std::this_thread::sleep_for(std::chrono::milliseconds(500));
 
-        PGconn* direct = openConnection(cl.pgsql_server_host, cl.pgsql_server_port,
-                                        cl.pgsql_server_username, cl.pgsql_server_password,
-                                        "PG-direct (superuser)");
         if (!direct) return;
 
         char q[96];
         snprintf(q, sizeof(q), "SELECT pg_terminate_backend(%d)", backend_pid);
@@
         }
         PQclear(kr);
-        PQfinish(direct);
     });
@@
     r = PQexec(cli, "SELECT pg_sleep(3)");
     killer.join();
+    if (direct) PQfinish(direct);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Fire the kill from a separate thread so it lands while the main thread
	// is blocked inside PQexec(SELECT pg_sleep). We use a DIRECT superuser
	// connection to the primary (not ProxySQL) so the kill path is unrelated
	// to the code path under test.
	std::atomic<bool> kill_delivered(false);
	std::thread killer([&]() {
	// Let the main thread enter pg_sleep before we fire.
	std::this_thread::sleep_for(std::chrono::milliseconds(500));
	PGconn* direct = openConnection(cl.pgsql_server_host, cl.pgsql_server_port,
	cl.pgsql_server_username, cl.pgsql_server_password,
	"PG-direct (superuser)");
	if (!direct) return;
	char q[96];
	snprintf(q, sizeof(q), "SELECT pg_terminate_backend(%d)", backend_pid);
	PGresult* kr = PQexec(direct, q);
	if (PQresultStatus(kr) == PGRES_TUPLES_OK && PQntuples(kr) == 1) {
	// pg_terminate_backend returns boolean: 't' iff signal delivered.
	kill_delivered.store(PQgetvalue(kr, 0, 0)[0] == 't');
	}
	PQclear(kr);
	PQfinish(direct);
	});
	// This query will be in-flight when the terminate_backend signal arrives.
	// With the fix, ProxySQL refuses to retry it (because the session is in
	// an explicit transaction) and the error propagates to the client. Without
	// the fix, ProxySQL retries on a fresh backend as autocommit and we'd see
	// the sleep complete successfully — the silent-corruption path.
	r = PQexec(cli, "SELECT pg_sleep(3)");
	// Fire the kill from a separate thread so it lands while the main thread
	// is blocked inside PQexec(SELECT pg_sleep). We use a DIRECT superuser
	// connection to the primary (not ProxySQL) so the kill path is unrelated
	// to the code path under test.
	PGconn* direct = openConnection(cl.pgsql_server_host, cl.pgsql_server_port,
	cl.pgsql_server_username, cl.pgsql_server_password,
	"PG-direct (superuser)");
	std::atomic<bool> kill_delivered(false);
	std::thread killer([&]() {
	// Let the main thread enter pg_sleep before we fire.
	std::this_thread::sleep_for(std::chrono::milliseconds(500));
	if (!direct) return;
	char q[96];
	snprintf(q, sizeof(q), "SELECT pg_terminate_backend(%d)", backend_pid);
	PGresult* kr = PQexec(direct, q);
	if (PQresultStatus(kr) == PGRES_TUPLES_OK && PQntuples(kr) == 1) {
	// pg_terminate_backend returns boolean: 't' iff signal delivered.
	kill_delivered.store(PQgetvalue(kr, 0, 0)[0] == 't');
	}
	PQclear(kr);
	});
	// This query will be in-flight when the terminate_backend signal arrives.
	// With the fix, ProxySQL refuses to retry it (because the session is in
	// an explicit transaction) and the error propagates to the client. Without
	// the fix, ProxySQL retries on a fresh backend as autocommit and we'd see
	// the sleep complete successfully — the silent-corruption path.
	r = PQexec(cli, "SELECT pg_sleep(3)");
	killer.join();
	if (direct) PQfinish(direct);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp` around lines
111 - 141, Pre-open the direct superuser connection with openConnection and
store the PGconn* (e.g. PGconn* direct) before spawning the killer thread so the
500ms sleep only controls when pg_terminate_backend is sent, not connection
setup; then start the killer thread which uses that pre-opened direct connection
to run snprintf + PQexec for "SELECT pg_terminate_backend(%d)" (using
backend_pid) and sets kill_delivered, and ensure PQclear/PQfinish still run
(either in the killer thread or after joining) to avoid leaks; keep the
PQexec(cli, "SELECT pg_sleep(3)") call as the in-flight query under test.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/tap/groups/check_groups.py`:
- Around line 66-94: In find_source_tests, avoid unconditionally adding files
that merely end with "-t": change the loop variable name dirpath to _dirpath,
and for the extensionless branch (the entry.endswith("-t") case) check that the
file is executable before adding it (use os.access(os.path.join(_dirpath,
entry), os.X_OK)) so editor backups or stray artifacts aren’t reported; keep the
existing behavior for recognized script extensions (SCRIPT_EXTS) and continue
using entry[:-len(ext)] when adding those entries.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8e9773ae-cff3-4001-af0c-bf3ecde40dfd

📥 Commits

Reviewing files that changed from the base of the PR and between 8e509d3 and e24ac09.

📒 Files selected for processing (5)

• .github/workflows/CI-lint-groups-json.yml
• test/tap/groups/check_groups.py
• test/tap/groups/groups.json
• test/tap/tests/fast_forward_grace_close.cpp
• test/tap/tests/unit/Makefile

✅ Files skipped from review due to trivial changes (1)

• test/tap/groups/groups.json

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
• GitHub Check: CI-builds / builds (debian12,-dbg)
• GitHub Check: CI-builds / builds (ubuntu22,-tap)
• GitHub Check: run / trigger
• GitHub Check: claude-review

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{cpp,h,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via #ifdef PROXYSQLGENAI, #ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters

Files:

• test/tap/tests/fast_forward_grace_close.cpp

🧠 Learnings (7)

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness

Applied to files:

• test/tap/tests/unit/Makefile
• test/tap/tests/fast_forward_grace_close.cpp

📚 Learning: 2026-04-01T21:27:03.216Z

Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.

Applied to files:

• test/tap/tests/unit/Makefile
• test/tap/tests/fast_forward_grace_close.cpp

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to test/tap/groups/groups.json : Register new test files in `test/tap/groups/groups.json` for test discovery via pattern rules in `test/tap/tests/Makefile`

Applied to files:

• test/tap/tests/unit/Makefile
• .github/workflows/CI-lint-groups-json.yml
• test/tap/groups/check_groups.py

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to test/tap/tests/*-t.cpp : Test files should follow naming pattern `test_*.cpp` or `*-t.cpp` in `test/tap/tests/`

Applied to files:

• test/tap/tests/unit/Makefile

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code

Applied to files:

• test/tap/tests/unit/Makefile

📚 Learning: 2026-01-20T09:34:19.124Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.

Applied to files:

• test/tap/tests/fast_forward_grace_close.cpp

📚 Learning: 2026-01-20T07:40:34.938Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).

Applied to files:

• test/tap/tests/fast_forward_grace_close.cpp

🪛 Ruff (0.15.10)

test/tap/groups/check_groups.py

[warning] 85-85: Loop control variable dirpath not used within loop body

Rename unused dirpath to _dirpath

(B007)

🔇 Additional comments (4)

test/tap/tests/fast_forward_grace_close.cpp (1)

62-73: LGTM — binlog volume increase is well justified.

The rationale is clearly documented in the comment, and bumping to ~50 MB gives sufficient margin over autotuned recv buffers so the EOF marker cannot slip through during the 8s grace window. Be aware this adds ~50 MB of INSERTs through ProxySQL at startup; on slow CI runners this may noticeably extend test runtime, but it's a reasonable tradeoff for reducing flakiness.

test/tap/tests/unit/Makefile (1)

320-328: LGTM — $(WASAN) added to standalone link lines.

Adding $(WASAN) here makes the two standalone rules consistent with the pattern rule (which propagates it via $(OPT)), resolving the ASAN link errors mentioned in the PR description. No other concerns.

.github/workflows/CI-lint-groups-json.yml (1)

3-16: LGTM — always lint + source-registration check.

Running lint_groups_json.py and check_groups.py --source on every PR/push (no paths: filter) ensures that adding a new TAP source without a groups.json entry fails CI, which matches the PR intent. workflow_dispatch for manual runs is a nice touch.

test/tap/groups/check_groups.py (1)

132-155: LGTM — mode-dependent severity is correctly centralized.

missing_on_disk_is_error cleanly encodes the asymmetry: in source mode, groups.json entries without matching sources are only a NOTE (since some binaries come from differently-named sources via explicit Makefile targets), while on-disk-but-missing-from-groups.json remains an error in both modes. Matches the documented behavior.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

find_source_tests will pick up any file ending in -t, including non-source artifacts.

Unlike find_executable_tests (which filters os.access(..., X_OK) and excludes .cpp), find_source_tests adds every filename ending in -t unconditionally at line 87-88. A stray foo-t file (e.g., editor backup, generated artifact left over in a dev checkout) would be reported as an unregistered test. CI runs on fresh checkouts so this is low risk in practice, but you may want to at least require the extensionless branch to be executable, to mirror find_executable_tests.

Also, per Ruff B007, dirpath at line 85 is unused — rename to _dirpath.

♻️ Suggested tightening

-        for dirpath, _, filenames in os.walk(scan_root):
+        for _dirpath, _, filenames in os.walk(scan_root):
             for entry in filenames:
                 if entry.endswith("-t"):
-                    sources.add(entry)
-                    continue
+                    path = os.path.join(_dirpath, entry)
+                    if os.path.isfile(path) and os.access(path, os.X_OK):
+                        sources.add(entry)
+                    continue
                 for ext in SCRIPT_EXTS:
                     if entry.endswith("-t" + ext):
                         sources.add(entry[: -len(ext)])
                         break

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	def find_source_tests(tap_root):
	"""Find all TAP test source files under the TAP test directories.
	Recognized forms:
	- foo-t.cpp / foo-t.sh / foo-t.py / foo-t.php -> expected binary 'foo-t'
	- executable file named 'foo-t' with no extension -> 'foo-t'
	Unlike find_executable_tests(), this does not require a build: it scans
	the source tree, so it works on a fresh checkout in a lint CI job.
	"""
	scan_roots = [
	os.path.join(tap_root, "tests"),
	os.path.join(tap_root, "tests_with_deps"),
	]
	sources = set()
	for scan_root in scan_roots:
	if not os.path.isdir(scan_root):
	continue
	for dirpath, _, filenames in os.walk(scan_root):
	for entry in filenames:
	if entry.endswith("-t"):
	sources.add(entry)
	continue
	for ext in SCRIPT_EXTS:
	if entry.endswith("-t" + ext):
	sources.add(entry[: -len(ext)])
	break
	return sources
	def find_source_tests(tap_root):
	"""Find all TAP test source files under the TAP test directories.
	Recognized forms:
	- foo-t.cpp / foo-t.sh / foo-t.py / foo-t.php -> expected binary 'foo-t'
	- executable file named 'foo-t' with no extension -> 'foo-t'
	Unlike find_executable_tests(), this does not require a build: it scans
	the source tree, so it works on a fresh checkout in a lint CI job.
	"""
	scan_roots = [
	os.path.join(tap_root, "tests"),
	os.path.join(tap_root, "tests_with_deps"),
	]
	sources = set()
	for scan_root in scan_roots:
	if not os.path.isdir(scan_root):
	continue
	for _dirpath, _, filenames in os.walk(scan_root):
	for entry in filenames:
	if entry.endswith("-t"):
	path = os.path.join(_dirpath, entry)
	if os.path.isfile(path) and os.access(path, os.X_OK):
	sources.add(entry)
	continue
	for ext in SCRIPT_EXTS:
	if entry.endswith("-t" + ext):
	sources.add(entry[: -len(ext)])
	break
	return sources

🧰 Tools

🪛 Ruff (0.15.10)

[warning] 85-85: Loop control variable dirpath not used within loop body

Rename unused dirpath to _dirpath

(B007)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/groups/check_groups.py` around lines 66 - 94, In find_source_tests,
avoid unconditionally adding files that merely end with "-t": change the loop
variable name dirpath to _dirpath, and for the extensionless branch (the
entry.endswith("-t") case) check that the file is executable before adding it
(use os.access(os.path.join(_dirpath, entry), os.X_OK)) so editor backups or
stray artifacts aren’t reported; keep the existing behavior for recognized
script extensions (SCRIPT_EXTS) and continue using entry[:-len(ext)] when adding
those entries.

[coderabbitai]

🧹 Nitpick comments (1)

test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp (1)

122-152: Optional: tolerate a slow query registration in pg_stat_activity.

The killer waits 500ms then does a single pg_stat_activity lookup. On a loaded CI host the in-flight pg_sleep query may not yet be visible as state='active' at that instant, which would yield 0 rows, leave kill_delivered=false, and fail the assertion on line 162 even though the fix is present. Since the in-flight query sleeps for 3s, a short bounded retry (e.g., up to ~2s, 100–200ms intervals) on an empty result set would make the test more robust without weakening it. Not a correctness bug — just a flake-hardening suggestion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp` around lines
122 - 152, The killer thread's single-shot pg_stat_activity lookup (in the
lambda that creates PGconn* direct and runs the find_and_kill query via
PQexecParams) can return 0 rows on slow CI; change that block to retry the
parameterized lookup when PQresultStatus is PGRES_TUPLES_OK but PQntuples(kr) ==
0 (and for transient errors) in a short loop (e.g., total timeout ~2s with
100–200ms sleeps) until either a row is found or the timeout elapses, then set
kill_delivered as before when a tuple is returned; ensure PQclear is called each
iteration and break on success or non-recoverable error so we don't leak results
or connections.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp`:
- Around line 122-152: The killer thread's single-shot pg_stat_activity lookup
(in the lambda that creates PGconn* direct and runs the find_and_kill query via
PQexecParams) can return 0 rows on slow CI; change that block to retry the
parameterized lookup when PQresultStatus is PGRES_TUPLES_OK but PQntuples(kr) ==
0 (and for transient errors) in a short loop (e.g., total timeout ~2s with
100–200ms sleeps) until either a row is found or the timeout elapses, then set
kill_delivered as before when a tuple is returned; ensure PQclear is called each
iteration and break on success or non-recoverable error so we don't leak results
or connections.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3dc0ca59-569b-45ca-b39a-16e59ac420dd

📥 Commits

Reviewing files that changed from the base of the PR and between e24ac09 and 8857543.

📒 Files selected for processing (1)

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: CI-builds / builds (ubuntu22,-tap)
• GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
• GitHub Check: CI-builds / builds (debian12,-dbg)
• GitHub Check: run / trigger
• GitHub Check: claude-review

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{cpp,h,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via #ifdef PROXYSQLGENAI, #ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters

Files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

test/tap/tests/*-t.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/

Files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

🧠 Learnings (8)

📓 Common learnings

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.

📚 Learning: 2026-04-01T21:27:03.216Z

Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-01-20T09:34:19.124Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-04-17T16:31:58.178Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.178Z
Learning: Applies to test/tap/tests/*-t.cpp : Test files should follow naming pattern `test_*.cpp` or `*-t.cpp` in `test/tap/tests/`

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-03-26T16:39:02.446Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

📚 Learning: 2026-01-20T07:40:34.938Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).

Applied to files:

• test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

🪛 Clang (14.0.6)

test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp

[error] 47-47: 'unistd.h' file not found

(clang-diagnostic-error)

🔇 Additional comments (1)

test/tap/tests/pgsql-retry_guard_in_txn_on_broken_backend-t.cpp (1)

47-47: Keep <unistd.h>; it's required for getpid().

A prior review suggested removing this include, but line 111 calls getpid(), which on POSIX is declared in <unistd.h>. Removing it would rely on transitive inclusion and is not portable. The Clang static-analysis "file not found" is a sandbox header-path issue, not a real defect. Leave as-is.

[coderabbitai]

Actionable comments posted: 9

🧹 Nitpick comments (2)

test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp (1)

663-674: Minor: clean up the plan-accounting comment.

The comment reads like working scratch notes ("wait counted = 11 -- let me recount manually below") rather than final documentation. The count is correct, just consider tightening the prose so the plan rationale reads cleanly for future maintainers.

✏️ Proposed cleanup

-	// Plan breakdown:
-	// case A: 8 assertions (BEGIN, ERR shape, notice shape, no-57P01-in-notice,
-	//   PQstatus, PQtransactionStatus, reject-batch, ROLLBACK WORK recovers,
-	//   post-recovery SELECT 1, victim gone, counters) -- wait counted = 11
-	//   let me recount manually below.
-	// case A ok() count = 11
-	// case B ok() count = 3
-	// case C ok() count = 1
-	// case D ok() count = 2
-	// Total = 17
+	// Plan breakdown (ok() calls per case on the success path; abort paths
+	// use emit_fail_fill() to match these counts):
+	//   case A: 11 (BEGIN, err shape, notice shape, no-57P01, PQstatus,
+	//               PQtransactionStatus, reject-batch, ROLLBACK WORK,
+	//               post-recovery SELECT, victim-gone, counters)
+	//   case B:  3 (COMMIT recovers, 25P01 notice, counters)
+	//   case C:  1 (counters unchanged)
+	//   case D:  2 (session terminated, counters unchanged)
 	plan(17);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp` around lines 663 - 674, The
plan-accounting comment inside main near the plan(17) call is written as scratch
notes; clean it up to concise documentation by removing informal phrases like
"let me recount manually below" and "wait counted = 11", keep the breakdown of
cases A–D and their assertion counts (case A = 11, B = 3, C = 1, D = 2) and
ensure the total matches plan(17); update the comment text around main and
plan(17) to a single clear sentence or brief bullet-style explanation so future
maintainers understand the rationale without informal notes.

lib/PgSQL_Session.cpp (1)

104-119: Rename the new constant to UPPER_SNAKE_CASE.

kTxPoisonedNoticeMaxLen violates the repository constant naming rule.

Suggested rename

-static constexpr unsigned int kTxPoisonedNoticeMaxLen = 4096;
+static constexpr unsigned int TX_POISONED_NOTICE_MAX_LEN = 4096;
 
 static std::string sanitize_backend_err_msg(const std::string& src) {
 	std::string out;
-	const size_t cap = std::min<size_t>(src.size(), kTxPoisonedNoticeMaxLen);
+	const size_t cap = std::min<size_t>(src.size(), TX_POISONED_NOTICE_MAX_LEN);
 	out.reserve(cap);
 	for (size_t i = 0; i < cap; ++i) {
 		char c = src[i];
 		out.push_back(c == '\0' ? '?' : c);
 	}
-	if (src.size() > kTxPoisonedNoticeMaxLen) {
+	if (src.size() > TX_POISONED_NOTICE_MAX_LEN) {
 		out.append("...[truncated]");
 	}

As per coding guidelines, Constants and macros should use UPPER_SNAKE_CASE naming.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/PgSQL_Session.cpp` around lines 104 - 119, Rename the constant
kTxPoisonedNoticeMaxLen to an UPPER_SNAKE_CASE identifier (for example
TX_POISONED_NOTICE_MAX_LEN) and update every reference to it (including the
declaration and its use inside sanitize_backend_err_msg) so the code follows the
repository constant naming convention; ensure the value and constexpr/unsigned
int type remain unchanged and run a build to verify no remaining references to
the old name.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@include/PgSQL_Session.h`:
- Around line 396-401: Update the misleading comment blocks that describe which
simple-query packets clear transaction poison: in the comment above the function
handler_poisoned_simple_query and the corresponding comment around the later
block (previously listing ROLLBACK TO SAVEPOINT as a recovery action), remove
ROLLBACK TO SAVEPOINT from the list of commands that "clear the poison" and
instead include it among the rejected statements that leave the session poisoned
(i.e., keep it in the ERROR 25P02/reject category); reference the
handler_poisoned_simple_query symbol to locate the first comment and the
matching poisoned-handling block for the second comment and make the wording
explicitly state that only ROLLBACK, COMMIT and ABORT clear poison while
ROLLBACK TO SAVEPOINT is rejected and retains the poison.

In `@lib/PgSQL_Session.cpp`:
- Around line 2747-2790: The log embeds the uncapped backend string via
myconn->get_error_code_with_message(), so replace that with the
already-sanitized/capped backend_err_msg (produced by sanitize_backend_err_msg)
when calling proxy_warning in this block (i.e., use backend_err_msg.c_str() or
build a small capped string combining code+backend_err_msg) so the warning log
cannot be amplified by an oversized backend message; update the proxy_warning
call site accordingly (references: backend_err_msg, proxy_warning,
get_error_code_with_message(), sanitize_backend_err_msg).
- Around line 2300-2328: The code treats Flush ('H') as unsupported in a
poisoned extended-query transaction; update the conditional in the tx_poisoned
check (the block that inspects command in PgSQL_Session.cpp where tx_poisoned
and command != 'Q' && command != 'X' are tested) to include 'H' alongside
'P','B','D','C','E' so that when tx_poisoned and command == 'H' the packet is
silently swallowed (l_free(pkt.size, pkt.ptr); continue;) just like
Parse/Bind/Describe/Close/Execute, leaving the single Sync-path response
unchanged.
- Around line 495-575: skip_ws_and_comments currently swallows unterminated /*
... */ and should return a bool to signal malformed input; change its signature
to return false on malformed/unterminated block comments and true on success,
then update all call sites (the initial call skip_ws_and_comments(sql, sql_len)
and the three subsequent calls on rest at the matches_kw checks and semicolon
handling) to check the return value and set kind = KIND_REJECT if it returns
false; ensure you propagate the same behavior for the optional WORK/TRANSACTION
consumption paths so any failed comment-skip forces rejection and leaves
tx_poisoned intact.

In `@test/tap/tests/Makefile`:
- Around line 191-195: The Makefile targets galera_1_timeout_count and
galera_2_timeout_no_count compile ../../../src/SQLite3_Server.cpp and
../../../lib/ProxySQL_Admin.cpp but do not list them as prerequisites; update
the rules for targets galera_1_timeout_count and galera_2_timeout_no_count to
include ../../../src/SQLite3_Server.cpp and ../../../lib/ProxySQL_Admin.cpp in
their prerequisite lists so changes to those sources trigger a rebuild of the
corresponding binaries.

In `@test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp`:
- Around line 68-76: set_admin_bool currently treats a 0-row UPDATE as success
and RestorePreserveClientAdminVar's destructor always restores true instead of
the original value, risking leaked global config; change set_admin_bool (the
function that runs the snprintf/PQexec sequence) to check the PGresult for
affected rows (use PQcmdTuples or equivalent) and return false when no rows were
updated, and modify RestorePreserveClientAdminVar to read and store the current
value of "pgsql-preserve_client_on_broken_backend_in_tx" before performing the
update and then restore that captured value in its destructor rather than
hardcoding true.
- Around line 252-257: Replace the failing ok(0, "SKIPPED: ...") loop with a
proper TAP skip: when kr.kill_delivered is false, emit a diagnostic with
diag("poison: killer failed to deliver pg_terminate_backend within 6s") and call
skip(9, "poison did not fire") to mark the remaining 9 tests as skipped; still
ensure you PQclear(kr.pgresult), PQfinish(cli) and return exit_status() in that
branch (references: kr.kill_delivered, kr.pgresult, PQclear, PQfinish,
exit_status).
- Around line 259-263: The diagnostic arguments dereference kr.pgresult
unconditionally and use the GNU ?: extension; change the three inline
expressions to be null-safe and C++17-compliant by guarding
PQresultStatus/PQresultErrorField calls with a kr.pgresult check and replacing
?: with the standard ternary operator. For example, when building the
status/severity/sqlstate strings for the ok(...) call in the test where
rejected_25P02_as_error(kr.pgresult) is used, compute temporary const char* (or
std::string) values using checks like kr.pgresult ?
PQresStatus(PQresultStatus(kr.pgresult)) : "(none)" and kr.pgresult &&
PQresultErrorField(kr.pgresult, PG_DIAG_SEVERITY) ?
PQresultErrorField(kr.pgresult, PG_DIAG_SEVERITY) : "(none)" (and similarly for
PG_DIAG_SQLSTATE), then pass those temporaries into the diagnostic format
instead of dereferencing kr.pgresult inline.

In `@test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp`:
- Around line 176-198: Remove the unused NoticeCollector struct and its callback
stub (symbols: NoticeCollector, NoticeCollector::callback) from the test file to
avoid confusion; ensure there are no references to it elsewhere (tests use
NoticeBag and notice_receiver_cb), delete the whole struct and its trailing
explanatory comment block so the file only contains the active notice-handling
implementation (NoticeBag/notice_receiver_cb).

---

Nitpick comments:
In `@lib/PgSQL_Session.cpp`:
- Around line 104-119: Rename the constant kTxPoisonedNoticeMaxLen to an
UPPER_SNAKE_CASE identifier (for example TX_POISONED_NOTICE_MAX_LEN) and update
every reference to it (including the declaration and its use inside
sanitize_backend_err_msg) so the code follows the repository constant naming
convention; ensure the value and constexpr/unsigned int type remain unchanged
and run a build to verify no remaining references to the old name.

In `@test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp`:
- Around line 663-674: The plan-accounting comment inside main near the plan(17)
call is written as scratch notes; clean it up to concise documentation by
removing informal phrases like "let me recount manually below" and "wait counted
= 11", keep the breakdown of cases A–D and their assertion counts (case A = 11,
B = 3, C = 1, D = 2) and ensure the total matches plan(17); update the comment
text around main and plan(17) to a single clear sentence or brief bullet-style
explanation so future maintainers understand the rationale without informal
notes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0af022c8-e51e-4ebf-8b79-afb8fad68cf2

📥 Commits

Reviewing files that changed from the base of the PR and between 8857543 and da02e17.

📒 Files selected for processing (9)

• include/PgSQL_Session.h
• include/PgSQL_Thread.h
• include/proxysql_structs.h
• lib/PgSQL_Session.cpp
• lib/PgSQL_Thread.cpp
• test/tap/groups/groups.json
• test/tap/tests/Makefile
• test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp
• test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: CI-builds / builds (debian12,-dbg)
• GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
• GitHub Check: CI-builds / builds (ubuntu22,-tap)
• GitHub Check: run / trigger
• GitHub Check: claude-review

🧰 Additional context used

📓 Path-based instructions (5)

include/**/*.{h,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Use #ifndef __CLASS_*_H include guards in header files

Files:

• include/proxysql_structs.h
• include/PgSQL_Session.h
• include/PgSQL_Thread.h

**/*.{cpp,h,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via #ifdef PROXYSQLGENAI, #ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters

Files:

• include/proxysql_structs.h
• include/PgSQL_Session.h
• lib/PgSQL_Thread.cpp
• test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp
• include/PgSQL_Thread.h
• test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp
• lib/PgSQL_Session.cpp

test/tap/groups/groups.json

📄 CodeRabbit inference engine (CLAUDE.md)

Register new test files in test/tap/groups/groups.json for test discovery via pattern rules in test/tap/tests/Makefile

Files:

• test/tap/groups/groups.json

lib/**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

One class per file typically in lib/ directory

Files:

• lib/PgSQL_Thread.cpp
• lib/PgSQL_Session.cpp

test/tap/tests/*-t.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/

Files:

• test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp
• test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp

🧠 Learnings (13)

📓 Common learnings

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.

📚 Learning: 2026-04-17T16:31:58.196Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/groups/groups.json : Register new test files in `test/tap/groups/groups.json` for test discovery via pattern rules in `test/tap/tests/Makefile`

Applied to files:

• test/tap/groups/groups.json

📚 Learning: 2026-04-01T21:27:03.216Z

Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.

Applied to files:

• test/tap/groups/groups.json
• test/tap/tests/Makefile
• test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp
• test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp
• lib/PgSQL_Session.cpp

📚 Learning: 2026-01-14T11:36:39.934Z

Learnt from: rahim-kanji
Repo: sysown/proxysql PR: 5299
File: lib/PgSQL_Session.cpp:5173-5233
Timestamp: 2026-01-14T11:36:39.934Z
Learning: The function `PgSQL_Session::send_parameter_error_response()` in `lib/PgSQL_Session.cpp` has a default parameter for its second argument (PGSQL_ERROR_CODES code) with the default value of PGSQL_ERROR_CODES::ERRCODE_INVALID_TEXT_REPRESENTATION, as declared in include/PgSQL_Session.h. Single-argument calls to this function are valid.

Applied to files:

• include/PgSQL_Session.h
• lib/PgSQL_Session.cpp

📚 Learning: 2026-04-17T16:31:58.196Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness

Applied to files:

• test/tap/tests/Makefile
• test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp
• test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp

📚 Learning: 2026-04-17T16:31:58.196Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code

Applied to files:

• test/tap/tests/Makefile
• lib/PgSQL_Session.cpp

📚 Learning: 2026-01-20T09:34:27.165Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.

Applied to files:

• test/tap/tests/Makefile

📚 Learning: 2026-04-17T16:31:58.196Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Use feature flags `PROXYSQL31=1` for v3.1.x Innovative tier and `PROXYSQLGENAI=1` for v4.0.x AI/MCP tier; PROXYSQLGENAI implies both PROXYSQL31 and PROXYSQLFFTO/PROXYSQLTSDB

Applied to files:

• lib/PgSQL_Thread.cpp

📚 Learning: 2026-04-17T16:31:58.196Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/tests/*-t.cpp : Test files should follow naming pattern `test_*.cpp` or `*-t.cpp` in `test/tap/tests/`

Applied to files:

• test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp

📚 Learning: 2026-01-20T09:34:19.124Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.

Applied to files:

• test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp
• test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp

📚 Learning: 2026-04-17T16:31:58.196Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to **/*.{cpp,h,hpp} : Use pthread mutexes for synchronization and `std::atomic<>` for counters

Applied to files:

• include/PgSQL_Thread.h

📚 Learning: 2026-03-26T16:39:02.446Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:39:02.446Z
Learning: In ProxySQL's lib/mysql_connection.cpp, `SHOW WARNINGS` detection for both `update_warning_count_from_connection()` and the `add_eof()` call in `ASYNC_USE_RESULT_CONT` intentionally uses `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This means the fix/feature does not work when `mysql-query_digests_keep_comment=1` (digest_text contains comments) or `mysql-query_digests=0` (digest_text is unavailable) — these configurations are explicitly excluded from the regression test for `reg_test_5306-show_warnings_with_comment-t`. This design is consistent across the codebase and is an accepted, documented limitation.

Applied to files:

• lib/PgSQL_Session.cpp

📚 Learning: 2026-01-20T07:40:34.938Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).

Applied to files:

• lib/PgSQL_Session.cpp

🪛 Clang (14.0.6)

test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp

[error] 48-48: 'unistd.h' file not found

(clang-diagnostic-error)

test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp

[error] 26-26: 'unistd.h' file not found

(clang-diagnostic-error)

🪛 Cppcheck (2.20.0)

lib/PgSQL_Thread.cpp

[warning] 4429-4429: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 4431-4431: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 4435-4435: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 4437-4437: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 4443-4443: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 4445-4445: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

🔇 Additional comments (5)

test/tap/groups/groups.json (1)

151-160: LGTM: the new PostgreSQL TAP tests are registered.

The three new test entries are discoverable via groups.json and consistently assigned to legacy-g2. As per coding guidelines, test/tap/groups/groups.json: Register new test files in test/tap/groups/groups.json for test discovery via pattern rules in test/tap/tests/Makefile.

include/proxysql_structs.h (1)

1110-1447: LGTM: the thread-local flag is declared consistently.

The new PostgreSQL thread variable is added in both the defining and extern sections, matching the surrounding thread-local configuration pattern.

lib/PgSQL_Thread.cpp (2)

379-379: Config flag wiring looks consistent.

The new preserve_client_on_broken_backend_in_tx variable is listed, defaulted, registered as a bool, and refreshed into the per-thread runtime flag.

Also applies to: 1177-1177, 2203-2203, 3971-3971

---

4248-4250: Tx-poison counters are wired coherently.

The counters are initialized per thread, rendered in stats_pgsql_global, and aggregated with the same guard pattern as the existing status getters.

Also applies to: 4427-4447, 5581-5601

include/PgSQL_Thread.h (1)

246-251: LGTM — consistent with existing per-thread counter pattern.

The new per-thread counters match the established design in this struct (see the file-level comment on line 240-242: "status variables are per thread only... no need for atomic operation"), and the aggregation responsibility is delegated to PgSQL_Threads_Handler. Naming and types follow convention.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect the poisoned simple-query classifier and tests for savepoint recovery behavior.
# Expected: ROLLBACK TO SAVEPOINT should not be classified as a full recovery command that clears tx_poisoned.

rg -n -C4 'ROLLBACK TO|SAVEPOINT|tx_poisoned|handler_poisoned_simple_query|PoisonTransaction' --iglob '*PgSQL_Session*' --iglob '*pgsql-tx_poisoned*'

Repository: sysown/proxysql

Length of output: 33011

---

🌐 Web query:

PostgreSQL ROLLBACK TO SAVEPOINT transaction remains active ReadyForQuery transaction status

💡 Result:

In PostgreSQL, executing ROLLBACK TO SAVEPOINT rolls back changes made after the named savepoint within the current transaction but does not end the overall transaction. The transaction remains active, allowing further statements to be executed until an explicit COMMIT or (full) ROLLBACK is issued. After processing the ROLLBACK TO SAVEPOINT command successfully, the PostgreSQL backend sends a ReadyForQuery (RFQ) message to the frontend with transaction status indicator 'T' (in transaction block), confirming the connection is ready for the next command while still inside the active transaction. This is the expected behavior, not an error—the transaction status remains active (not 'I' for idle), as the top-level transaction continues. Only a full ROLLBACK, COMMIT, or session termination ends the transaction and returns the status to 'I' (idle). Savepoints create subtransactions, but rolling back to one keeps the parent transaction open.

Citations:

• 1: https://www.postgresql.org/docs/current/sql-rollback-to.html
• 2: https://www.postgresql.org/docs/15/sql-savepoint.html
• 3: https://www.postgresql.org/docs/current/sql-savepoint.html
• 4: https://github.com/postgres/postgres/blob/master/src/backend/access/transam/README
• 5: http://www.postgresql.org/docs/15/protocol-flow.html
• 6: https://www.postgresql.org/docs/current/protocol-message-formats.html

---

Fix misleading comments about transaction poison recovery.

The implementation correctly rejects ROLLBACK TO SAVEPOINT while poisoned (keeping the session poisoned), but multiple comments incorrectly list it as a recovery/clear case alongside ROLLBACK, COMMIT, and ABORT. PostgreSQL semantics confirm ROLLBACK TO SAVEPOINT leaves the outer transaction active and cannot be the sole recovery path on a dead backend. Update the comments at lines 396–401 and 484–489 to exclude ROLLBACK TO SAVEPOINT from recovery actions and clarify it belongs in the rejection category.

Comment fixes required

-	// While tx_poisoned, classify a 'Q' packet and either clear the poison
-	// and synthesize a ROLLBACK response (for ROLLBACK / COMMIT / ABORT /
-	// ROLLBACK TO SAVEPOINT) or reject with ERROR 25P02 (anything else).
+	// While tx_poisoned, classify a 'Q' packet and either clear the poison
+	// and synthesize a ROLLBACK response (for full-transaction ROLLBACK /
+	// COMMIT / ABORT) or reject with ERROR 25P02 (anything else, including
+	// ROLLBACK TO SAVEPOINT because it does not end the transaction).

-	//   ROLLBACK / ROLLBACK TO SAVEPOINT / ABORT -> synthesize
+	//   ROLLBACK / ABORT -> synthesize
 	//     CommandComplete('ROLLBACK') + ReadyForQuery('I'), clear flag.
 	//   COMMIT -> same ROLLBACK response + NoticeResponse carrying the
 	//     "there is no transaction in progress" warning, clear flag.
-	//   anything else (including RELEASE SAVEPOINT) -> reply ERROR 25P02
-	//     + ReadyForQuery('E'), stay poisoned.
+	//   anything else (including ROLLBACK TO SAVEPOINT and RELEASE SAVEPOINT)
+	//     -> reply ERROR 25P02 + ReadyForQuery('E'), stay poisoned.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@include/PgSQL_Session.h` around lines 396 - 401, Update the misleading
comment blocks that describe which simple-query packets clear transaction
poison: in the comment above the function handler_poisoned_simple_query and the
corresponding comment around the later block (previously listing ROLLBACK TO
SAVEPOINT as a recovery action), remove ROLLBACK TO SAVEPOINT from the list of
commands that "clear the poison" and instead include it among the rejected
statements that leave the session poisoned (i.e., keep it in the ERROR
25P02/reject category); reference the handler_poisoned_simple_query symbol to
locate the first comment and the matching poisoned-handling block for the second
comment and make the wording explicitly state that only ROLLBACK, COMMIT and
ABORT clear poison while ROLLBACK TO SAVEPOINT is rejected and retains the
poison.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and view the context around lines 495-575
fd -t f "PgSQL_Session.cpp" | head -5

Repository: sysown/proxysql

Length of output: 80

---

🏁 Script executed:

# Get the file size to understand what we're working with
wc -l lib/PgSQL_Session.cpp

Repository: sysown/proxysql

Length of output: 85

---

🏁 Script executed:

# View the specific lines in question to verify the code matches
sed -n '495,575p' lib/PgSQL_Session.cpp | head -100

Repository: sysown/proxysql

Length of output: 3645

---

🏁 Script executed:

# Search for where tx_poisoned is set/cleared and how poisoned sessions are handled
rg "tx_poisoned" lib/PgSQL_Session.cpp -B2 -A2

Repository: sysown/proxysql

Length of output: 6119

---

🏁 Script executed:

# Look for existing tests related to poisoned sessions and ROLLBACK
fd -t f "\.cpp$|\.h$" lib/ | xargs grep -l "poisoned" | head -10

Repository: sysown/proxysql

Length of output: 101

---

🏁 Script executed:

# Search for test files related to poisoned sessions or PgSQL
fd -t f "test" . -o "reg_test" . -o "*poisoned*" . 2>/dev/null | grep -E "\.(py|sql|cpp)$" | head -20

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Search for regression tests in the repo
find . -type f -name "*reg_test*" -o -name "*test*poisoned*" 2>/dev/null | head -20

Repository: sysown/proxysql

Length of output: 1301

---

🏁 Script executed:

# Look for test directories
fd -t d "test" -o "tests" -o "t" . | grep -v node_modules | head -10

Repository: sysown/proxysql

Length of output: 188

---

🏁 Script executed:

# Check the handler_poisoned_simple_query function signature and recovery logic
sed -n '470,600p' lib/PgSQL_Session.cpp | grep -A50 "handler_poisoned_simple_query"

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Find the handler_poisoned_simple_query function definition
rg "handler_poisoned_simple_query" lib/PgSQL_Session.cpp -B5 -A100 | head -150

Repository: sysown/proxysql

Length of output: 6421

---

🏁 Script executed:

# Check if there are any existing tests for poisoned sessions
rg "tx_poisoned\|poisoned" test/ -l | head -20

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Look for test files that might test ROLLBACK/COMMIT behavior
fd -type f -name "*pgsql*" test/tap/tests/*.cpp | grep -E "commit|rollback|poison" -i | head -10

Repository: sysown/proxysql

Length of output: 229

---

🏁 Script executed:

# Search more broadly for pgsql tests with ROLLBACK in the test code
rg "ROLLBACK" test/tap/tests/ --iglob "*.cpp" -l | head -10

Repository: sysown/proxysql

Length of output: 524

---

🏁 Script executed:

# Check what the full handler_poisoned_simple_query function returns and how it handles kind
sed -n '480,625p' lib/PgSQL_Session.cpp

Repository: sysown/proxysql

Length of output: 6230

---

🏁 Script executed:

# Search for any existing tests that test ROLLBACK with comments
rg "ROLLBACK.*\/\*\|\/\*.*ROLLBACK" test/ -i

Repository: sysown/proxysql

Length of output: 165

---

🏁 Script executed:

# Look for pgsql tests related to comments or poisoned state
rg "poisoned" test/tap/tests/*.cpp -A5 -B5

Repository: sysown/proxysql

Length of output: 27458

---

🏁 Script executed:

# Get the full reject_cases list from the poisoned recovery test
sed -n '170,210p' test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp

Repository: sysown/proxysql

Length of output: 1867

---

🏁 Script executed:

# Check if there are any comment-related test cases
rg "\/\*|comment" test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp -B2 -A2

Repository: sysown/proxysql

Length of output: 137

---

🏁 Script executed:

# Look at the actual reject_cases array to see all test cases
sed -n '200,240p' test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp

Repository: sysown/proxysql

Length of output: 1727

---

🏁 Script executed:

# Find the reject_cases array definition
rg "reject_cases\[\]" test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp -A20

Repository: sysown/proxysql

Length of output: 1149

---

🏁 Script executed:

# Look for any tests with malformed input or trailing comments
rg "ROLLBACK\|COMMIT" test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp | head -30

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Check the entire case_A_rejection function to see all test scenarios
sed -n '290,380p' test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp

Repository: sysown/proxysql

Length of output: 3268

---

🏁 Script executed:

# Check if there are any tests that explicitly test unterminated comments
rg "unterminated\|malformed" test/tap/tests/pgsql-tx_poisoned*.cpp -i

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Verify the current behavior by tracing through with "ROLLBACK /*"
# Let's also check the exact line numbers in the handler where recovery happens
sed -n '600,615p' lib/PgSQL_Session.cpp

Repository: sysown/proxysql

Length of output: 519

---

🏁 Script executed:

# Check what error code is used for rejection and understand the severity
sed -n '600,615p' lib/PgSQL_Session.cpp

Repository: sysown/proxysql

Length of output: 519

---

🏁 Script executed:

# Look at the write_tx_poisoned_error function to understand the rejection response
rg "write_tx_poisoned_error" lib/PgSQL_Session.cpp -A10 | head -30

Repository: sysown/proxysql

Length of output: 1114

---

🏁 Script executed:

# Trace through the exact behavior with a simpler test
# Check if rest_len calculation is correct for "ROLLBACK /*"
python3 << 'EOF'
# Input: "ROLLBACK /*" (11 chars)
sql_len = 11
verb_len = 8  # "ROLLBACK"
rest_len = sql_len - verb_len
print(f"After ROLLBACK match: rest_len = {rest_len}")

# rest points to " /*" (3 chars)
# Skip whitespace: rest_len goes from 3 to 2, rest points to "/*"
rest_len_after_ws = 2
print(f"After skipping whitespace: rest_len = {rest_len_after_ws}")

# Process /* comment:
# p += 2, n -= 2 (consumes "/*")
# Inner while: n >= 2 && !(p[0]=='*' && p[1]=='/')
# n becomes 0, else sets n=0
rest_len_final = 0
print(f"After processing unterminated comment: rest_len = {rest_len_final}")

# Recovery condition check at line 573
if rest_len_final > 0:
    print("Result: KIND_REJECT (would reject)")
else:
    print("Result: KIND_ROLLBACK (would recover and clear poison) - BUG!")
EOF

Repository: sysown/proxysql

Length of output: 247

---

Reject unterminated block comments in poisoned-session recovery classifier.

Statements like ROLLBACK /* (unterminated block comment) currently allow recovery to proceed: skip_ws_and_comments() silently sets n=0 when no closing */ is found, making the classifier believe nothing remains after the verb. This causes the check at line 573 (if (rest_len > 0)) to fail, leaving kind as KIND_ROLLBACK and triggering the recovery path that clears tx_poisoned. Keep the session poisoned instead by having skip_ws_and_comments() return bool to signal malformed input, and check all four call sites (lines 517, 554, 558, 563) to force KIND_REJECT on failure.

Suggested direction

-	auto skip_ws_and_comments = [](const char*& p, unsigned int& n) {
+	auto skip_ws_and_comments = [](const char*& p, unsigned int& n) -> bool {
 		for (;;) {
 			while (n > 0 && (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r')) { p++; n--; }
 			if (n >= 2 && p[0] == '-' && p[1] == '-') {
 				p += 2; n -= 2;
 				while (n > 0 && *p != '\n') { p++; n--; }
 				continue;
 			}
 			if (n >= 2 && p[0] == '/' && p[1] == '*') {
 				p += 2; n -= 2;
 				while (n >= 2 && !(p[0] == '*' && p[1] == '/')) { p++; n--; }
-				if (n >= 2) { p += 2; n -= 2; } else { n = 0; }
+				if (n >= 2) { p += 2; n -= 2; } else { return false; }
 				continue;
 			}
 			break;
 		}
+		return true;
 	};
 
-	skip_ws_and_comments(sql, sql_len);
+	if (!skip_ws_and_comments(sql, sql_len)) {
+		sql_len = 0;
+	}

Also check return values at lines 554, 558, and 563 (the rest calls) to force KIND_REJECT when trailing comments are malformed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/PgSQL_Session.cpp` around lines 495 - 575, skip_ws_and_comments currently
swallows unterminated /* ... */ and should return a bool to signal malformed
input; change its signature to return false on malformed/unterminated block
comments and true on success, then update all call sites (the initial call
skip_ws_and_comments(sql, sql_len) and the three subsequent calls on rest at the
matches_kw checks and semicolon handling) to check the return value and set kind
= KIND_REJECT if it returns false; ensure you propagate the same behavior for
the optional WORK/TRANSACTION consumption paths so any failed comment-skip
forces rejection and leaves tx_poisoned intact.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's check the file exists and examine the mentioned sections
wc -l lib/PgSQL_Session.cpp

Repository: sysown/proxysql

Length of output: 85

---

🏁 Script executed:

# Read the section around lines 2300-2328 to see the context
sed -n '2280,2340p' lib/PgSQL_Session.cpp | cat -n

Repository: sysown/proxysql

Length of output: 3225

---

🏁 Script executed:

# Now check the default branch mentioned at lines 2582-2594
sed -n '2570,2610p' lib/PgSQL_Session.cpp | cat -n

Repository: sysown/proxysql

Length of output: 1901

---

🏁 Script executed:

# Search for where command 'H' is explicitly handled, if anywhere
rg -n "command == 'H'" lib/PgSQL_Session.cpp

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Search for the broader switch/if structure that handles commands
rg -n -B2 -A2 "command == 'P'" lib/PgSQL_Session.cpp | head -50

Repository: sysown/proxysql

Length of output: 369

---

🏁 Script executed:

# Search for comments about Flush or 'H' handling
rg -n -B3 -A3 "c != 'H'" lib/PgSQL_Session.cpp

Repository: sysown/proxysql

Length of output: 552

---

🏁 Script executed:

# Search for Flush message documentation or comments
rg -n -i "flush" lib/PgSQL_Session.cpp | head -20

Repository: sysown/proxysql

Length of output: 160

---

🏁 Script executed:

# Check if there's any explicit H handling in the switch statement
sed -n '2330,2580p' lib/PgSQL_Session.cpp | grep -n "case 'H'" | head -5

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Let's look at the broader context around line 2587 to understand the H handling
sed -n '2560,2610p' lib/PgSQL_Session.cpp | cat -n

Repository: sysown/proxysql

Length of output: 2374

---

🏁 Script executed:

# Search for any other H/Flush related code or comments
rg -n "'H'" lib/PgSQL_Session.cpp | head -20

Repository: sysown/proxysql

Length of output: 144

---

🏁 Script executed:

# Check what happens in a normal (non-poisoned) extended query with H
sed -n '2300,2580p' lib/PgSQL_Session.cpp | grep -A10 -B10 "case 'H'" | head -40

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Let's check the PostgreSQL extended-query protocol docs context - search for any protocol documentation in comments
rg -n "Extended Query\|extended.query" lib/PgSQL_Session.cpp | head -20

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Let's see what happens around the poisoned gate more broadly - get more context before and after
sed -n '2290,2360p' lib/PgSQL_Session.cpp | cat -n

Repository: sysown/proxysql

Length of output: 3610

---

🏁 Script executed:

# Search for PostgreSQL protocol documentation or constants for message types
rg -n "Flush\|'H'" lib/PgSQL_Session.cpp -B2 -A2 | head -40

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Let's check if there are tests or examples showing the expected behavior
fd -t f "test.*pgsql" -o ".*pgsql.*test" | head -10

Repository: sysown/proxysql

Length of output: 204

---

🏁 Script executed:

# Check the poisoned gate comments more carefully and see what Extended Query messages should be
sed -n '2293,2305p' lib/PgSQL_Session.cpp

Repository: sysown/proxysql

Length of output: 857

---

🏁 Script executed:

# Let's look at the PostgreSQL protocol specification comment or any protocol-related doc
rg -n "Parse\|Bind\|Execute" lib/PgSQL_Session.cpp | head -5

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Check if there's any mention of protocol message types or their names
rg -n "'P'\|'B'\|'E'\|'S'" lib/PgSQL_Session.cpp | grep -i "parse\|bind\|execute\|sync\|flush" | head -10

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Search for HTTP/web references about PostgreSQL Flush message type
# Let's verify from the codebase if H is documented anywhere
rg -n "Message type" lib/PgSQL_Session.cpp | head -10

Repository: sysown/proxysql

Length of output: 210

---

🏁 Script executed:

# Let's check if there are any constants defined for message types
rg -n "0x48\|0x[Hh]" lib/PgSQL_Session.cpp | head -10

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Look for the full switch case statement to see all handled message types
sed -n '2330,2600p' lib/PgSQL_Session.cpp | grep "case '" | head -20

Repository: sysown/proxysql

Length of output: 234

---

🏁 Script executed:

# Let's verify the PostgreSQL protocol message types to confirm H is Flush
# Check if there's a header or protocol definition file
fd -t f "protocol\|pgsql" lib/ | grep -E "\.(h|hpp)$" | head -10

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Let's search for any documentation about message types in header files
rg -n "Flush" lib/ -t h -t hpp | head -10

Repository: sysown/proxysql

Length of output: 86

---

🏁 Script executed:

# Let's check what the PostgreSQL protocol actually expects for Flush
# by looking at the myprot.generate_error_packet implementation
rg -n "generate_error_packet" lib/ -t h -t hpp | head -5

Repository: sysown/proxysql

Length of output: 86

---

🏁 Script executed:

# Let's check if there's any include or reference to PostgreSQL protocol documentation
rg -n "H\|Flush" include/ 2>/dev/null | head -20

Repository: sysown/proxysql

Length of output: 41

---

🏁 Script executed:

# Let's look at what message types are being handled in the switch statement
sed -n '2330,2600p' lib/PgSQL_Session.cpp | cat -n | grep "case '"

Repository: sysown/proxysql

Length of output: 311

---

🏁 Script executed:

# Let's examine the context where H is specially handled (line 2587)
# to understand the intent
sed -n '2575,2600p' lib/PgSQL_Session.cpp | cat -n

Repository: sysown/proxysql

Length of output: 1231

---

🏁 Script executed:

# Let's verify the ASCII code for 'H' and understand the protocol better
python3 << 'EOF'
# Check ASCII value
print(f"H is ASCII: {ord('H')} or 0x{ord('H'):02X}")

# In PostgreSQL extended-query protocol:
# P = Parse
# B = Bind  
# D = Describe
# C = Close
# E = Execute
# S = Sync
# H = Flush (0x48)
# Q = Simple Query
# X = Terminate

protocol_messages = {
    'P': 'Parse',
    'B': 'Bind',
    'D': 'Describe',
    'C': 'Close',
    'E': 'Execute',
    'S': 'Sync',
    'H': 'Flush',
    'Q': 'Simple Query',
    'X': 'Terminate'
}

print("\nPostgreSQL Frontend message types:")
for code, name in sorted(protocol_messages.items()):
    print(f"  {code} = {name}")

print("\nCurrently handled in poisoned gate:")
print("  P, B, D, C, E (silent swallow)")
print("  S (error response)")
print("  Q, X (fall through)")
print("\nNOT in poisoned gate (falls through to default):")
print("  H (Flush)")
EOF

Repository: sysown/proxysql

Length of output: 414

---

Add Flush ('H') to the poisoned extended-query silent-swallow list.

Flush currently falls through to the default "Feature not supported" error path. In poisoned transactions, Flush (like Parse/Bind/Describe/Close/Execute) should be silently swallowed as part of the extended-query frame; the only client-visible response should come at Sync.

Suggested fix

-							if (command == 'P' || command == 'B' || command == 'D' ||
-							    command == 'C' || command == 'E') {
+							if (command == 'P' || command == 'B' || command == 'D' ||
+							    command == 'C' || command == 'E' || command == 'H') {
 								// Silent swallow — Parse/Bind/Describe/Close/Execute
 								// packets carry no client-visible response while
 								// poisoned. The whole logical ext-query gets
 								// accounted for once, at Sync.
 								l_free(pkt.size, pkt.ptr);
 								continue;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (tx_poisoned && command != 'Q' && command != 'X') {
	if (command == 'P' || command == 'B' || command == 'D' ||
	command == 'C' || command == 'E') {
	// Silent swallow — Parse/Bind/Describe/Close/Execute
	// packets carry no client-visible response while
	// poisoned. The whole logical ext-query gets
	// accounted for once, at Sync.
	l_free(pkt.size, pkt.ptr);
	continue;
	}
	if (command == 'S') {
	// One "rejected statement" per logical extended
	// query (== per Sync), matching the simple-query
	// path where a single 'Q' packet bumps the counter
	// exactly once.
	thread->status_variables.tx_poisoned_rejected_statements_total++;
	PG_pkt pgpkt{};
	pgpkt.set_multi_pkt_mode(true);
	write_tx_poisoned_error(pgpkt,
	"current transaction is aborted, commands ignored until end of transaction block (extended-query path; issue ROLLBACK via simple query to recover)");
	pgpkt.write_ReadyForQuery('E');
	pgpkt.set_multi_pkt_mode(false);
	auto buff = pgpkt.detach();
	client_myds->PSarrayOUT->add((void*)buff.first, buff.second);
	client_myds->DSS = STATE_SLEEP;
	l_free(pkt.size, pkt.ptr);
	if (mirror == false) RequestEnd(NULL, false);
	continue;
	}
	if (tx_poisoned && command != 'Q' && command != 'X') {
	if (command == 'P' || command == 'B' || command == 'D' ||
	command == 'C' || command == 'E' || command == 'H') {
	// Silent swallow — Parse/Bind/Describe/Close/Execute
	// packets carry no client-visible response while
	// poisoned. The whole logical ext-query gets
	// accounted for once, at Sync.
	l_free(pkt.size, pkt.ptr);
	continue;
	}
	if (command == 'S') {
	// One "rejected statement" per logical extended
	// query (== per Sync), matching the simple-query
	// path where a single 'Q' packet bumps the counter
	// exactly once.
	thread->status_variables.tx_poisoned_rejected_statements_total++;
	PG_pkt pgpkt{};
	pgpkt.set_multi_pkt_mode(true);
	write_tx_poisoned_error(pgpkt,
	"current transaction is aborted, commands ignored until end of transaction block (extended-query path; issue ROLLBACK via simple query to recover)");
	pgpkt.write_ReadyForQuery('E');
	pgpkt.set_multi_pkt_mode(false);
	auto buff = pgpkt.detach();
	client_myds->PSarrayOUT->add((void*)buff.first, buff.second);
	client_myds->DSS = STATE_SLEEP;
	l_free(pkt.size, pkt.ptr);
	if (mirror == false) RequestEnd(NULL, false);
	continue;
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/PgSQL_Session.cpp` around lines 2300 - 2328, The code treats Flush ('H')
as unsupported in a poisoned extended-query transaction; update the conditional
in the tx_poisoned check (the block that inspects command in PgSQL_Session.cpp
where tx_poisoned and command != 'Q' && command != 'X' are tested) to include
'H' alongside 'P','B','D','C','E' so that when tx_poisoned and command == 'H'
the packet is silently swallowed (l_free(pkt.size, pkt.ptr); continue;) just
like Parse/Bind/Describe/Close/Execute, leaving the single Sync-path response
unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Apply the backend-error cap to the warning log too.

The NoticeResponse is capped, but the log still embeds get_error_code_with_message() uncapped. A rogue backend can still amplify logs with a very large error string.

Suggested fix

 	const std::string backend_err_msg = sanitize_backend_err_msg(myconn->get_error_message());
+	const std::string backend_err_for_log = sanitize_backend_err_msg(myconn->get_error_code_with_message());
 
 	PG_pkt pgpkt{};
 	pgpkt.set_multi_pkt_mode(true);
@@
 	proxy_warning("Backend broken mid-transaction; poisoning client session on "
 		"(hg=%d,%s:%d,backend_pid=%d) for user=\"%s@%s\" db=\"%s\" digest=\"%s\" backend_err=\"%s\". "
 		"Client must issue ROLLBACK to recover.\n",
 		myconn->parent->myhgc->hid, myconn->parent->address, myconn->parent->port,
 		myconn->get_backend_pid(), fe_user, client_addr, fe_db, digest,
-		myconn->get_error_code_with_message().c_str());
+		backend_err_for_log.c_str());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/PgSQL_Session.cpp` around lines 2747 - 2790, The log embeds the uncapped
backend string via myconn->get_error_code_with_message(), so replace that with
the already-sanitized/capped backend_err_msg (produced by
sanitize_backend_err_msg) when calling proxy_warning in this block (i.e., use
backend_err_msg.c_str() or build a small capped string combining
code+backend_err_msg) so the warning log cannot be amplified by an oversized
backend message; update the proxy_warning call site accordingly (references:
backend_err_msg, proxy_warning, get_error_code_with_message(),
sanitize_backend_err_msg).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Show Galera target prerequisites and recipes.
# Expected: both target prerequisite lines list ../../../src/SQLite3_Server.cpp and ../../../lib/ProxySQL_Admin.cpp.

awk '
  /^galera_1_timeout_count:/ { print; getline; print; next }
  /^galera_2_timeout_no_count:/ { print; getline; print; next }
' test/tap/tests/Makefile

Repository: sysown/proxysql

Length of output: 1066

---

Add the linked source files to the target prerequisites.

Lines 192 and 195 compile ../../../src/SQLite3_Server.cpp and ../../../lib/ProxySQL_Admin.cpp but these files are not listed as prerequisites. Incremental builds can keep stale binaries after changes to these sources.

Proposed Makefile dependency fix

-galera_1_timeout_count: galera_1_timeout_count.cpp $(TAP_LDIR)/libtap.so
+galera_1_timeout_count: galera_1_timeout_count.cpp ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp $(TAP_LDIR)/libtap.so
 	$(CXX) -DTEST_GALERA $< ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp -I$(CLICKHOUSE_CPP_IDIR) -I$(COREDUMPER_IDIR) $(IDIRS) $(LDIRS) -L$(CLICKHOUSE_CPP_LDIR) -L$(LZ4_LDIR) $(OPT) $(OBJ) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) $(CLICKHOUSE_CPP_LDIR)/libclickhouse-cpp-lib.a $(CLICKHOUSE_CPP_PATH)/contrib/zstd/zstd/libzstdstatic.a $(LZ4_LDIR)/liblz4.a -lscram -lusual -Wl,--allow-multiple-definition -o $@
 
-galera_2_timeout_no_count: galera_2_timeout_no_count.cpp $(TAP_LDIR)/libtap.so
+galera_2_timeout_no_count: galera_2_timeout_no_count.cpp ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp $(TAP_LDIR)/libtap.so
 	$(CXX) -DTEST_GALERA $< ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp -I$(CLICKHOUSE_CPP_IDIR) -I$(COREDUMPER_IDIR) $(IDIRS) $(LDIRS) -L$(CLICKHOUSE_CPP_LDIR) -L$(LZ4_LDIR) $(OPT) $(OBJ) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) $(CLICKHOUSE_CPP_LDIR)/libclickhouse-cpp-lib.a $(CLICKHOUSE_CPP_PATH)/contrib/zstd/zstd/libzstdstatic.a $(LZ4_LDIR)/liblz4.a -lscram -lusual -Wl,--allow-multiple-definition -o $@

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	galera_1_timeout_count: galera_1_timeout_count.cpp $(TAP_LDIR)/libtap.so
	$(CXX) -DTEST_GALERA $< ../../../src/SQLite3_Server.cpp $(IDIRS) $(LDIRS) $(OPT) $(OBJ) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) -lscram -lusual -o $@
	$(CXX) -DTEST_GALERA $< ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp -I$(CLICKHOUSE_CPP_IDIR) -I$(COREDUMPER_IDIR) $(IDIRS) $(LDIRS) -L$(CLICKHOUSE_CPP_LDIR) -L$(LZ4_LDIR) $(OPT) $(OBJ) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) $(CLICKHOUSE_CPP_LDIR)/libclickhouse-cpp-lib.a $(CLICKHOUSE_CPP_PATH)/contrib/zstd/zstd/libzstdstatic.a $(LZ4_LDIR)/liblz4.a -lscram -lusual -Wl,--allow-multiple-definition -o $@
	galera_2_timeout_no_count: galera_2_timeout_no_count.cpp $(TAP_LDIR)/libtap.so
	$(CXX) -DTEST_GALERA $< ../../../src/SQLite3_Server.cpp $(IDIRS) $(LDIRS) $(OPT) $(OBJ) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) -lscram -lusual -o $@
	$(CXX) -DTEST_GALERA $< ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp -I$(CLICKHOUSE_CPP_IDIR) -I$(COREDUMPER_IDIR) $(IDIRS) $(LDIRS) -L$(CLICKHOUSE_CPP_LDIR) -L$(LZ4_LDIR) $(OPT) $(OBJ) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) $(CLICKHOUSE_CPP_LDIR)/libclickhouse-cpp-lib.a $(CLICKHOUSE_CPP_PATH)/contrib/zstd/zstd/libzstdstatic.a $(LZ4_LDIR)/liblz4.a -lscram -lusual -Wl,--allow-multiple-definition -o $@
	galera_1_timeout_count: galera_1_timeout_count.cpp ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp $(TAP_LDIR)/libtap.so
	$(CXX) -DTEST_GALERA $< ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp -I$(CLICKHOUSE_CPP_IDIR) -I$(COREDUMPER_IDIR) $(IDIRS) $(LDIRS) -L$(CLICKHOUSE_CPP_LDIR) -L$(LZ4_LDIR) $(OPT) $(OBJ) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) $(CLICKHOUSE_CPP_LDIR)/libclickhouse-cpp-lib.a $(CLICKHOUSE_CPP_PATH)/contrib/zstd/zstd/libzstdstatic.a $(LZ4_LDIR)/liblz4.a -lscram -lusual -Wl,--allow-multiple-definition -o $@
	galera_2_timeout_no_count: galera_2_timeout_no_count.cpp ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp $(TAP_LDIR)/libtap.so
	$(CXX) -DTEST_GALERA $< ../../../src/SQLite3_Server.cpp ../../../lib/ProxySQL_Admin.cpp -I$(CLICKHOUSE_CPP_IDIR) -I$(COREDUMPER_IDIR) $(IDIRS) $(LDIRS) -L$(CLICKHOUSE_CPP_LDIR) -L$(LZ4_LDIR) $(OPT) $(OBJ) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) $(CLICKHOUSE_CPP_LDIR)/libclickhouse-cpp-lib.a $(CLICKHOUSE_CPP_PATH)/contrib/zstd/zstd/libzstdstatic.a $(LZ4_LDIR)/liblz4.a -lscram -lusual -Wl,--allow-multiple-definition -o $@

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/Makefile` around lines 191 - 195, The Makefile targets
galera_1_timeout_count and galera_2_timeout_no_count compile
../../../src/SQLite3_Server.cpp and ../../../lib/ProxySQL_Admin.cpp but do not
list them as prerequisites; update the rules for targets galera_1_timeout_count
and galera_2_timeout_no_count to include ../../../src/SQLite3_Server.cpp and
../../../lib/ProxySQL_Admin.cpp in their prerequisite lists so changes to those
sources trigger a rebuild of the corresponding binaries.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Restore the previous admin value and reject 0-row updates.

set_admin_bool() currently treats a 0-row UPDATE as success, and the guard always restores true instead of the value that was present before the test. That can leak global config into later tests and hide admin-variable drift.

Suggested direction

 	PGresult* r = PQexec(admin, q);
-	bool ok1 = (PQresultStatus(r) == PGRES_COMMAND_OK);
+	bool ok1 = (PQresultStatus(r) == PGRES_COMMAND_OK
+		&& strcmp(PQcmdTuples(r), "1") == 0);

Then have RestorePreserveClientAdminVar capture the current pgsql-preserve_client_on_broken_backend_in_tx value before Line 234 and restore that captured value in the destructor.

Also applies to: 98-101, 232-235

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp` around lines 68 - 76,
set_admin_bool currently treats a 0-row UPDATE as success and
RestorePreserveClientAdminVar's destructor always restores true instead of the
original value, risking leaked global config; change set_admin_bool (the
function that runs the snprintf/PQexec sequence) to check the PGresult for
affected rows (use PQcmdTuples or equivalent) and return false when no rows were
updated, and modify RestorePreserveClientAdminVar to read and store the current
value of "pgsql-preserve_client_on_broken_backend_in_tx" before performing the
update and then restore that captured value in its destructor rather than
hardcoding true.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Find existing TAP skip usage in this test suite.
rg -n -C3 '\bskip\s*\(|# SKIP|SKIPPED:' test/tap/tests

Repository: sysown/proxysql

Length of output: 50372

---

Replace ok(0, "SKIPPED: ...") loop with proper TAP skip directive.

The pattern ok(0, "SKIPPED: ...") still records failures even with the "SKIPPED" string. When kill-delivery fails due to infrastructure timing issues, use the standard skip() function instead:

diag("poison: killer failed to deliver pg_terminate_backend within 6s");
skip(9, "poison did not fire");

This keeps infrastructure flakiness from appearing as test failures.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp` around lines 252 -
257, Replace the failing ok(0, "SKIPPED: ...") loop with a proper TAP skip: when
kr.kill_delivered is false, emit a diagnostic with diag("poison: killer failed
to deliver pg_terminate_backend within 6s") and call skip(9, "poison did not
fire") to mark the remaining 9 tests as skipped; still ensure you
PQclear(kr.pgresult), PQfinish(cli) and return exit_status() in that branch
(references: kr.kill_delivered, kr.pgresult, PQclear, PQfinish, exit_status).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Detect GNU omitted-middle conditional operators in this file.
python3 - <<'PY'
from pathlib import Path
p = Path("test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp")
for i, line in enumerate(p.read_text().splitlines(), 1):
    if "?:" in line or " ?: " in line:
        print(f"{p}:{i}: {line}")
PY

Repository: sysown/proxysql

Length of output: 311

---

🏁 Script executed:

# Read the exact lines from the file to verify the snippet
sed -n '259,265p' test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp

Repository: sysown/proxysql

Length of output: 412

---

🏁 Script executed:

# Search for the rejected_25P02_as_error function definition to check if it's null-safe
rg -n "rejected_25P02_as_error" test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp -A 3

Repository: sysown/proxysql

Length of output: 942

---

🏁 Script executed:

# Check if there are any other GNU ?:0 operators in the file beyond the known ones
grep -n "?:" test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp

Repository: sysown/proxysql

Length of output: 201

---

Make the poison failure diagnostic null-safe and C++17-standard.

The predicate rejected_25P02_as_error() handles nullptr correctly, but the diagnostic arguments still dereference kr.pgresult unconditionally. Additionally, the ?: operator on lines 262–263 is a GNU extension, not standard C++17.

Suggested fix

+	const char* poison_status = kr.pgresult ? PQresStatus(PQresultStatus(kr.pgresult)) : "(null)";
+	const char* poison_severity = kr.pgresult ? PQresultErrorField(kr.pgresult, PG_DIAG_SEVERITY) : nullptr;
+	const char* poison_sqlstate = kr.pgresult ? PQresultErrorField(kr.pgresult, PG_DIAG_SQLSTATE) : nullptr;
 	ok(rejected_25P02_as_error(kr.pgresult),
 	   "pg_sleep after mid-tx backend kill returns 25P02 severity=ERROR (status=%s severity=%s sqlstate=%s)",
-	   PQresStatus(PQresultStatus(kr.pgresult)),
-	   PQresultErrorField(kr.pgresult, PG_DIAG_SEVERITY) ?: "(none)",
-	   PQresultErrorField(kr.pgresult, PG_DIAG_SQLSTATE) ?: "(none)");
+	   poison_status,
+	   poison_severity ? poison_severity : "(none)",
+	   poison_sqlstate ? poison_sqlstate : "(none)");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-tx_poisoned_extended_query-t.cpp` around lines 259 -
263, The diagnostic arguments dereference kr.pgresult unconditionally and use
the GNU ?: extension; change the three inline expressions to be null-safe and
C++17-compliant by guarding PQresultStatus/PQresultErrorField calls with a
kr.pgresult check and replacing ?: with the standard ternary operator. For
example, when building the status/severity/sqlstate strings for the ok(...) call
in the test where rejected_25P02_as_error(kr.pgresult) is used, compute
temporary const char* (or std::string) values using checks like kr.pgresult ?
PQresStatus(PQresultStatus(kr.pgresult)) : "(none)" and kr.pgresult &&
PQresultErrorField(kr.pgresult, PG_DIAG_SEVERITY) ?
PQresultErrorField(kr.pgresult, PG_DIAG_SEVERITY) : "(none)" (and similarly for
PG_DIAG_SQLSTATE), then pass those temporaries into the diagnostic format
instead of dereferencing kr.pgresult inline.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Remove abandoned NoticeCollector struct.

This struct is never instantiated or referenced. Its callback is an empty stub (it allocates a PGresult then (void)dup;s it without doing anything), and its trailing comment describes an approach that was ultimately replaced by NoticeBag/notice_receiver_cb below. Leaving it in place is confusing and will likely trip up future readers or static analysis.

♻️ Proposed cleanup

-// Collect NoticeResponse payloads on a PGconn so we can later inspect them.
-struct NoticeCollector {
-	std::vector<PGresult*> notices;
-	static void callback(void* arg, const PGresult* res) {
-		NoticeCollector* self = static_cast<NoticeCollector*>(arg);
-		// Dup the PGresult so we can outlive the libpq-owned original.
-		PGresult* dup = PQmakeEmptyPGresult(nullptr, PGRES_NONFATAL_ERROR);
-		(void)res;
-		if (!dup) return;
-		// We can't deep-copy arbitrary PGresult state via public libpq, so
-		// instead stash the severity + sqlstate + message as synthesized fields.
-		// Callers of this helper that need finer-grained inspection would read
-		// the original PGresult; for our assertions the three fields suffice.
-		// Store the three fields by overloading the PGresult's error message.
-		// libpq provides no public setter for PG_DIAG_SEVERITY etc., so we
-		// keep a side-car copy instead.
-		(void)dup;
-	}
-	// Simpler approach: use PQnoticeProcessor (the text-message processor)
-	// plus a parallel SQLSTATE/severity capture via PQsetNoticeReceiver on
-	// a fresh PGresult stream — but PGresult fields aren't settable. We work
-	// around that by capturing the raw text the backend was going to print.
-};
-
-// Simplified, working notice collector: we capture the *text* libpq would
-// normally print (via PQsetNoticeProcessor) plus a parallel stash of sqlstate
-// strings pulled from each notice (via PQsetNoticeReceiver). Both callbacks
-// fire for the same notice, so we record once per callback invocation.
+// Notice collector: captures severity / sqlstate / primary message for every
+// NoticeResponse fired on the connection. Installed via PQsetNoticeReceiver.
 struct CapturedNotice {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/pgsql-tx_poisoned_recovery-t.cpp` around lines 176 - 198,
Remove the unused NoticeCollector struct and its callback stub (symbols:
NoticeCollector, NoticeCollector::callback) from the test file to avoid
confusion; ensure there are no references to it elsewhere (tests use NoticeBag
and notice_receiver_cb), delete the whole struct and its trailing explanatory
comment block so the file only contains the active notice-handling
implementation (NoticeBag/notice_receiver_cb).

[sonarqubecloud]

Quality Gate failed

Failed conditions
2 Security Hotspots

See analysis details on SonarQube Cloud