Prepared Statements Design #62
Closed
SeanTAllen
started this conversation in
Research
Replies: 1 comment
-
|
Implemented in PR #70. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Goal
Add parameterized query support using PostgreSQL's extended query protocol. This eliminates SQL injection risks from string interpolation and enables type-safe parameter passing.
Scope
This Implementation
max_rows = 0(complete result sets only)Future Work
Public API
New Types
API Unification (Breaking Change)
Introduce a
Querytype alias:Update
Session,Result, andResultReceiverto useQuery:Migration: Existing code changes
SimpleQuerytoQueryinResultReceiverimplementations andResult.query()call sites. SinceSimpleQueryis a member of theQueryunion, passingSimpleQueryvalues still works unchanged at call sites.Alternative considered: Separate
execute_preparedbehavior. This avoids breaking changes but duplicates the queue/execution infrastructure and forces users to choose upfront which receiver interface to implement, even when they want to handle both query types identically.Files Modified
The breaking change touches these files:
Public API types:
simple_query.pony— no change (stillclass val SimpleQuery)prepared_query.pony—PreparedQueryclassquery.pony—Querytype aliasresult.pony—Resulttrait andResultSet,SimpleResult,RowModifyingchangeSimpleQuerytoQueryresult_receiver.pony—pg_query_failedparameter changes fromSimpleQuerytoQuerySession state machine:
session.pony—Session.executesignature,_SessionState.executesignature, all concrete stateexecutemethods,_SessionLoggedIn.query_queuetype,_SessionLoggedIn.on_shutdown, new_SessionLoggedIndelegation methods, new_SessionStatecallbacks,_NotAuthenticateddefaults,_QueryStatenew callbacks,_QueryNoQueryInFlightdefaults,_QueryReady.try_run_queryquery-type dispatch,_SimpleQueryInFlightshutdown defaults for new callbacks, new_ExtendedQueryInFlightclassProtocol layer:
_message_type.pony— new message type constants_backend_messages.pony— new message classes_response_parser.pony— junk detection fix, new match branches,_ResponseParserResultunion update_frontend_message.pony— new message functions_response_message_parser.pony— routing for new message typesTests:
_test_frontend_message.pony— new frontend message tests_test_response_parser.pony— new parser tests, new test message builders_test_query.pony— new integration tests, update existing receivers (SimpleQuerytoQueryinpg_query_failed)_test.pony— register new tests, update test actors (_DoesntAnswerClientincluding itsSetIs[SimpleQuery]field →SetIs[Query],_ZeroRowSelectTestClient, etc.)Examples:
examples/query/query-example.pony— updatepg_query_failedparameter typeWire Protocol
Extended Query Sequence
All statements and portals are unnamed (lifetime = until next Parse/Bind).
All five frontend messages are sent as a single concatenated buffer via one
send()call. This avoids Nagle's algorithm delays and matches how PostgreSQL pipelining works. Each_FrontendMessagefunction returns its ownArray[U8] valfor independent testability;_QueryReady.try_run_queryconcatenates them before sending.Frontend Messages (additions to
_FrontendMessage)parse(name, query, param_type_oids)'P'bind(portal, stmt, params)'B'describe_portal(portal)'D'execute_msg(portal, max_rows)'E'sync()'S'closeandflushare not needed for unnamed objects but could be added for completeness.Note: The frontend type bytes
'D','E','C'overlap with backend message types (DataRow, ErrorResponse, CommandComplete). This is not a conflict — the parser only handles backend messages. Frontend message functions use char literals directly rather than adding confusingly-named entries to_MessageType.Parse Message Format
For unnamed statements:
stmt_name = "". For server-inferred types:num_param_types = 0(no OID array).Bind Message Format
Text-format shorthand:
num_param_formats = 0(all text),num_result_formats = 0(all text). NULL parameters useval_len = -1with no value bytes.Describe Message Format
Describe portal (not statement) to get RowDescription without ParameterDescription.
Execute Message Format
max_rows = 0means fetch all rows.Sync Message Format
Fixed 5-byte message. Triggers ReadyForQuery response.
Backend Messages (additions to
_backend_messages.pony)'1'_ParseCompleteMessage'2'_BindCompleteMessage'3'_CloseCompleteMessage'n'_NoDataMessage't'_ParameterDescriptionMessageparam_oids: Array[U32] val's'_PortalSuspendedMessageParameterDescription only arrives from Describe(Statement), which we don't use. Including it in the parser for completeness and forward compatibility.
Response Parser Changes
_ResponseParserResultUpdateAll six new message types must be added to the
_ResponseParserResulttype union in_response_parser.pony. Without this, the new match branches cannot return the new types and the code will not compile:Junk Detection Fix
The current parser rejects non-letter message types as junk:
This rejects
'1'(49),'2'(50),'3'(51) as junk, which crashes the session when ParseComplete/BindComplete/CloseComplete arrive. Fix by also accepting digits:New Match Branches
Add cases to
_ResponseParser.apply():'1'-> skip message, return_ParseCompleteMessage'2'-> skip message, return_BindCompleteMessage'3'-> skip message, return_CloseCompleteMessage'n'-> skip message, return_NoDataMessage's'-> skip message, return_PortalSuspendedMessage't'-> parse Int16(num_params) + Int32, return_ParameterDescriptionMessageNew match branches use character literals directly (e.g.,
'1','n') rather than adding entries to_MessageType. The existing_MessageTypenaming convention (authentication_request(),command_complete(), etc.) doesn't extend cleanly to names like "parse_complete" which could be confused with the Parse frontend message. Adding_MessageTypeconstants for these is optional and can be decided during implementation.Implementation coordination: The parser changes (
_ResponseParser+_ResponseParserResult) and the router changes (_ResponseMessageParser) must be done together. If the parser returns new types that the router doesn't handle, they will silently fall through thematchin_ResponseMessageParser.apply(). The implementation phases list these in different phases for conceptual clarity, but they must be committed together.Session State Machine
New Callbacks on
_SessionStateDefault implementations via the trait hierarchy:
_NotAuthenticatedtrait:_IllegalState()for all six new callbacks (consistent with existing query callbacks likeon_command_complete,on_data_row, etc.)_SessionLoggedIn: delegates all six toquery_state(same pattern as existing query callbacks)New Callbacks on
_QueryStateThe
_QueryStateinterface gains six new methods, mirroring the_SessionStateadditions:Default implementations:
_QueryNoQueryInFlighttrait:li.shutdown(s)for all six (consistent with existing defaults foron_command_complete,on_data_row, etc.)_SimpleQueryInFlight:li.shutdown(s)for all six — receiving extended query protocol messages during a simple query is a protocol anomalyNew
_ExtendedQueryInFlightQuery StateA new
_QueryStateimplementation analogous to_SimpleQueryInFlight. Owns per-query accumulation data:Callback behavior:
on_parse_complete: no-op acknowledgmenton_bind_complete: no-op acknowledgmenton_no_data: no-op —_row_descriptionstaysNone(initialized by constructor), andCommandCompleteuses_row_descriptionpresence to choose result typeon_close_complete:li.shutdown(s)— we never send Close, so receiving this is anomalouson_parameter_description:li.shutdown(s)— we Describe portals not statements, so receiving this is anomalouson_portal_suspended:li.shutdown(s)— we usemax_rows = 0, so receiving this is anomalous. Note: if future work enablesmax_rows > 0, this callback would need real handling and_ResponseMessageParserwould need a yield point onPortalSuspendedsimilar toReadyForQueryData accumulation and result delivery (
on_data_row,on_row_description,on_command_complete,on_empty_query_response,on_error_response,on_ready_for_query) are identical to_SimpleQueryInFlight. Both states accumulate rows and metadata in the same fields, and both deliver results to the receiver atquery_queue(0). This is ~80 lines of shared logic. Factoring it into a shared trait is desirable but may be awkward in Pony — traits can't have fields, so theisofield access patterns (destructive reads inon_command_complete) would need accessor methods or a different factoring approach. If a clean trait extraction isn't feasible, parallel implementations are acceptable with a comment linking the two classes as intentional mirrors.try_run_queryis a no-op (same as_SimpleQueryInFlight— once a query is in flight, another cannot start)._QueryReady.try_run_queryChangesCurrently dispatches only simple queries:
Updated to match on query type:
_SessionLoggedInChangesquery_queuetype:Array[(SimpleQuery, ResultReceiver)]changes toArray[(Query, ResultReceiver)]executesignature:SimpleQuerychanges toQuery. The same signature change applies to_SessionState.executeand all four concrete implementations:_SessionUnopened.execute,_SessionClosed.execute,_SessionConnected.execute, and_SessionLoggedIn.executeon_shutdown: loop variable destructure getsQueryinstead ofSimpleQueryquery_state, following the same pattern as existing callbacks:Error Recovery
Extended query error handling works naturally with existing logic:
ErrorResponseand discards all remaining messages in the sequence until it reaches the Sync.on_error_response(delegated through_SessionLoggedInto_ExtendedQueryInFlight) delivers the failure to the receiver — the implementation is the same as_SimpleQueryInFlight.ReadyForQuery.on_ready_for_querydequeues the completed query and transitions to_QueryReadyor_QueryNotReady— again, the same implementation as_SimpleQueryInFlight.The acknowledgment messages (ParseComplete, BindComplete) that may arrive before the failing step are no-ops, so they don't interfere. If the first step (Parse) fails, no acknowledgments arrive at all — only ErrorResponse followed by ReadyForQuery.
EmptyQueryResponseis also possible for extended queries (e.g.,PreparedQuery("", [])) and is handled correctly via the sameon_empty_query_responselogic shared with_SimpleQueryInFlight.Transaction status caveat: The dequeue (
query_queue.shift()) happens unconditionally inon_ready_for_querybefore the idle check, so the completed query is always properly removed. However, if the error occurs inside an explicit transaction (BEGIN/COMMIT), the server sends ReadyForQuery with failed-transaction status ('E'), and the state transitions to_QueryNotReadyrather than_QueryReady. This means the next queued query won't be dispatched until an idle ReadyForQuery arrives (which requires the client to issueROLLBACK). This is correct behavior — you can't execute queries in a failed transaction — but the session has no way to recover without the client knowing to send aROLLBACKviaSimpleQuery. This is a pre-existing limitation that affects both simple and extended queries and is out of scope for this work._ResponseMessageParserRoutingAdd routing for new message types to
_ResponseMessageParser.apply():Testing
Build and Run Commands
Integration tests require a running PostgreSQL 14.5 (
make start-pg-container). See the Makefile for supportedsslvalues.Unit Tests — Frontend Messages
_TestFrontendMessageParse— Parse with empty name, query with$1parameter, zero param type OIDs_TestFrontendMessageParseWithTypes— Parse with explicit param type OIDs_TestFrontendMessageBind— Bind with text params, unnamed portal/statement_TestFrontendMessageBindWithNull— Bind with NULL parameter (val_len = -1)_TestFrontendMessageDescribePortal— Describe with type byte'P'and empty name_TestFrontendMessageExecute— Execute with unnamed portal, max_rows = 0_TestFrontendMessageSync— Fixed 5-byte Sync messageUnit Tests — Response Parser
_TestResponseParserParseCompleteMessage— Verify'1'parsed correctly_TestResponseParserBindCompleteMessage— Verify'2'parsed correctly_TestResponseParserNoDataMessage— Verify'n'parsed correctly_TestResponseParserCloseCompleteMessage— Verify'3'parsed correctly_TestResponseParserParameterDescriptionMessage— Verify't'with param OIDs_TestResponseParserPortalSuspendedMessage— Verify's'parsed correctly_TestResponseParserDigitMessageTypeNotJunk— Verify'1'is accepted by junk detection (and'/'is still rejected)_TestResponseParserMultipleMessagesParseCompleteFirst— ParseComplete + BindComplete + CommandComplete + ReadyForQuery sequence (verify buffer advancement across the full extended query response)Test Message Builders
New
_Incoming*TestMessageclasses for constructing raw protocol bytes:_IncomingParseCompleteTestMessage—'1' + Int32(4)_IncomingBindCompleteTestMessage—'2' + Int32(4)_IncomingNoDataTestMessage—'n' + Int32(4)_IncomingCloseCompleteTestMessage—'3' + Int32(4)_IncomingParameterDescriptionTestMessage—'t' + Int32(len) + Int16(count) + Int32[](oids)_IncomingPortalSuspendedTestMessage—'s' + Int32(4)Integration Tests
_TestPreparedQueryResults—SELECT $1::textwith one parameter, verify result value matches_TestPreparedQueryInsert—INSERT INTO ... VALUES ($1, $2)with text params_TestPreparedQueryNullParam— Parameter withNonevalue, verify NULL handling_TestPreparedQueryMultipleParams— Query with 3+ parameters of different types_TestPreparedQueryNonExistentTable—SELECT * FROM non_existent_table WHERE id = $1with param, verify error delivery_TestPreparedQueryAfterSessionClosed— Execute after close, verifySessionClosederror_TestPreparedQueryEmptyParams— Query with$-parameters but empty params array (server error)_TestPreparedQueryZeroParams— Query with no$-parameters, empty params array (extended protocol exercised with no parameters)_TestPreparedQueryMixedWithSimple— Alternate simple and prepared queries on same sessionImplementation Phases
These phases are for conceptual organization. In practice, the protocol layer (phase 1) and session integration (phase 4) must be committed together so the parser and router stay in sync.
_MessageTypeadditions,_backend_messagesnew classes,_ResponseParserResultunion update,_ResponseParserjunk fix + new branches,_FrontendMessagenew functionsPreparedQuery,Querytype alias, updateResultReceiver,Result, all Result implementations, all existing test actors_SessionStatenew callbacks +_NotAuthenticateddefaults,_QueryStatenew callbacks +_QueryNoQueryInFlightdefaults,_SimpleQueryInFlightshutdown defaults for new callbacks, new_ExtendedQueryInFlightclass,_SessionLoggedIndelegation + type changes,_QueryReady.try_run_queryquery-type dispatch,_ResponseMessageParserroutingDecision Points
These are areas where multiple valid approaches exist. The plan takes a position on each, but alternatives are noted.
Unified
executevs separateexecute_prepared: The plan recommends unifying under a singleexecute(Query, ResultReceiver)(breaking change toResultReceiversignature). The alternative — a separateexecute_preparedbehavior — avoids breaking changes but duplicates infrastructure.Including
closeandflushfrontend messages: Not needed for unnamed objects. Could add now for forward compatibility with named statements, or defer until needed.Single send vs multiple sends: The plan recommends concatenating all five extended query messages into a single buffer before calling
send()once. The alternative — five separatesend()calls — would work functionally but is less efficient. The individual message functions are still separate for testability; concatenation happens in_QueryReady.try_run_query.Shared data accumulation logic:
_ExtendedQueryInFlightand_SimpleQueryInFlighthave ~80 lines of identical logic. A shared trait is preferred but may conflict with Pony's trait limitations (no fields, awkwardisoaccessor patterns). If a clean trait extraction isn't feasible, parallel implementations with cross-referencing comments are acceptable. This decision is deferred to implementation since the right answer depends on what Pony's type system allows.Beta Was this translation helpful? Give feedback.
All reactions