PgSQL native backend protocol: replace libpq on the data path (connect/auth/TLS, simple query, COPY, extended query via stmt pipeline, Describe cache) - #5882
Conversation
…ing + unit tests Add PgSQL_Backend_Msg_Framer: a pure wire-message framer for the native PostgreSQL backend protocol. It accepts fed bytes (possibly partial) and yields complete messages (type byte + 4-byte big-endian length-prefixed body), signaling FRAME_NEED_MORE on incomplete trailing bytes and FRAME_ERROR on a malformed length. Header stays light (cstdint/cstddef only). Destructor frees the realloc'd buffer to avoid a leak on long-lived connections. Wire it into libproxysql.a via _OBJ_CXX in lib/Makefile. Also fix a pre-existing duplicate vec.o on the unit-test link line: the test/tap/tests/unit/Makefile appended SQLITE3_LDIR/vec.o to STATIC_LIBS in two separate PROXYSQL40 blocks, producing ~98 duplicate-symbol errors that broke every unit test under PROXYSQL40. Keep the single append after the autodetection block.
…hecks, noncopyable)
…proof-correctness test
…nc_connect assert) + timeout teardown [Task 1.6a]
… verification) [Task 1.6b]
…nding Docker) [Task 1.8]
…hrough results) [Task 1.6c / Phase 2 core]
The native simple-query path appended a NUL to query.length bytes, but the client-query callers (async_query with pgsql_real_query.QuerySize) pass a length that already includes the trailing NUL, producing a malformed double-NUL Query body. PostgreSQL rejects it with 08P01 'invalid message format', breaking the backend connection. Normalize to the SQL up to the first NUL (bounded by query.length) plus a single terminator, matching PQsendQuery semantics. The strlen()-based callers (async_send_simple_command/init_connect) are unaffected.
…nnection is_connection_in_reusable_state() called PQtransactionStatus(pgsql_conn) directly; in native mode pgsql_conn is NULL so libpq returns PQTRANS_UNKNOWN, making the session treat a normal backend query error (ErrorResponse + ReadyForQuery, the connection is still idle/reusable) as a broken connection and retry instead of forwarding the error (with its SQLSTATE) to the client. Derive the transaction status from the natively-tracked ReadyForQuery byte in native mode.
…nd-point,, when cbind is set
… both offered and TLS
…(+SCRAM in pg_lite_client) New TAP test pgsql-native_portals-t drives named-portal Bind/Execute/Describe/ Close/Sync byte streams with the hand-rolled pg_lite_client and compares the client-visible backend message sequence between a direct PostgreSQL backend (SCRAM-SHA-256 oracle) and ProxySQL in native-backend-protocol mode. Covers the 9-case corpus (PORTAL_BASIC/MULTI/SUSPEND/TXN/SYNC_DESTROY/CLOSE_IDEMPOTENT/ ERR_BIND_DUP/LIBPQ_MODE_REJECTS/UNNAMED_UNCHANGED) plus a multiplexing pin-release check. 12/12 ok. - pg_lite_client: add SCRAM-SHA-256 (AuthenticationSASL 10/11/12) via the in-tree pg_scram_* wrappers, guarded by -DPG_LITE_CLIENT_SCRAM so the shared source stays link-clean for tests that do not enable it. - Makefile: pgsql-native_portals-t rule adds pg_lite_client.cpp + -lscram -lusual (galera-rule precedent) and -DPG_LITE_CLIENT_SCRAM. - groups.json: register under legacy-g1 (same groups as the other native tests). Production hardening (P2 review follow-up): the ALREADY_BOUND (named Execute / resume) drive branch in PgSQL_Connection::stmt_execute_start was gated on native_mode but, on a libpq-mode backend connection reached with a warm-pool flag flip, would fall through to the libpq path and silently re-Bind the unnamed portal. Add the symmetric FEATURE_NOT_SUPPORTED reject already present for native_bind_only / native_close_only. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…transaction — stale sticky pin held backend conn out of the pool Both clear_named_portals() epilogue sites were gated on processing_extended_query, so a simple-query COMMIT/ROLLBACK (which destroys every portal server-side at txn end) left stale registry entries; the sticky pin computed from !named_portals.empty() then kept the backend connection attached to the session indefinitely — multiplexing never resumed. Found by pgsql-native_portals-t's reworked NON-vacuous pin-release check (client stays connected while polling stats_pgsql_connection_pool: pre-fix ConnUsed stayed 1 -> 1 after COMMIT; post-fix 1 -> 0). Drop the extended-query gate on the rc0-boundary clear. libpq-safe: named portals can only be registered in native mode (named Bind is rejected in libpq mode) and clear_named_portals() is a no-op when the registry is empty, so a libpq connection's unmaintained native_txn_status is never acted upon. The rc==-1 error-epilogue site keeps its gate: a non-empty registry at a boundary implies an open explicit transaction, where a failed simple query leaves txn-state 'E' (not 'I') and portals are correctly retained. Regression: pgsql-native_portals-t 12/12, pgsql-native_prepared-t 27/27, pgsql-native_transactions-t 16/16. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
… auth restore, narrowed error normalization, log-verified coverage Review fixes on top of 1becd76: - FIX 1: the pin-release check now keeps the raw client CONNECTED and idle while polling stats_pgsql_connection_pool: asserts ConnUsed=1 during the explicit txn (portal bound) and ConnUsed->0 within a bounded 5s window after COMMIT, BEFORE disconnecting — session teardown can no longer fake the release. This immediately exposed a real P1 bug (fixed in the previous commit): pre-fix the assertion failed 1 -> 1. - FIX 2: AuthMethodScope gains an idempotent restore(); the two BAIL_OUT paths after construction call it explicitly (BAIL_OUT is exit(255) and skips destructors), and construction no longer changes the variable when the current value cannot be read (nothing to restore). - FIX 3: the E/N -> SQLSTATE reduction is narrowed to the two frames whose errors ProxySQL synthesizes locally (PORTAL_TXN / PORTAL_SYNC_DESTROY post-invalidation Execute). PORTAL_ERR_BIND_DUP's backend-generated 42P03 is now compared with the FULL error payload and matches byte-for-byte (severity/message/file/line/routine forwarded unchanged). - FIX 4: native_path_used is no longer hardcoded — each case's proxy leg is log-verified (drain + "falling back to libpq" tripwire, the pgsql-native_prepared-t pattern). pgsql-native_portals-t 12/12 ok, RC 0. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…rtal_name in session reset (phase-review M2) Final whole-phase review verdict: READY — no Critical/Important cross-cutting findings; this lands the one recommended cheap fix. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
|
Named portals landed (commits
Known follow-ups (tracked in the plan/spec docs): cross-hostgroup portal-registry clear edge under |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/PgSQL_Session.cpp (1)
7009-7035: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore client/session state on Describe-cache hits.
This synthesized response path returns without setting
client_myds->DSSback toSTATE_SLEEPorstatusback toWAITING_CLIENT_DATA, unlike the local Bind/Close completion paths. A cache hit with no backend dispatch can leave the session in the extended-sync processing state after the response has already been generated.Proposed fix
client_myds->myprot.generate_describe_from_cache(true, send_ready_packet, txn_state, dc); RequestEnd(NULL, false); + client_myds->DSS = STATE_SLEEP; + status = WAITING_CLIENT_DATA; return 0;🤖 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 `@lib/PgSQL_Session.cpp` around lines 7009 - 7035, The statement-level Describe cache-hit path in PgSQL_Session::generate_describe_from_cache_response (the stmt_type == 'S' block) returns after synthesizing the response but does not restore the client/session state. Update this cache-hit branch to mirror the normal completion paths by setting client_myds->DSS back to STATE_SLEEP and client_myds->status back to WAITING_CLIENT_DATA before returning, so the session is no longer left in extended-sync processing after RequestEnd.
🤖 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 `@docs/superpowers/plans/2026-07-07-pgsql-native-named-portals-plan.md`:
- Around line 55-56: The named portal registry update in
handle_post_sync_bind_message should not replace named_portals[name] before Bind
succeeds, because a failed Bind would overwrite the existing portal state. Keep
the released bind_msg and stmt_info staged separately while preparing
CurrentQuery.extended_query_info and find_or_create_backend, then publish the
new registry entry only in the BindComplete success path; if the portal name
already exists, preserve PostgreSQL’s existing duplicate-portal error behavior
instead of mutating the map early.
In `@lib/PgSQL_Logger.cpp`:
- Around line 1016-1018: The query-less Close handling in PgSQL_Logger still
flows through the SIMPLE_QUERY digest path even when c_stmt_close_no_query is
set, so update the logic around c_stmt_close_no_query, let, and the digest
computation in the query branch to skip digest generation for
PROCESSING_STMT_CLOSE events with no query text. Use the existing checks in
PgSQL_Logger::... (the block that logs query text and computes the digest from
CurrentQuery.QueryParserArgs) to route these events away from the SIMPLE_QUERY
case and ensure the Close message logs without a stale digest or query parsing
work.
In `@lib/PgSQL_Session.cpp`:
- Around line 3373-3377: The pipeline variable sync logic in
PgSQL_Session::ProcessStartupPacket now treats PROCESSING_STMT_BIND and
PROCESSING_STMT_CLOSE as extended-query states, but the restore guard in the
dynamic-variable branch still only allows PROCESSING_STMT_EXECUTE. Update the
restore condition in the same pipeline-sync path so it applies consistently to
the new BIND/CLOSE states as well, using the existing
processing_extended_query/PROCESSING_STMT_* checks around the variable sync and
restore handling.
- Around line 3630-3642: The portal registry is being cleared too early in
PROCESSING_STMT_CLOSE, which can leave CurrentQuery.extended_query_info holding
dangling pointers during query finalization. In the CLOSE handling path, compute
the post-query sticky decision first, call RequestEnd() while
named_portals/closing_portal_name and pending_named_bind are still intact, and
only then erase the portal entry and clear the related state before
finishQuery(). Apply the same ordering fix in the other CLOSE-related block
around the second location noted in the comment, using RequestEnd(),
finishQuery(), named_portals, closing_portal_name, and pending_named_bind as the
key symbols to update.
- Around line 3849-3862: The named-portal cleanup in PgSQL_Session’s ERROR
epilogue only runs when the reusable connection is still present and
native_txn_status is I, so the portal registry can survive a backend loss and
later point at stale state. Update the clear_named_portals() path in the
extended-query error handling to also clear the registry when the owning backend
has been torn down or reconnected, using the same PgSQL_Session/myconn checks
that distinguish reusable vs. lost connections. Keep the existing
transaction-state behavior, but ensure portal bookkeeping is reset whenever the
backend that owned those portals is no longer valid.
In `@test/tap/tests/pgsql-native_portals-t.cpp`:
- Around line 484-487: The bailout path in the native-mode setup leaves
global/runtime state mutated if `setNativeMode()` succeeds but
`flushBackendPool()` fails; update the early-exit handling in the relevant test
setup block to do a best-effort rollback before `BAIL_OUT`. Use the existing
`auth_scope.restore()` pattern and also explicitly revert the native mode /
backend pool state in the same failure branch, so the later final restore block
in the test remains a backup rather than the only cleanup.
---
Outside diff comments:
In `@lib/PgSQL_Session.cpp`:
- Around line 7009-7035: The statement-level Describe cache-hit path in
PgSQL_Session::generate_describe_from_cache_response (the stmt_type == 'S'
block) returns after synthesizing the response but does not restore the
client/session state. Update this cache-hit branch to mirror the normal
completion paths by setting client_myds->DSS back to STATE_SLEEP and
client_myds->status back to WAITING_CLIENT_DATA before returning, so the session
is no longer left in extended-sync processing after RequestEnd.
🪄 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: c20404df-71e9-4ab5-9ae8-63dbabb57ea0
📒 Files selected for processing (16)
docs/superpowers/plans/2026-07-07-pgsql-native-named-portals-plan.mddocs/superpowers/specs/2026-07-07-pgsql-native-extq-stmt-pipeline-design.mdinclude/PgSQL_Connection.hinclude/PgSQL_PreparedStatement.hinclude/PgSQL_Session.hinclude/proxysql_structs.hlib/PgSQL_Connection.cpplib/PgSQL_Logger.cpplib/PgSQL_PreparedStatement.cpplib/PgSQL_Protocol.cpplib/PgSQL_Session.cpplib/PgSQL_Variables.cpptest/tap/groups/groups.jsontest/tap/tests/Makefiletest/tap/tests/pg_lite_client.cpptest/tap/tests/pgsql-native_portals-t.cpp
✅ Files skipped from review due to trivial changes (1)
- docs/superpowers/specs/2026-07-07-pgsql-native-extq-stmt-pipeline-design.md
🚧 Files skipped from review as they are similar to previous changes (3)
- test/tap/groups/groups.json
- lib/PgSQL_Protocol.cpp
- include/PgSQL_Connection.h
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: CI-lint-groups-json / lint: docs(pgsql): mark named portals implemented in spec; clear closing_po…
Conclusion: failure
##[group]Run python3 test/tap/groups/lint_groups_json.py
�[36;1mpython3 test/tap/groups/lint_groups_json.py�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
groups.json format lint: 1 error(s) found:
Keys not sorted: 'pgsql-native_copy-t' should come before 'pgsql-native_transactions-t'
Hint: run 'python3 /home/runner/work/proxysql/proxysql/test/tap/groups/lint_groups_json.py --fix' to auto-fix
##[error]Process completed with exit code 1.
GitHub Actions: CI-lint-groups-json / 0_lint.txt: docs(pgsql): mark named portals implemented in spec; clear closing_po…
Conclusion: failure
##[group]Run python3 test/tap/groups/lint_groups_json.py
�[36;1mpython3 test/tap/groups/lint_groups_json.py�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
groups.json format lint: 1 error(s) found:
Keys not sorted: 'pgsql-native_copy-t' should come before 'pgsql-native_transactions-t'
Hint: run 'python3 /home/runner/work/proxysql/proxysql/test/tap/groups/lint_groups_json.py --fix' to auto-fix
##[error]Process completed with exit code 1.
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Feature tiers are controlled via flags:PROXYSQL31=1for v3.1.x features (FFTO, TSDB),PROXYSQL40=1for v4.0.x features (plugin loader).PROXYSQL40=1implies bothPROXYSQL31=1andPROXYSQLFFTO=1andPROXYSQLTSDB=1. Use conditional compilation with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Class names usePascalCasewith protocol prefixes:MySQL_,PgSQL_, orProxySQL_(e.g.,MySQL_Protocol,PgSQL_Session).
Member variables usesnake_case.
Constants and macros useUPPER_SNAKE_CASE.
Use C++17; conditional compilation for feature tiers via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSE.
Use RAII for resource management; use jemalloc for memory allocation.
Use pthread mutexes for synchronization; usestd::atomic<>for counters.
Files:
lib/PgSQL_Variables.cpplib/PgSQL_PreparedStatement.cpptest/tap/tests/pg_lite_client.cppinclude/PgSQL_PreparedStatement.hinclude/proxysql_structs.hinclude/PgSQL_Session.hlib/PgSQL_Logger.cpptest/tap/tests/pgsql-native_portals-t.cpplib/PgSQL_Session.cpp
{lib,src}/**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
GenAI/MCP/RAG/LLM features live entirely in
plugins/genai/and load as a.soat runtime viadlopen. Do not guard withPROXYSQLGENAIin core code — that flag no longer guards any core code as of Step 7 of the GenAI plugin carve-out.
Files:
lib/PgSQL_Variables.cpplib/PgSQL_PreparedStatement.cpplib/PgSQL_Logger.cpplib/PgSQL_Session.cpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/. Test binaries are built via pattern rulemake <testname>-twhich compiles<testname>-t.cppinto<testname>-t. Register new tests ingroups.json.
Files:
test/tap/tests/pg_lite_client.cpptest/tap/tests/pgsql-native_portals-t.cpp
{test/infra/**/*.bash,test/tap/**/Makefile,.github/workflows/**/*.yml}
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS use
run-tests-isolated.bashfor running TAP tests. It handles infrastructure setup, ProxySQL start, test execution, and cleanup. Never manually create Docker networks, start containers, or run init scripts.
Files:
test/tap/tests/Makefile
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in headers use
#ifndef __CLASS_*_Hformat (e.g.,#ifndef __MYSQL_PROTOCOL_H).
Files:
include/PgSQL_PreparedStatement.hinclude/proxysql_structs.hinclude/PgSQL_Session.h
🧠 Learnings (3)
📚 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/pg_lite_client.cpptest/tap/tests/pgsql-native_portals-t.cpp
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/plans/2026-07-07-pgsql-native-named-portals-plan.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
docs/superpowers/plans/2026-07-07-pgsql-native-named-portals-plan.md
🪛 LanguageTool
docs/superpowers/plans/2026-07-07-pgsql-native-named-portals-plan.md
[grammar] ~54-~54: Ensure spelling is correct
Context: ...site's libpq-mode bytes identical (same errcode/message/order). - [ ] **P1.2 Registry +...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~92-~92: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...'s'; Execute(p1, 0) → 1×'D' + 'C'. 4. PORTAL_TXN: BEGIN (simple query); bind p1; Syn...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ndefined-cursor error (both legs). 5. PORTAL_SYNC_DESTROY: bind p1 outside txn; Sync...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~94-~94: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...it-txn end) — both legs identical. 6. PORTAL_CLOSE_IDEMPOTENT: Close('P', "nonexiste...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~95-~95: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... per protocol) — proxy must match. 7. PORTAL_ERR_BIND_DUP: Bind p1 twice without clo...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~96-~96: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... backend 42P03 — proxy must match. 8. PORTAL_LIBPQ_MODE_REJECTS: leg B only, with `p...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~97-~97: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...regression guard for invariant 1). 9. PORTAL_UNNAMED_UNCHANGED: unnamed flow through...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (15)
lib/PgSQL_Variables.cpp (1)
267-268: LGTM!lib/PgSQL_PreparedStatement.cpp (1)
179-184: LGTM!test/tap/tests/pg_lite_client.cpp (1)
9-16: LGTM!Also applies to: 308-317, 337-391
lib/PgSQL_Session.cpp (1)
221-228: LGTM!Also applies to: 387-394, 3015-3017, 3095-3113, 3305-3309, 3469-3505, 3580-3595, 6528-6539, 6901-6991, 7130-7213, 7244-7370, 7390-7573, 7585-7613
test/tap/tests/Makefile (1)
367-368: 📐 Maintainability & Code QualityNo action needed
pgsql-native_portals-tis already listed intest/tap/groups/groups.json.> Likely an incorrect or invalid review comment.include/PgSQL_PreparedStatement.h (3)
65-88: Duplicate of the existing describe-cache synchronization finding.The
describe_cacheatomic pointer publication concern on this range was already raised in the prior review. As per coding guidelines, “Use pthread mutexes for synchronization; usestd::atomic<>for counters.”Source: Coding guidelines
32-37: LGTM!
125-131: LGTM!include/proxysql_structs.h (1)
322-332: LGTM!Also applies to: 1146-1146, 1490-1490
include/PgSQL_Session.h (3)
9-11: LGTM!Also applies to: 128-137, 181-186
267-272: 🎯 Functional CorrectnessNo duplicate
pending_named_binddeclaration
include/PgSQL_Session.hdeclarespending_named_bindonly once; the other match iscommit_pending_named_bind().> Likely an incorrect or invalid review comment.
164-167: 🎯 Functional CorrectnessNo action needed:
max_rowsis reset to0inPgSQL_Query_Info::reset_extended_query_info(), and both execute paths assign it explicitly.> Likely an incorrect or invalid review comment.lib/PgSQL_Logger.cpp (1)
1029-1038: LGTM!test/tap/tests/pgsql-native_portals-t.cpp (2)
1-431: LGTM!Also applies to: 490-620, 626-627
622-624: LGTM!
| - [ ] **P1.2 Registry + Bind dispatch.** In `handle_post_sync_bind_message`, named branch: resolve `stmt_client_name` via `local_stmts->find_stmt_info_from_stmt_name` (reuse the function's EXISTING unknown-statement error path — read what it does for unnamed and keep bytes identical); create/overwrite `named_portals[name]` entry holding the released bind_msg + stmt_info; set `CurrentQuery.extended_query_info` (stmt fields, `bind_msg` pointer to the registry-owned message, `stmt_client_portal_name` = registry key c_str, flags SYNC per frame position); `find_or_create_backend`, `status = PROCESSING_STMT_BIND`, `return 1` — mirroring the tail of `handle_post_sync_execute_message` (`:7184-7206`) including `pgsql_real_query` handling (read what Execute transfers there; Bind has no query text — check what DESCRIBE transfers as the closest no-text precedent). | ||
| Overwrite semantics: PostgreSQL errors on Bind to an existing portal name ("portal already exists", 42P03) — DO NOT silently overwrite; pass the Bind to the backend and let it error naturally (registry entry replaced only on successful BindComplete — hook the success path). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '45,70p' docs/superpowers/plans/2026-07-07-pgsql-native-named-portals-plan.md
rg -n "named_portals|BindComplete|portal already exists|42P03|overwrite semantics|success path" docs/superpowers/plans -SRepository: sysown/proxysql
Length of output: 12231
Delay named_portals[name] replacement until BindComplete. docs/superpowers/plans/2026-07-07-pgsql-native-named-portals-plan.md:55-56 The plan writes named_portals[name] before Bind succeeds, but also says the entry should only be replaced after a successful BindComplete. If Bind fails, that would clobber the previous portal state. Stage the new entry separately and publish it only on success, or reject duplicates before mutating the map.
🤖 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 `@docs/superpowers/plans/2026-07-07-pgsql-native-named-portals-plan.md` around
lines 55 - 56, The named portal registry update in handle_post_sync_bind_message
should not replace named_portals[name] before Bind succeeds, because a failed
Bind would overwrite the existing portal state. Keep the released bind_msg and
stmt_info staged separately while preparing CurrentQuery.extended_query_info and
find_or_create_backend, then publish the new registry entry only in the
BindComplete success path; if the portal name already exists, preserve
PostgreSQL’s existing duplicate-portal error behavior instead of mutating the
map early.
| // Named-portal Close (PROCESSING_STMT_CLOSE) has no query text; when true the query | ||
| // branch below logs an empty query instead of a stale CurrentQuery.QueryPointer. | ||
| bool c_stmt_close_no_query = false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Suppress the digest path for query-less Close events too.
c_stmt_close_no_query prevents stale query text from being logged, but let remains SIMPLE_QUERY, so Line 1072 still computes a digest from CurrentQuery.QueryParserArgs for a Close message that has no query.
Proposed fix
uint64_t query_digest = 0;
- if (let != PGSQL_LOG_EVENT_TYPE::STMT_EXECUTE && let != PGSQL_LOG_EVENT_TYPE::STMT_DESCRIBE) {
+ if (c_stmt_close_no_query) {
+ query_digest = 0;
+ } else if (let != PGSQL_LOG_EVENT_TYPE::STMT_EXECUTE && let != PGSQL_LOG_EVENT_TYPE::STMT_DESCRIBE) {
query_digest = GloPgQPro->get_digest(&sess->CurrentQuery.QueryParserArgs);
} else {
query_digest = sess->CurrentQuery.extended_query_info.stmt_info->digest;
}Also applies to: 1039-1046, 1069-1074, 1095-1096
🤖 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 `@lib/PgSQL_Logger.cpp` around lines 1016 - 1018, The query-less Close handling
in PgSQL_Logger still flows through the SIMPLE_QUERY digest path even when
c_stmt_close_no_query is set, so update the logic around c_stmt_close_no_query,
let, and the digest computation in the query branch to skip digest generation
for PROCESSING_STMT_CLOSE events with no query text. Use the existing checks in
PgSQL_Logger::... (the block that logs query text and computes the digest from
CurrentQuery.QueryParserArgs) to route these events away from the SIMPLE_QUERY
case and ensure the Close message logs without a stale digest or query parsing
work.
| bool processing_extended_query = (status == PROCESSING_STMT_PREPARE || | ||
| status == PROCESSING_STMT_EXECUTE || | ||
| status == PROCESSING_STMT_DESCRIBE || | ||
| status == PROCESSING_STMT_BIND || | ||
| status == PROCESSING_STMT_CLOSE); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow restored BIND/CLOSE statuses in pipeline variable sync.
Adding BIND/CLOSE to processing_extended_query makes them enter this pipeline-sync branch, but the restore check still only accepts PROCESSING_STMT_EXECUTE; a valid variable sync before named-portal Bind/Close can now destroy the session.
Proposed fix
- if (status != PROCESSING_STMT_EXECUTE) {
+ if (status != PROCESSING_STMT_EXECUTE &&
+ status != PROCESSING_STMT_BIND &&
+ status != PROCESSING_STMT_CLOSE) {Apply the same condition to the dynamic-variable branch below.
Also applies to: 3417-3424, 3449-3456
🤖 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 `@lib/PgSQL_Session.cpp` around lines 3373 - 3377, The pipeline variable sync
logic in PgSQL_Session::ProcessStartupPacket now treats PROCESSING_STMT_BIND and
PROCESSING_STMT_CLOSE as extended-query states, but the restore guard in the
dynamic-variable branch still only allows PROCESSING_STMT_EXECUTE. Update the
restore condition in the same pipeline-sync path so it applies consistently to
the new BIND/CLOSE states as well, using the existing
processing_extended_query/PROCESSING_STMT_* checks around the variable sync and
restore handling.
| // --- Named-portal lifetime on the ERROR epilogue (Task P2) --- | ||
| // An ErrorResponse aborts the (implicit) transaction; once the backend is | ||
| // back at ReadyForQuery 'I' the server has destroyed all portals, so drop | ||
| // the registry to match (mirrors the rc0 clear). An explicit txn stays 'E' | ||
| // (aborted-until-ROLLBACK) and keeps its portals — they are cleared only | ||
| // when the txn finally ends ('I'), matching PostgreSQL. Guarded on the | ||
| // backend still being the reusable connection; if it was torn down the | ||
| // portals are gone with it and the session either ends (destructor clears | ||
| // via reset()) or reconnects fresh. | ||
| if (processing_extended_query && rc == -1 && myconn && | ||
| myconn->is_connection_in_reusable_state() && | ||
| myconn->native_txn_status == 'I') { | ||
| clear_named_portals(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear named portals when the owning backend is lost.
This only clears the registry when the connection is reusable and reports txn state I. If the backend connection is torn down/reconnected, any named portals on it are gone too, but the session registry can remain populated and keep routing later Execute/Describe/Close calls to stale portal state.
Proposed fix
- if (processing_extended_query && rc == -1 && myconn &&
- myconn->is_connection_in_reusable_state() &&
- myconn->native_txn_status == 'I') {
- clear_named_portals();
- }
+ if (processing_extended_query && rc == -1 && myconn) {
+ const bool backend_lost = !myconn->is_connection_in_reusable_state();
+ if (backend_lost || myconn->native_txn_status == 'I') {
+ clear_named_portals();
+ }
+ }📝 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.
| // --- Named-portal lifetime on the ERROR epilogue (Task P2) --- | |
| // An ErrorResponse aborts the (implicit) transaction; once the backend is | |
| // back at ReadyForQuery 'I' the server has destroyed all portals, so drop | |
| // the registry to match (mirrors the rc0 clear). An explicit txn stays 'E' | |
| // (aborted-until-ROLLBACK) and keeps its portals — they are cleared only | |
| // when the txn finally ends ('I'), matching PostgreSQL. Guarded on the | |
| // backend still being the reusable connection; if it was torn down the | |
| // portals are gone with it and the session either ends (destructor clears | |
| // via reset()) or reconnects fresh. | |
| if (processing_extended_query && rc == -1 && myconn && | |
| myconn->is_connection_in_reusable_state() && | |
| myconn->native_txn_status == 'I') { | |
| clear_named_portals(); | |
| } | |
| // --- Named-portal lifetime on the ERROR epilogue (Task P2) --- | |
| // An ErrorResponse aborts the (implicit) transaction; once the backend is | |
| // back at ReadyForQuery 'I' the server has destroyed all portals, so drop | |
| // the registry to match (mirrors the rc0 clear). An explicit txn stays 'E' | |
| // (aborted-until-ROLLBACK) and keeps its portals — they are cleared only | |
| // when the txn finally ends ('I'), matching PostgreSQL. Guarded on the | |
| // backend still being the reusable connection; if it was torn down the | |
| // portals are gone with it and the session either ends (destructor clears | |
| // via reset()) or reconnects fresh. | |
| if (processing_extended_query && rc == -1 && myconn) { | |
| const bool backend_lost = !myconn->is_connection_in_reusable_state(); | |
| if (backend_lost || myconn->native_txn_status == 'I') { | |
| clear_named_portals(); | |
| } | |
| } |
🤖 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 `@lib/PgSQL_Session.cpp` around lines 3849 - 3862, The named-portal cleanup in
PgSQL_Session’s ERROR epilogue only runs when the reusable connection is still
present and native_txn_status is I, so the portal registry can survive a backend
loss and later point at stale state. Update the clear_named_portals() path in
the extended-query error handling to also clear the registry when the owning
backend has been torn down or reconnected, using the same PgSQL_Session/myconn
checks that distinguish reusable vs. lost connections. Keep the existing
transaction-state behavior, but ensure portal bookkeeping is reset whenever the
backend that owned those portals is no longer valid.
| if (!setNativeMode(admin.get(), true) || !flushBackendPool(admin.get(), BACKEND_HG, saved)) { | ||
| auth_scope.restore(); // BAIL_OUT is exit(255): destructor never runs | ||
| BAIL_OUT("failed to enable native mode / flush pool"); | ||
| return exit_status(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Rollback native mode and pool state before this BAIL_OUT.
If setNativeMode(true) succeeds but flushBackendPool() fails, this exits after mutating global/runtime state and skips the final restore block at Lines 622-624. Do a best-effort rollback before bailing.
Proposed fix
// ---- Native mode + fresh native-only pool for the differential corpus ----
if (!setNativeMode(admin.get(), true) || !flushBackendPool(admin.get(), BACKEND_HG, saved)) {
+ setNativeMode(admin.get(), false);
+ flushBackendPool(admin.get(), BACKEND_HG, saved);
auth_scope.restore(); // BAIL_OUT is exit(255): destructor never runs
BAIL_OUT("failed to enable native mode / flush pool");
return exit_status();
}📝 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.
| if (!setNativeMode(admin.get(), true) || !flushBackendPool(admin.get(), BACKEND_HG, saved)) { | |
| auth_scope.restore(); // BAIL_OUT is exit(255): destructor never runs | |
| BAIL_OUT("failed to enable native mode / flush pool"); | |
| return exit_status(); | |
| if (!setNativeMode(admin.get(), true) || !flushBackendPool(admin.get(), BACKEND_HG, saved)) { | |
| setNativeMode(admin.get(), false); | |
| flushBackendPool(admin.get(), BACKEND_HG, saved); | |
| auth_scope.restore(); // BAIL_OUT is exit(255): destructor never runs | |
| BAIL_OUT("failed to enable native mode / flush pool"); | |
| return exit_status(); |
🤖 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-native_portals-t.cpp` around lines 484 - 487, The
bailout path in the native-mode setup leaves global/runtime state mutated if
`setNativeMode()` succeeds but `flushBackendPool()` fails; update the early-exit
handling in the relevant test setup block to do a best-effort rollback before
`BAIL_OUT`. Use the existing `auth_scope.restore()` pattern and also explicitly
revert the native mode / backend pool state in the same failure branch, so the
later final restore block in the test remains a backup rather than the only
cleanup.
…(design §4) Native-mode backend connections have no libpq handle, so the existing cancel/terminate path (PQgetCancel/PQcancel, PQbackendPID) produced a NULL cancel object and backend PID 0 — an in-flight query on a native connection could not be cancelled or its backend terminated. This closes the gap from the native-protocol design spec §4: "native mode opens a fresh connection and sends a CancelRequest with the stored key". - pg_build_cancel_request(): pure encoder for the fixed 16-byte CancelRequest packet (len 16, code 80877102, pid, secret; big-endian, no type byte). - PgSQL_backend_kill_thread CANCEL_QUERY: when the killed connection is native, open a fresh blocking TCP connection to hostname:port and send the raw CancelRequest carrying (native_backend_pid, native_backend_secret) captured from BackendKeyData, instead of calling PQcancel. - TERMINATE_CONNECTION: for native connections set backend_pid to the real BackendKeyData PID so the libpq pg_terminate_backend() path targets the right backend (PQbackendPID(NULL) was 0). - PgSQL_Backend_Kill_Args carries native_mode + native_secret_key; the two call sites (Session cancel, HostGroups terminate) populate them for native connections. All cancel triggers funnel through handler_again___new_thread_to_cancel_query (client frontend CancelRequest, admin KILL QUERY, query timeout), so this fixes every trigger in native mode. LIMITATION: the CancelRequest is sent over a plain connection (protocol-standard even for TLS sessions). A backend whose pg_hba requires TLS (hostssl-only) will refuse it — the same constraint class PQcancel operates under; documented at the send site. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
New TAP test pgsql-native_cancel-t drives an identical client-visible query
cancellation in both libpq and native backend modes and asserts the outcomes
match. For each mode it starts a long SELECT pg_sleep(30) through ProxySQL,
fires a frontend CancelRequest via PQcancel(), and asserts:
- the query aborts with SQLSTATE 57014 (canceling statement due to user
request) — the empirical libpq-mode bar,
- the cancel takes effect promptly (well under the 30s sleep),
- the client session stays usable afterward (SELECT 1),
- the backend query is actually gone (checked on a DIRECT backend connection
via pg_stat_activity),
- the native phase truly exercised the native path (no libpq fallback), and
- libpq vs native produce the identical outcome.
The libpq phase runs first as the differential bar; the native phase must
match it. Registered in groups.json alongside the sibling native tests
(legacy-g1 and the mysql-* variant groups). 10/10 assertions pass.
Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…ership double-free in auth unit test A1 (HIGH, production): the rc0 epilogue called clear_named_portals() before RequestEnd()/LogQuery(), freeing the Bind-message packet that CurrentQuery.extended_query_info.stmt_client_name still pointed into; PgSQL_Event::write_query_format_2_json then read the freed pointer when eventslog format=2 (JSON) is enabled. Defer the destructive clear_named_portals() call until after RequestEnd() runs (which nulls stmt_client_name via CurrentQuery.end() only after logging), while computing sticky_backend_connection with the same pre-clear-equivalent value it used before, so pinning behavior is unchanged. Audited every other clear_named_portals()/reset() call site for the same hazard; none are affected (RequestEnd already ran first, or the current command's CurrentQuery never references the registry being cleared). A2 (test-only): pgsql_backend_auth-t's loopback-TLS fixtures (cases 11/12) shared one sbio/cbio pair across both SSL objects via SSL_set_bio(), which consumes one reference per BIO role; the second SSL_free() therefore double-freed. Add BIO_up_ref() on each BIO before the second SSL_set_bio(), ported from the wt-asan worktree fix. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…sertion in cancel test Review hardening on the native CancelRequest work (41df3ca/ad4312c72): 1. pg_native_send_cancel_request: the blocking connect() was unbounded — a black-holed backend would park the detached kill thread for the kernel's full connect timeout (~2min). Now: non-blocking connect + poll(POLLOUT) with a 5s bound + SO_ERROR check, then blocking send bounded with SO_SNDTIMEO. Primitive stays self-contained; fd closed on all paths. 2. pgsql-native_cancel-t: the native phase previously proved native-path engagement only by absence of the libpq-fallback tripwire. It now also POSITIVELY asserts the raw-CancelRequest branch ran, via a single-pass scan for the "Canceled query (native) on ... successfully" log line that only that branch emits (combined scan with the fallback regex so the two checks don't consume each other's lines), folded into the case result. 3. pgsql-native_cancel-t: documented why the direct pg_stat_activity check on saved[0] is sufficient (single-backend infra; hostgroups 0/1 point at the same host:port). Also softened the TLS limitation comment per review: PostgreSQL processes CancelRequest at the startup-packet layer before SSL negotiation and pg_hba matching, so a plaintext cancel commonly succeeds even against hostssl-only backends; a refusal is still handled gracefully (error + counter, query runs to completion — same as a lost PQcancel). Verified: make debug clean; container restarted; pgsql-native_cancel-t 10/10 (RC 0) with native_cancel_logged=1 in the native phase. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…t-owner teardown path, eventslog on)
New case PORTAL_STMT_LAST_OWNER: Parse s1 -> Bind p1(s1) -> Close('S', s1)
-> Execute(p1) -> Sync. Closing the statement while its portal lives makes
the portal's registry entry the LAST shared_ptr owner of the statement info;
PostgreSQL keeps the portal executable (both legs return the row), and the
Sync's implicit-txn teardown drops that final reference in the rc0 epilogue
— the exact ordering d561b76 fixed (clear_named_portals() deferred until
after RequestEnd()/LogQuery() has read stmt_client_name/digest for the
eventslog). Runs with the eventslog verified active (infra default
default_log=1/format=2; forced+restored otherwise) so the format=2 JSON
writer actually performs the read the UAF hit. The differential alone can
pass on a lucky heap (the UAF historically fired only under ASAN), so the
case additionally asserts ProxySQL_Uptime stayed monotonic across the case
(no crash/angel restart) and its primary value — making this path exist for
future ASAN runs — is documented in the script comment.
pgsql-native_portals-t: 13/13 ok, RC 0
(case detail: backend='1[]2[]3[]D[..9]C[SELECT 1]Z{I}', native=yes,
uptime_monotonic=349->349, eventslog default_log=1 format=2).
Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…MIT warning storm)
The native path fed PgSQL_ExplicitTxnStateMgr TWICE per query: once from the
native ReadyForQuery ('Z') handler in add_native_backend_message() (added by
e9428cb when native completion bypassed the shared handler epilogue) and once
from PgSQL_Session::handler()'s post-RunQuery rc0 epilogue (the libpq path's
single hook, which native completion also returns through after the extended-
query stmt-pipeline refactor). The first call registered/cleared the txn; the
second re-ran start_transaction()/commit() on the now-updated state and tripped
its 'already/no transaction in progress' warning branch -- once per transaction,
for simple AND extended-protocol BEGIN/COMMIT alike (pgbench -M prepared's per-
COMMIT warning storm, 3.22M lines / ~900MB in a 60s bench run; 0 in libpq mode).
Remove the redundant 'Z'-handler call; handler() owns the single registration for
both modes. handler() fires for every native completion path (simple query,
extended sync-terminated Execute, and extended flush-terminated Execute -- the
last never reaches the 'Z' handler), so no case is left uncovered. libpq behavior
is unchanged. native_txn_status capture and buffer flush in the 'Z' handler stay.
Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
Extend pgsql-native_transactions-t with extended-protocol BEGIN/COMMIT: E0-E2 via PQexecParams (unnamed extended portal), E3-E4 via PQprepare+PQexecPrepared (the exact pgbench -M prepared shape; E4 = 3 cycles). Fold a positive-absence assertion into every case's result_match: scan_native_window() requires ZERO 'no/already transaction in progress' warnings in the native-run log window (applies to the 15 simple cases too -- the double-registration bug hit them as well). Two log-scrape correctness fixes needed for the assertion to bite: clear the stream eofbit left by drainLogToNow() before scanning (same fix wait_for_log_match documents), and match the case-correct substring -- the log emits capital 'There', RE2 is case-sensitive. Verified: against a server with the bug re-introduced all 20 cases fail (native_txn_warnings=2/6/1); against the fixed server 21/21 green with 0 warnings emitted. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…(§16-§17) Hard-won during the pgsql native-backend work: debug-only harness, shared lib/obj between flavors, file-bind-mounted binary (restart after rebuild, never rebuild mid-run), INFRA type-vs-id confusion, INFRA_ID collisions, docker-start-skips-provisioning. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/PgSQL_Protocol.cpp (2)
2818-2830: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep CommandComplete row-count parsing bounded.
If a malformed
CommandCompletepayload omits the terminating NUL,strtoull()can read pastpayload_len. Parse the trailing digits manually or requiretaglen < payload_lenbefore using C-string APIs.Proposed fix
if (start < end) { // We have a trailing number; this is the affected-rows count. - affected_rows = strtoull((const char*)(payload + start), NULL, 10); + uint64_t parsed = 0; + for (uint32_t j = start; j < end; j++) { + parsed = parsed * 10 + (uint64_t)(payload[j] - '0'); + } + affected_rows = parsed; }🤖 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 `@lib/PgSQL_Protocol.cpp` around lines 2818 - 2830, The CommandComplete row-count parsing in PgSQL_Protocol.cpp is still using a C-string conversion on an untrusted payload slice, which can read past payload_len when the NUL terminator is missing. Update the affected-rows extraction logic in the CommandComplete handling block to stay bounded by payload_len, either by manually accumulating the trailing digits from payload[start..end) or by only calling strtoull after confirming the terminator is within bounds and the slice is safely NUL-terminated.
2747-2756: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard message-size arithmetic before reserving/copying.
payload_len + 4and1 + 4 + payload_lencan wrap for malformed backend frames, leading to undersized allocation followed bymemcpy()of the originalpayload_len.Proposed fix
unsigned int PgSQL_Query_Result::add_native_backend_message(char type, const unsigned char* payload, uint32_t payload_len) { // Reconstruct the raw client-wire message: type(1) + be32 length(4) + payload. // The length field is (payload_len + 4) per the PostgreSQL wire protocol (it // counts itself but not the type byte). + if (payload_len > UINT32_MAX - 4 || payload_len > UINT_MAX - 5) { + result_packet_type |= PGSQL_QUERY_RESULT_ERROR; + return 0; + } const unsigned int size = 1 + 4 + payload_len; const uint32_t wire_len = (uint32_t)(payload_len + 4);🤖 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 `@lib/PgSQL_Protocol.cpp` around lines 2747 - 2756, Guard the message-size arithmetic in PgSQL_Protocol before calling buffer_reserve_space and l_alloc: in the packet-building path around payload_len, verify that payload_len is small enough that both payload_len + 4 and 1 + 4 + payload_len cannot overflow. If the size is invalid, reject the frame or fail early before any allocation/copy, and keep the check close to the code that computes wire_len, size, and uses memcpy so the standalone packet path cannot allocate too small a buffer.
🤖 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-native_transactions-t.cpp`:
- Line 270: The test file currently defines NativeLogScan multiple times,
causing a redefinition compile error. Remove the duplicate struct declaration
and keep only one NativeLogScan definition near the related native transaction
scan test setup, then update any references in the surrounding test code to use
that single definition.
---
Outside diff comments:
In `@lib/PgSQL_Protocol.cpp`:
- Around line 2818-2830: The CommandComplete row-count parsing in
PgSQL_Protocol.cpp is still using a C-string conversion on an untrusted payload
slice, which can read past payload_len when the NUL terminator is missing.
Update the affected-rows extraction logic in the CommandComplete handling block
to stay bounded by payload_len, either by manually accumulating the trailing
digits from payload[start..end) or by only calling strtoull after confirming the
terminator is within bounds and the slice is safely NUL-terminated.
- Around line 2747-2756: Guard the message-size arithmetic in PgSQL_Protocol
before calling buffer_reserve_space and l_alloc: in the packet-building path
around payload_len, verify that payload_len is small enough that both
payload_len + 4 and 1 + 4 + payload_len cannot overflow. If the size is invalid,
reject the frame or fail early before any allocation/copy, and keep the check
close to the code that computes wire_len, size, and uses memcpy so the
standalone packet path cannot allocate too small a buffer.
🪄 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: c4c8f9a1-399e-4705-8a9d-324b79a17541
📒 Files selected for processing (3)
doc/agents/common-mistakes.mdlib/PgSQL_Protocol.cpptest/tap/tests/pgsql-native_transactions-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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-native_transactions-t.cpp
**/*.{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:
test/tap/tests/pgsql-native_transactions-t.cpplib/PgSQL_Protocol.cpp
🧠 Learnings (4)
📚 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-native_transactions-t.cpp
📚 Learning: 2026-04-11T13:16:05.854Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:16:05.854Z
Learning: When validating GitHub-rendered Markdown in this repository (e.g., links that use heading anchors), account for GitHub slug behavior for headings containing an em-dash (—) surrounded by spaces: GitHub strips the em-dash and converts each surrounding space into a hyphen independently, which can produce a double hyphen (--) in the generated anchor. Therefore, do NOT flag as broken links any anchors whose expected slug contains a double hyphen specifically attributable to an em-dash surrounded by spaces in the source heading. (Example: `...vocabulary — read...` -> `...vocabulary--read...`.)
Applied to files:
doc/agents/common-mistakes.md
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
doc/agents/common-mistakes.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
doc/agents/common-mistakes.md
🪛 LanguageTool
doc/agents/common-mistakes.md
[style] ~306-~306: Consider using a different adjective to strengthen your wording.
Context: ...h run-tests-isolated.bash without the full group.
(FULL_ENTIRE)
🔇 Additional comments (3)
doc/agents/common-mistakes.md (1)
275-292: LGTM!Also applies to: 293-306
test/tap/tests/pgsql-native_transactions-t.cpp (1)
172-240: LGTM!Also applies to: 254-269, 271-295, 328-350, 363-400, 402-526, 565-565
lib/PgSQL_Protocol.cpp (1)
2760-2817: LGTM!Also applies to: 2831-2907
Hardening round: cancellation, sanitizer pass, and the promised benchmarkNative query cancellation implemented ( ASAN pass over the whole native suite (dedicated worktree, full sanitizer build): 10 TAP tests + 12 unit binaries green under ASAN; the branch's manual-memory surfaces (portal registry, raw captures, describe cache, framer, builders) produced zero sanitizer records — except one real find: a use-after-free between named-portal teardown and the event logger ( Benchmarking found a third bug before producing numbers: native mode registered explicit transactions twice (per-ReadyForQuery handler + shared epilogue), tripping a per-transaction "no transaction in progress" warning — 3.2M log lines / ~900MB during a single pgbench run. Fixed by removing the duplicate call, with a reviewer-verified invariant proof that the epilogue covers every native completion path ( Benchmark (release build, host-run proxysql, dedicated postgres:16 backend, 54×60s interleaved runs, 3 passes/cell; native-vs-libpq, median tps / proxy CPU):
The read-path gains at equal-or-lower proxy CPU are consistent with the double-copy elimination this PR exists for. The tpcb@c32 cell (fully write-saturated through one backend) is within noise across passes; flagged honestly rather than averaged away. Full methodology + limitations in the branch session reports ( Filed along the way: #5896 (connect-path debug assert on unresolvable host), #5897 (pre-existing native SET-tracking gaps), #5904/#5905/#5906 (concurrent soak, TLS corpus, fake-server robustness harness — the queued testing follow-ups). Agent-facing infra hazard catalog added to Current test matrix at HEAD |
…backend-protocol # Conflicts: # lib/PgSQL_Session.cpp
…mits
The proxy_debug() macro is gated on the admin-debug master switch
(GloVars.global.gdbg, include/proxysql_debug.h): unless admin-debug='true',
every proxy_debug() call is a runtime no-op and no MOD# line is ever written
to the foreground/teed proxysql.log.
The docker-pgsql16-single infra never provisioned admin-debug — unlike every
MySQL infra, whose docker-proxy-post.bash applies conf/proxysql/infra-config.sql
(SET admin-debug='true'; admin-debug_output=2; debug_levels verbosity=7). So
debug-level markers scraped by pgsql-native_prepared-t P25/P26 ("Describe
served from metadata cache", emitted at proxy_debug(PROXY_DEBUG_MYSQL_COM,5))
could never appear -> cache_hits_2nd_mode=0.
Mirror the MySQL convention in config.sql (applied by docker-proxy-post.bash):
enable admin-debug, keep debug_output=2 (debug DB only, no stderr flood), and
set module verbosity=7 (except pkt_array/net). Tests raise debug_output to 3
for their scrape phase via DebugLogScope.
Not a v3.0 code regression: all debug-propagation code (debug.cpp,
proxysql_debug.h, main.cpp gdbg default, ProxySQL_Admin.cpp gdbg/set_variable)
is byte-identical across 181e87c..89ed5b6; the FlushVariableStats admin
refactor is purely additive and debug output works end-to-end once admin-debug
is enabled. This is a latent infra provisioning gap this infra always had.
|
- New infras use dbdeployer (infra-dbdeployer-pgsql17-repl), matching the existing infra-dbdeployer-* convention; first dbdeployer PG infra. - Frame the initial phase as discovery (failure inventory, xfail catalogue), no expectation of 100% success; SP-2 CI is reporting-oriented. - Add backend-protocol mode (pgsql-use_native_backend_protocol off/on) as a first-class test axis, tracking native-backend PR sysown#5882; differential harness grows to 6 targets (proxy-libpq / proxy-native / direct x text/binary). - Reframe LISTEN/NOTIFY as a per-mode contract test; NOTIFY forwarding is owned by sysown#5882 (already ships pgsql-native_notify-t), not this spec.
…consistency hardening (final review) - diff.py: snapshot/restore pgsql-use_native_backend_protocol around the target loop in _run() (shared by run_case/run_case_sql) so a native-mode toggle never leaks into later cases once PR sysown#5882 lands the variable; a pure no-op today since the variable is absent. - conftest.py: pin client_encoding=UTF8 on the proxy DSN, matching targets.py and drivers/python/adapter.py. - behaviors/{connect,prepared,session_isolation}.py: wrap bodies in try/finally so connections close even on assert failure; make PsycopgAdapter.close() idempotent since session_isolation.py's finally may close an already-closed connection. - behaviors/transactions.py: fix stale comment pointing at a nonexistent harness/oracle.py; oracle_w lives in tests/test_routing_oracle.py.
2 participants
What
Replaces libpq on the ProxySQL → PostgreSQL backend data path with a native wire-protocol implementation, behind the runtime flag
pgsql-use_native_backend_protocol(default off; libpq stays compiled in as fallback and as the differential-test oracle). Monitor and plugins keep using libpq.Design specs and implementation plans are in
docs/superpowers/specs/anddocs/superpowers/plans/(2026-06-11 through 2026-07-07).Highlights
COPY ... TO STDOUTstreams natively;COPY ... FROM STDINkeeps the session fast-forward route (byte-equal, zero-copy); a CopyFail safety net turns any unexpected CopyInResponse on the native drive into a clean error instead of a protocol hang.local_stmtsclient registry, per-backend statement reuse and implicit re-Parse are all retained — only the wire layer is swapped (typed Parse/Bind/Describe/Execute/Close/Flush/Sync builders; per-step drain with ack filtering that preserves ProxySQL's BindComplete/CloseComplete/ParseComplete synthesis; pipeline-abort recovery that injects a Sync on mid-frame errors).PgSQL_STMT_Global_info(set-once, atomic publish): repeat Describes are served without a backend round trip, in both backend modes.SQL3_Free_Connections, native query errors misclassified as broken connections, a bare-ack assert crash, a CopyFail partial-send hang.Testing
Differential testing against the libpq path as oracle (a divergence is a hard failure):
pgsql-native_auth_differential,query_differential(16/16),streaming,transactions(16/16),copy(15/15, with truthful per-route coverage reporting),prepared(27/27 strict — no escape hatches; all EXT_* operations native and byte-equal, incl. named statements + DEALLOCATE, mid-frame error recovery positively asserted, cross-mode Describe-cache parity in both directions),notify,stress(200× PREPARE/SELECT/txn across the pool).legacy-g1group: all pgsql tests green; the 10 MySQL-side failures were individually root-caused as unrelated to this branch (PgSQL-only diff; mostly sharedtest.sbtest1contamination between tests — triage notes available, tracking issues to follow).Known limitations / follow-ups
-PLUS-only-server edge cases fall back to libpq at connect time (logged once per backend).https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
Summary by CodeRabbit
tls-server-end-point), with graceful fallback when not available.