fix(pgsql-ffto): stop ReadyForQuery finalizing pipelined extended queries (test_ffto_pgsql_pipeline-t) - #6021
Conversation
…ries CI-legacy-g9 failed on test_ffto_pgsql_pipeline-t with a signature that is not a flake but a stats-attribution shift. Scenario 1 pipelines SELECT, INSERT and UPDATE before a single Sync and observed: SELECT rows_affected 0 (exp 0) rows_sent 0 (exp 1) <- got nothing INSERT rows_affected 0 (exp 1) rows_sent 1 (exp 0) <- got SELECT's UPDATE rows_affected 1 (exp 1) rows_sent 0 (exp 0) <- got INSERT's Every result landed one position late, and the totals confirm it: expected sent=1/affected=2, observed sent=1/affected=1, the last CommandComplete dropped once the queue drained to IDLE. Scenario 2 passed only because its ten executions share one digest, which hides any shift. Root cause is the 'Z' branch of process_server_message(), which finalized the current query unconditionally. ReadyForQuery is the correct finalizer for SIMPLE queries -- their CommandComplete deliberately skips finalizing, see the m_current_finalize_on_sync check -- but an extended-protocol Execute is finalized by its own CommandComplete. Finalizing one on ReadyForQuery reports it with zeroed counters and pops the pending deque, so every later response in the batch is attributed to the wrong query. Restrict the 'Z' branch to finalize-on-sync queries. The stray ReadyForQuery comes from the test itself, so fix that too. PgConnection::execute() only writes the Query message and never reads the reply, so three back-to-back setup queries leave three unread ReadyForQuery messages in the socket. consumeInputUntilReady() stops at the FIRST one it sees, so the wait after the Parse batch actually consumed DROP TABLE's reply; from there the client ran a full exchange behind the server while still pipelining, leaving an old ReadyForQuery in flight when the Bind/Execute batch began. Each setup query now consumes its own response. Both halves are worth having: the test is now protocol-correct, and FFTO no longer corrupts stats_pgsql_query_digest for any client that pipelines this way -- silent, plausible-looking wrong numbers rather than a visible error. VERIFICATION -- read this before assuming the fix is proven. The failure is timing-dependent (whether the stray ReadyForQuery reaches the proxy before or after the batch's first Execute) and does NOT reproduce locally: the test passed 5/5 unfixed and 6/6 fixed on this machine, and it failed on a 2-core GitHub runner. The fix therefore rests on code analysis matching the CI signature exactly, not on a red-to-green reproduction. What IS verified is the absence of regression: the full test_ffto_* set was run with and without the lib change and both give an identical 7 pass / 4 fail. Those 4 (test_ffto_mysql_mixed_protocol-t, test_ffto_mysql_transactions-t, test_ffto_pgsql-t, test_ffto_pgsql_concurrent-t) fail identically on unmodified v3.0 in this local environment and pass in CI, so they are environment-specific and unrelated -- two of them are MySQL FFTO tests that this PgSQLFFTO change cannot reach at all.
|
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 (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (3)include/**/*.hpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{cpp,h,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/tap/tests/**/*.cpp📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-01-20T09:34:19.124ZApplied to files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe change adds response tracking to FFTO query state. Response terminators now mark queries as observed, and ChangesPostgreSQL protocol flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant PgSQLFFTO
participant PostgreSQL
Client->>PgSQLFFTO: Send Execute/Sync and overlapping Query
PgSQLFFTO->>PostgreSQL: Forward pipelined requests
PostgreSQL->>PgSQLFFTO: Return response terminators
PgSQLFFTO->>PgSQLFFTO: Track response ownership
PostgreSQL->>PgSQLFFTO: Send ReadyForQuery
PgSQLFFTO->>Client: Drain responses with separate metrics
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![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 `@lib/PgSQLFFTO.cpp`:
- Around line 286-288: Track pending Sync response boundaries so
finalize_current_query cannot activate a query queued after Sync before that
Sync emits ReadyForQuery. Update the query activation/finalization flow around
m_current_finalize_on_sync and finalize_current_query while preserving existing
Execute completion behavior. Add a regression test covering Execute, Sync, and a
simple Query pipelined before replies are consumed.
🪄 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: 431524d9-2eb5-44c1-a898-b361e95af3a3
📒 Files selected for processing (2)
lib/PgSQLFFTO.cpptest/tap/tests/test_ffto_pgsql_pipeline-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: build
🧰 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/test_ffto_pgsql_pipeline-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/test_ffto_pgsql_pipeline-t.cpplib/PgSQLFFTO.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/test_ffto_pgsql_pipeline-t.cpp
🔇 Additional comments (1)
test/tap/tests/test_ffto_pgsql_pipeline-t.cpp (1)
152-171: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5972d221c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (m_current_finalize_on_sync) { | ||
| finalize_current_query(); | ||
| } |
There was a problem hiding this comment.
When a valid extended Parse/Bind/Execute contains a nonempty string that parses as empty, such as whitespace or a comment, PostgreSQL returns EmptyQueryResponse (I) followed by ReadyForQuery, rather than CommandComplete. track_query() still records this execution with finalize_on_sync=false, so this guard now ignores its only handled terminator and leaves it current; the next query's CommandComplete is then attributed to the empty query and subsequent digest statistics remain shifted. Handle I as an extended execution completion before suppressing ReadyForQuery finalization.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6021 +/- ##
==========================================
- Coverage 53.06% 49.47% -3.59%
==========================================
Files 478 478
Lines 143736 142825 -911
Branches 36348 36353 +5
==========================================
- Hits 76267 70663 -5604
- Misses 50559 54091 +3532
- Partials 16910 18071 +1161
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…he query kind
Addresses three review findings, all of which show the previous guard used the
wrong discriminator. Gating the 'Z' branch on m_current_finalize_on_sync fixed
the reported pipeline case but left neighbouring orderings broken.
CodeRabbit (Major): Execute + Sync + simple Query pipelined in one client
write. The Execute's CommandComplete finalizes it and activates the queued
simple query; the ReadyForQuery that follows belongs to the *Sync*, i.e. the
exchange that just ended, but the simple query is now current and has
finalize_on_sync=true, so it was finalized with zero counters and its own
CommandComplete discarded. Unlike the original report this ordering is
deterministic, not a race -- it follows purely from what the client sends.
codex (P1): an extended Execute whose text is empty once comments/whitespace
are stripped is answered by EmptyQueryResponse ('I'), never CommandComplete.
With 'Z' suppressed for extended queries it was never finalized at all, so it
stayed current and swallowed the next query's CommandComplete.
gitar-bot: same shape for PortalSuspended ('s') on a row-limited Execute.
Latent today -- Execute's max-rows field is parsed but never acted on
(issue #5900) -- but it stalls the queue the day that is fixed.
The unifying fix: track whether the current query has received its OWN
response terminator (m_response_seen, set by 'C', 'I' and 's') and gate 'Z'
on that instead of on the query kind. This is correct for every ordering. A
simple query that got its CommandComplete finalizes on ReadyForQuery exactly
as before. An extended Execute finalizes on its own terminator and cannot
still be current when ReadyForQuery arrives. A query still awaiting its first
response -- whether because a stale ReadyForQuery is in flight or because it
was activated mid-batch -- is left alone for the response that is genuinely
its own. 'I' and 's' also finalize extended executions directly, so neither
can stall the queue.
Adds Scenario 3 to test_ffto_pgsql_pipeline-t covering the CodeRabbit case
(extended Execute + Sync + simple Query, then assert each digest kept its own
row counts). Plan count 13 -> 19. Because that ordering is deterministic this
is a genuine regression test: on unfixed code the simple query's digest
records rows_sent=0 instead of 1.
Correction to the verification claims in the PR descriptionWhile acting on the review feedback I discovered the local runs I reported were vacuous, and the description is wrong as written. Correcting it here rather than quietly editing it.
Attempting a genuine local run then hit a second problem: What still stands: the regression comparison was like-for-like — the full What no longer stands: any claim that the fix was exercised locally. It has not been. The Scenario 3 added in 72099b6 is a deterministic regression test for the CodeRabbit case (it does not depend on the race), so CI is where both it and the original fix get their first real exercise. |
Code Review ✅ Approved 1 resolved / 1 findingsRestricts ReadyForQuery finalization in PgSQLFFTO to sync-bound queries, resolving the incorrect query completion finding and preventing statistics attribution errors in pipelined extended queries. ✅ 1 resolved✅ Edge Case: Extended query without CommandComplete no longer finalized on 'Z'
OptionsAuto-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 |
|
|
The The job failed in its build step, not in the tests (they were
Nothing in this PR touches libcurl or genai, and the same workflow passed on this branch's previous commit ( |
1 participant
CI-legacy-g9failed ontest_ffto_pgsql_pipeline-twhile running #6020. The PR under test changes no runtime code at all, so this is a pre-existing bug inv3.0that the run merely surfaced — hence a separate PR againstv3.0rather than a fix folded into #6020.The signature
Scenario 1 pipelines three different statements before a single Sync. The observed digest stats:
SELECT val FROM ffto_pg_pipe WHERE id = $1INSERT INTO ffto_pg_pipe VALUES ($1,$2)UPDATE ffto_pg_pipe SET val = $2 WHERE id = $1Every result landed one position late. The UPDATE assertion passed only because INSERT and UPDATE happen to share the same expectation (
affected=1, sent=0). The totals confirm the shift rather than a lag: expectedsent=1 / affected=2, observedsent=1 / affected=1— the final CommandComplete was dropped entirely once the queue drained toIDLE.Scenario 2 passed throughout because its ten executions share a single digest, which makes any shift invisible.
Root cause
PgSQLFFTO::process_server_message()finalized the current query unconditionally on'Z'(ReadyForQuery):ReadyForQuery is the right finalizer for simple queries — their CommandComplete deliberately skips finalizing, which is what the
m_current_finalize_on_synccheck immediately above exists for. But an extended-protocol Execute is finalized by its own CommandComplete, and a pipelined batch has several queued at once. Finalizing one on ReadyForQuery reports it with zeroed counters and pops the pending deque, so every subsequent response in the batch is attributed to the wrong query and the last is lost.The
'Z'branch is now restricted to finalize-on-sync queries.Where the stray ReadyForQuery came from
Not hypothetical — the test manufactures one:
PgConnection::execute()only writes the Query message; it never reads the reply. Three back-to-back setup queries therefore leave three unread ReadyForQuery messages in the socket.consumeInputUntilReady()stops at the first ReadyForQuery it sees. So the wait after the Parse batch actually consumedDROP TABLE's reply, not the one it was waiting for.Each setup query now consumes its own response, so the client is protocol-correct. Both halves are worth keeping: the test no longer desynchronizes, and FFTO no longer corrupts
stats_pgsql_query_digestfor any client that pipelines this way. The corruption mode matters — silent, plausible-looking wrong numbers, not a visible error.Verification — please read before assuming this is proven
The failure does not reproduce locally. It is timing-dependent: whether the stray ReadyForQuery reaches the proxy before or after the batch's first Execute. On this machine the test passed 5/5 unfixed and 6/6 fixed; it failed on a 2-core GitHub runner. So the local passes are not evidence the fix works, and I have not produced a red-to-green reproduction. The fix rests on code analysis that matches the CI signature exactly — including the dropped final CommandComplete, which a simple "counters lagged" explanation cannot account for.
What is verified is the absence of regression. The full
test_ffto_*set was run with and without thelib/PgSQLFFTO.cppchange, rebuilding and restarting the ProxySQL container each time:The 4 failures —
test_ffto_mysql_mixed_protocol-t,test_ffto_mysql_transactions-t,test_ffto_pgsql-t,test_ffto_pgsql_concurrent-t— fail identically on unmodifiedv3.0in this local environment and passed in CI on the same commit range, so they are environment-specific and unrelated. Two of them are MySQL FFTO tests, which aPgSQLFFTOchange cannot reach at all.Making the race deterministic would need fault injection between the proxy's client-side and server-side reads, which is beyond what this fix warrants. CI on this branch is the next real signal.
Related
Surfaced by #6020 (PG protocol/compat test stack SP-1..SP-3), which is test/infra/docs only and does not touch
lib/.Summary by CodeRabbit