feat(storage): rebind transaction and pinned-connection SQL exactly once - #950
Conversation
…torage Callers only use the interface method set; widening the server field, buildGRPCTernClient, and test-helper signatures removes the concrete-type coupling so a second storage backend can be wired without touching callers.
…coupling Rename locals that shadowed the storage package import and add a compile-time storage.Storage conformance assertion in mysqlstore.
…ore core Mechanical move (rename-detected) of the store implementation and its white-box tests; mysqlstore becomes a thin public constructor over the shared core so a second dialect backend can assemble the same store logic with its own dependencies. No SQL or behavior changes.
…apper Stores hold a rebindDB instead of a raw *sql.DB, so every statement executed directly on the pool passes through the dialect's placeholder binder exactly once (identity for MySQL). This is the execution seam a Postgres backend needs to rewrite "?" placeholders to "$n" without touching store SQL. Transaction and pinned-connection handles remain raw passthroughs for a follow-up.
Extend the rebind boundary from direct pool execution to transactions and pinned connections: BeginTx/Conn now return rebind-aware wrappers, so every store statement rebinds its placeholders exactly once at execution time. The advisory-lock flow keeps a sanctioned raw() escape because namedlock.Locker emits engine-native SQL on *sql.Conn.
…ore core (#948) Mechanical move (rename-detected) of the store implementation and its white-box tests; mysqlstore becomes a thin public constructor over the shared core so a second dialect backend can assemble the same store logic with its own dependencies. No SQL or behavior changes.
…apper (#949) * refactor(storage): move mysqlstore internals to shared internal/sqlstore core Mechanical move (rename-detected) of the store implementation and its white-box tests; mysqlstore becomes a thin public constructor over the shared core so a second dialect backend can assemble the same store logic with its own dependencies. No SQL or behavior changes. * feat(storage): route direct pool execution through rebind-aware DB wrapper Stores hold a rebindDB instead of a raw *sql.DB, so every statement executed directly on the pool passes through the dialect's placeholder binder exactly once (identity for MySQL). This is the execution seam a Postgres backend needs to rewrite "?" placeholders to "$n" without touching store SQL. Transaction and pinned-connection handles remain raw passthroughs for a follow-up.
…re-tx-conn-rebind
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head Verdict: correct and complete — the rebind boundary is now genuinely closed, and I couldn't find a path around it. The only thing standing between this and merge is mechanical: the branch conflicts with Findings1. (mechanical, gates merge) The branch is unmergeable until it reconciles with Action items
Verified (tried to break, couldn't)The bypass surface is closed by construction — after this change the only This review was generated by Claude Code (claude-fable-5). |
aparajon
left a comment
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approving on Armand's behalf after the adversarial correctness review above (no blocking correctness findings; merge after reconciling the branch with main). This stamp was left by Claude Code (claude-fable-5).
…contracts Make the two binder bypasses grep-distinct and self-documenting: the advisory-lock escape is lockerConn(), and Raw is lifecycle-only with no SQL allowed through it. Also let the recording driver stub accept transaction options so the isolation-level-bearing BeginTx paths are unit-covered.
|
Review response from Kiran's (@Kiran01bm) AI code review assessment agent Summary: the single (mechanical) finding is fixed — the branch has reconciled with
|
There was a problem hiding this comment.
Pull request overview
This PR completes the SQL placeholder-rebinding boundary in pkg/storage/internal/sqlstore by ensuring that statements executed via transactions and pinned connections also pass through the dialect binder exactly once at the execution edge (closing gaps that would break non-identity binders like Postgres $n).
Changes:
- Wrap
BeginTxandConnto return rebind-aware transaction/connection handles (*rebindTx,*rebindConn) so transactional and pinned-connection execution paths rebind placeholders exactly once. - Thread the wrapper types through store internals (apply/control-request flows) and preserve a single advisory-lock escape hatch via
rebindConn.raw()(to provide the pinned*sql.Conntonamedlock). - Add unit tests asserting “exactly-once” rebinding across pool, transaction, pinned-connection transaction, and that the
raw()escape bypasses the binder.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/storage/internal/sqlstore/db.go | Introduces rebindTx / rebindConn and wraps BeginTx/Conn to enforce exactly-once rebinding at execution. |
| pkg/storage/internal/sqlstore/sql_helpers.go | Updates rollback helper to operate on rebind-aware transactions. |
| pkg/storage/internal/sqlstore/identity.go | Aligns identity insert execution interfaces with rebind-aware pool/tx handles. |
| pkg/storage/internal/sqlstore/applies.go | Threads rebind-aware tx/conn through apply write + advisory-lock plumbing and uses raw() for namedlock boundary. |
| pkg/storage/internal/sqlstore/control_requests.go | Updates control request “FOR UPDATE” helpers to use *rebindTx. |
| pkg/storage/internal/sqlstore/apply_operations.go | Uses pinned raw conn specifically at the namedlock boundary for stranded reaper election. |
| pkg/storage/internal/sqlstore/db_test.go | Adds tests proving rebinding happens exactly once for pool/tx/pinned-tx and is bypassed via raw(). |
| pkg/storage/internal/sqlstore/apply_operations_test.go | Updates namedlock test usage to go through the raw() escape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Raw runs f against the pinned driver connection, for lifecycle control such | ||
| // as discarding a session whose advisory-lock state is uncertain. | ||
| func (c *rebindConn) Raw(f func(driverConn any) error) error { | ||
| return c.conn.Raw(f) | ||
| } |
Summary
Extends the placeholder-rebind boundary from direct pool execution to transactions and pinned connections. Every store statement — pool, transaction, or pinned-connection transaction — now passes through the dialect's binder exactly once, on the final assembled SQL, at the moment it executes.
What
rebindDB.BeginTxreturns a*rebindTxandrebindDB.Connreturns a*rebindConn, so stores can no longer obtain a raw handle that bypasses the binder.rebindTxrebindsExecContext/QueryContext/QueryRowContextand passesCommit/Rollbackthrough;rebindConnbegins rebind-aware transactions on the pinned session.applies.go,apply_operations.go,control_requests.go,sql_helpers.go) now carry the wrapper types through their transaction and lock-connection plumbing.rebindConn.raw()handsnamedlock.Lockerthe pinned*sql.Conn, since locker implementations emit their engine's native placeholders and must run on the session that holds the lock.raw()escape never invokes the binder.Why
The store builds all SQL with MySQL-style
?placeholders; engines with different wire syntax (Postgres$n) rebind at the execution boundary. Until now only direct pool execution was wrapped — statements running inside transactions or on pinned connections reached the driver unrebound, which would break the first non-identity binder. Closing those paths makes the rebind boundary complete before a Postgres dialect lands.