Repository navigation
Conversation
PostgreSQL runs every simple Query in the unnamed statement, so it drops the client's unnamed prepared statement first. Proxy keeps its copy. One test shows the copy surviving a simple Query. Another pipelines a failing batch whose skipped unnamed Parse is rolled back after Proxy has read the following Query, which must not bring the old copy back. The DEALLOCATE ALL test ran the statement as a simple Query, so it only held because of this gap. It now runs DEALLOCATE ALL through the extended protocol, which is where the unnamed statement survives it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Context now releases the unnamed statement when it reads a simple Query, mapped, refused or with mapping disabled, as PostgreSQL does before it runs the Query. PostgreSQL releases it whatever happens to earlier operations, so the rollback snapshots of earlier operations that changed the unnamed statement forget it too, as they do for a SQL DEALLOCATE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… model A property test drives one connection with random extended-protocol and simple Query traffic, and answers each message Proxy forwards from a small model of PostgreSQL: implicit and explicit transactions, failed transactions, skipping until Sync, duplicate names, the unnamed statement and portal, DISCARD ALL, SET LOCAL and temporary tables. After every ReadyForQuery it checks that Proxy holds no statement PostgreSQL released, holds no operation or portal state at idle, and agrees with PostgreSQL on UNSAFE_DISABLE_MAPPING and on whether the temporary table exists. It runs on a plaintext connection and on one with an encrypted column. The module lists what the model leaves out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Automated review attestation (Claude Code /code-review medium + cross-check against head 0e35fc1; no existing PR feedback to dedupe against).
Critical/blocking: none. No correctness bug found in the production change; the unnamed-statement release on a simple Query matches PostgreSQL's ordering. Static review only; I did not run the tests or wait on CI.
Non-blocking:
- minor
packages/cipherstash-proxy/src/postgresql/context/lifecycle.rs:59: noCHANGELOG.md[Unreleased]entry for the unnamed-statement fix, whichCLAUDE.mdasks for on notable changes. - Inline: one
minoron the model's one-directional invariant.
Generated by Claude Code
| for name in &held { | ||
| assert!( | ||
| postgres.statements.contains_key(name), | ||
| "Proxy holds statement {name:?}, which PostgreSQL released" |
There was a problem hiding this comment.
minor check() only asserts Proxy holds no statement PostgreSQL released, never that Proxy still holds every statement PostgreSQL holds. Proxy dropping a statement PostgreSQL kept (e.g. an over-eager release_unnamed_statement) lets the next Bind through unencrypted and still passes. Also assert postgres.statements.keys() ⊆ held, at least on the encrypted connection.
Generated by Claude Code
There was a problem hiding this comment.
Agreed. I checked the reverse direction locally over 3,000 cases per config and it holds, so the assertion goes in with this PR's next code push.
Generated by Claude Code
Requested by Toby
Stacked on #496. I'll retarget it to
mainonce #496 merges.This adds a model-based property test for Proxy's mirror of a connection, and fixes the one bug it found.
Property test
postgresql/middleware/tests/connection_model.rsdrives one connection through the middleware harness with random traffic:SETandSET LOCAL CIPHERSTASH.UNSAFE_DISABLE_MAPPING, BEGIN, COMMIT, ROLLBACK,DISCARD ALL,DEALLOCATE, and creating or dropping a temporary table.A small model of PostgreSQL answers each message Proxy forwards.
It covers implicit and explicit transactions, failed transactions, skipping until Sync, duplicate names, the unnamed statement and portal,
DISCARD ALLrefused in a transaction block,SET LOCALoutside one, and prepared statements not being transactional.Messages Proxy rewrites so that PostgreSQL fails them fail in the model too.
After every
ReadyForQuerythe test checks four things:UNSAFE_DISABLE_MAPPING.It runs on a plaintext connection and on one with an encrypted column, at 256 cases each by default (
PROPTEST_CASESraises that).The module header lists what the model leaves out, mainly pipelining, partial Execute and savepoints.
I checked the test is sensitive by planting faults in the model.
Three of them were caught:
DISCARD ALLkeeping the temporary table, a failed batch not rolling back, andROLLBACKkeeping changes.20,000 cases per config pass with the fix below.
Bug found: a simple Query kept the unnamed statement
PostgreSQL drops the unnamed prepared statement before it runs any simple Query. Proxy kept its copy.
The impact was small: one stale entry, and PostgreSQL rejects any Bind to it.
The fix is test-first:
7023ad0adds two failing harness tests. One covers the plain case. The other covers a pipelined failing batch whose skipped unnamed Parse is rolled back after Proxy has read the following Query.70f85aamakes Context release the unnamed statement when it reads a simple Query (mapped, refused, or with mapping disabled). It also clears earlier rollback snapshots for that statement, the same rule SQLDEALLOCATEalready follows.One existing test changed.
sql_deallocate_all_keeps_the_unnamed_statementsentDEALLOCATE ALLas a simple Query, so it only passed because of this bug. It now runsDEALLOCATE ALLthrough the extended protocol, which is where the unnamed statement survives it.Acknowledgment
By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.
Generated by Claude Code