feat: report timeout terminations with SQLSTATE - #6016
Conversation
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change records PostgreSQL session kill reasons, maps timeout reasons to SQLSTATE errors, sends termination responses to clients, and adds TAP coverage for session and transaction-idle timeouts. ChangesPostgreSQL timeout error flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PgSQL_Thread
participant PgSQL_Session
participant PostgreSQLClient
PgSQL_Thread->>PgSQL_Session: Record timeout kill reason
PgSQL_Session->>PgSQL_Session: Map reason to SQLSTATE and message
PgSQL_Session->>PostgreSQLClient: Send ErrorResponse and flush output
PgSQL_Session->>PostgreSQLClient: Terminate processing
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/pgsql-wait_timeout-t.cpp`:
- Around line 140-143: Update the test cleanup around the existing timeout setup
and early returns to save the original values of every modified runtime
variable, including pgsql-poll_timeout, and restore them on every exit path.
Ensure cleanup and LOAD PGSQL VARIABLES TO RUNTIME failures cause the test to
fail, including when execution reaches the returns near the existing test flow
and the final cleanup block.
🪄 Autofix
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 Plus
Run ID: ba3cd9d6-7dd5-47a5-a948-97ff1e1f2461
📒 Files selected for processing (6)
include/Base_Session.hlib/MySQL_Session.cpplib/PgSQL_Session.cpplib/PgSQL_Thread.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-wait_timeout-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
include/**/*.h
📄 CodeRabbit inference engine (CLAUDE.md)
Header include guards use the
#ifndef __CLASS_*_Hconvention.
Files:
include/Base_Session.h
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
include/Base_Session.hlib/PgSQL_Thread.cpplib/MySQL_Session.cpplib/PgSQL_Session.cpptest/tap/tests/pgsql-wait_timeout-t.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/pgsql-wait_timeout-t.cpp
🧠 Learnings (1)
📚 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-wait_timeout-t.cpp
🔇 Additional comments (5)
include/Base_Session.h (1)
29-34: LGTM!Also applies to: 93-93
lib/MySQL_Session.cpp (1)
652-652: LGTM!lib/PgSQL_Session.cpp (1)
305-305: LGTM!Also applies to: 3129-3151, 3834-3843
lib/PgSQL_Thread.cpp (1)
3414-3416: LGTM!Also applies to: 3751-3752, 3761-3762, 3880-3880
test/tap/groups/groups.json (1)
164-164: LGTM!
| // restore defaults so the short timeouts don't leak into later tests in the group | ||
| admin_q(admin, "SET pgsql-wait_timeout=28800000"); | ||
| admin_q(admin, "SET pgsql-max_transaction_idle_time=14400000"); | ||
| admin_q(admin, "LOAD PGSQL VARIABLES TO RUNTIME"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore every modified runtime variable on every exit path.
Lines 67 and 95 set pgsql-poll_timeout=500, but this cleanup does not restore it. Lines 131 and 136 also return before this block runs. This can change timeout behavior for later TAP tests.
Save the prior values, run cleanup before every return, and fail the test if cleanup or LOAD PGSQL VARIABLES TO RUNTIME fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/tap/tests/pgsql-wait_timeout-t.cpp` around lines 140 - 143, Update the
test cleanup around the existing timeout setup and early returns to save the
original values of every modified runtime variable, including
pgsql-poll_timeout, and restore them on every exit path. Ensure cleanup and LOAD
PGSQL VARIABLES TO RUNTIME failures cause the test to fail, including when
execution reaches the returns near the existing test flow and the final cleanup
block.
|
@renecannao I'm not sure how do we map this to MySQL/MariaDB. These error codes map cleanly to Postgres. Open to suggestions. Should we send our own ProxySQL error here? But then, that would cause assymetry with PG. |
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| int rc = run_q(proxy, "SELECT 1"); | ||
| ok(rc == 0, (rc == 0 ? "Connection alive" : "Connection killed")); | ||
|
|
||
| sleep(9); |
There was a problem hiding this comment.
💡 Quality: Timeout test relies on fixed sleeps and wall-clock timing
pgsql-wait_timeout-t.cpp drives both cases with real timeouts plus a hard-coded sleep(9) (~18s total), and depends on the maintenance loop firing within that window. On loaded CI this wall-clock dependence makes the test slow and potentially flaky. Consider polling for termination in a loop with a bounded deadline instead of a single fixed sleep, so the test succeeds as soon as the connection is killed and fails fast otherwise.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsAdds SQLSTATE reporting and error messages for PostgreSQL idle timeouts to match native behavior. Consider replacing fixed sleeps in the timeout test with synchronization to avoid flakiness. 💡 Quality: Timeout test relies on fixed sleeps and wall-clock timing📄 test/tap/tests/pgsql-wait_timeout-t.cpp:86 📄 test/tap/tests/pgsql-wait_timeout-t.cpp:121 pgsql-wait_timeout-t.cpp drives both cases with real timeouts plus a hard-coded sleep(9) (~18s total), and depends on the maintenance loop firing within that window. On loaded CI this wall-clock dependence makes the test slow and potentially flaky. Consider polling for termination in a loop with a bounded deadline instead of a single fixed sleep, so the test succeeds as soon as the connection is killed and fails fast otherwise. 🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Thank you @Snehil-Shah for working on this. Reporting the PostgreSQL timeout SQLSTATE and message to clients is a valuable compatibility improvement. I found two items that should be addressed before merging:
Thanks again for the contribution. With those paths covered, this will be a solid improvement. |
2 participants
Currenlty, timeouts directly kill connections without sending any fitting ErrorResponse. This brings it into parity with Postgres's true behavior.
I have added a new
kill_reasonto session state, making the design extendable.Summary by CodeRabbit
New Features
Bug Fixes
Tests