Query-in-Flight Sub-State Machine Design #64
Closed
SeanTAllen
started this conversation in
Research
Replies: 1 comment
-
|
this is mostly addressed via #67 |
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.
-
Goal
Replace the boolean flags (
_queryableand_query_in_flight) in_SessionLoggedInwith a proper sub-state machine that makes illegal query states unrepresentable and provides a natural extension point for the extended query protocol.This is a prerequisite for the prepared statements work (discussion #62). With two query protocols (simple and extended), the booleans cannot distinguish which protocol is in flight, making it impossible to validate that the correct acknowledgment messages are arriving for the active protocol.
Current State
_SessionLoggedIntracks query execution using two boolean flags and two data fields:These encode three logical states:
_queryable_query_in_flightfalsefalsetruefalsefalsetrueThe fourth combination (
true,true) is never intentionally reached.The data fields are reset in multiple places:
on_command_completeresets both via destructive-read swap;on_empty_query_responseresets both via destructive-read;on_error_responseresets both explicitly; andon_ready_for_queryresets both when dequeuing a completed query. This belt-and-suspenders approach prevents stale data but spreads the cleanup logic across four methods.Problems
Repeated guards: Every query callback has an identical
if _query_in_flight then ... else shutdown(s) endguard — five places today (on_command_complete,on_data_row,on_row_description,on_empty_query_response,on_error_response), growing to eleven with prepared statements.No protocol distinction: With both simple and extended query protocols, the boolean cannot distinguish which protocol is in flight.
ParseCompletearriving during a simple query should trigger shutdown, but the boolean-guarded approach cannot express this.Double-delivery on failed transactions: When
on_ready_for_queryarrives with non-idle status during an in-flight query, the current code does not dequeue the query — but its error has already been delivered viaon_error_response. Ifclose()is called afterward,on_shutdowniterates the queue and sendsSessionClosedto the same receiver, resulting in a double delivery ofpg_query_failed.Scattered cleanup: Per-query data (
_data_rows,_row_description) lives on_SessionLoggedInand must be explicitly reset in every command-terminating callback. With the data owned by the in-flight state instead, cleanup is structural — the data is destroyed when the state transitions out.Design
The sub-state machine follows the same pattern as the outer session state machine: an interface defines the callbacks, a trait provides defaults for states that share behavior, and concrete classes implement state-specific logic. The sub-state is entirely internal to
_SessionLoggedIn— no public API changes._QueryStateInterfaceDefines callbacks for all query-related protocol messages, plus
try_run_queryfor attempting to start the next queued query.Query state methods receive both
Session ref(for connection operations likesendandshutdown) and_SessionLoggedIn ref(for queue access and state transitions). This mirrors the outer state machine where state methods receiveSession refand sets.state._QueryNoQueryInFlightTraitProvides default shutdown behavior for states where no query is in flight. Query data callbacks and result callbacks trigger
shutdown— consistent with the existing pattern where unexpected server messages during authenticated state cause a graceful shutdown rather than a panic.The trait leaves
on_ready_for_queryabstract — each concrete no-query-in-flight state handles it differently.Concrete States
_QueryNotReadyServer has not signaled readiness. This is the initial state after authentication and the state after a non-idle
ReadyForQuery(failed transaction).on_ready_for_query(idle): transition to_QueryReady, calltry_run_queryon_ready_for_query(non-idle): remain in_QueryNotReadyshutdown(s)(via_QueryNoQueryInFlight)try_run_query: no-op (via_QueryNoQueryInFlight)_QueryReadyServer is idle and ready to accept a query. This is a transitional state — if the queue is non-empty,
try_run_queryimmediately transitions to an in-flight state.on_ready_for_query:shutdown(s)— the server only sendsReadyForQueryin response to a query cycle or Sync, never unsolicited. Receiving it when no query is in flight indicates a protocol anomaly.shutdown(s)(via_QueryNoQueryInFlight)try_run_query: if queue non-empty, send query, transition to_SimpleQueryInFlight_SimpleQueryInFlightSimple query protocol in progress. Owns the per-query accumulation data (
_data_rowsand_row_description), which are created fresh for each query and destroyed when the state transitions out.on_data_row: accumulate row dataon_row_description: store column metadataon_command_complete: build and deliver result to receiver, reset accumulation data for potential multi-statement querieson_empty_query_response: validate no data accumulated, deliverSimpleResultto receiveron_error_response: deliver error to receiveron_ready_for_query(idle): dequeue completed query, transition to_QueryReady, calltry_run_queryon_ready_for_query(non-idle): dequeue completed query, transition to_QueryNotReadytry_run_query: no-op (query already in flight)The
on_command_completelogic preserves the existing dispatch on_row_description: when aRowDescriptionwas received (non-None), the result is aResultSet— even if zero data rows arrived (a SELECT returning no matches). When noRowDescriptionwas received (None), the result isRowModifying— unless data rows somehow accumulated without a description, which is aDataError.The
on_empty_query_responsevalidation matches the current code: if any data rows or a row description accumulated beforeEmptyQueryResponsearrived, that's a protocol anomaly and the query fails withDataError.The
on_error_responseresets_data_rowsand_row_descriptionbefore delivering the error. While the data would be abandoned on the next state transition anyway, the reset ensures the in-flight state is clean for potential subsequent messages in a multi-statement query (matching the current code's behavior).State Transition Diagram
Changes to
_SessionLoggedInRemove:
var _queryable: Boolvar _query_in_flight: Boolvar _data_rows: Array[Array[(String|None)] val] isovar _row_description: (Array[(String, U32)] val | None)Add:
var _query_state: _QueryStateKeep unchanged:
_query_queue: Array[(SimpleQuery, ResultReceiver)]_notify,_readbufon_shutdown(iterates queue, sendsSessionClosedfailures). This method does not need to interact with_query_state— the queue drain is sufficient for cleanup, and any per-query data held by the in-flight state is simply abandoned (garbage collected) when_SessionLoggedInis replaced by_SessionClosed.Constructor:
Delegation: Each query callback on
_SessionLoggedIndelegates to_query_state:execute: Pushes to queue and asks the current state to run:_run_queryis eliminated. Its logic moves into_QueryReady.try_run_query.Data Ownership
Per-query accumulation data (
_data_rowsand_row_description) moves from_SessionLoggedIninto_SimpleQueryInFlight. Each new query creates a fresh_SimpleQueryInFlightwith empty data, and when the query completes, the old state and its data are replaced.The current code resets these fields in four places (
on_command_complete,on_empty_query_response,on_error_response, andon_ready_for_query). The sub-state machine still resets within the command-terminating callbacks (for multi-statement query support), but theon_ready_for_queryreset becomes unnecessary — transitioning to a new state inherently discards the old data.Incidental Fix: Double-Delivery
The current code only dequeues in
on_ready_for_querywhenmsg.idle()is true. WhenReadyForQuery(non-idle)arrives during an in-flight query, the query stays in the queue despite its error already having been delivered viaon_error_response. Ifclose()is called,on_shutdownsendsSessionClosedto the same receiver — a double delivery ofpg_query_failed.The sub-state machine fixes this naturally:
_SimpleQueryInFlight.on_ready_for_queryalways dequeues the completed query before transitioning, regardless of idle status.ReadyForQuery(any status) marks the end of the query cycle, so dequeuing is always correct.The broader transaction issue — session becomes unusable in a failed transaction because
_QueryNotReadyprevents sending new queries — remains. That requiresROLLBACKsupport, which is out of scope.External Interface
No changes.
Session,SimpleQuery,Result,ResultReceiver,SessionStatusNotify, and all other public types remain unchanged. This is a purely internal refactoring.Extension for Prepared Statements
The prepared statements work (discussion #62) will extend this sub-state machine by:
_QueryStatefor extended query protocol messages (on_parse_complete,on_bind_complete,on_no_data,on_close_complete,on_parameter_description,on_portal_suspended).shutdownimplementations for these callbacks to_QueryNoQueryInFlight(unexpected when no query is in flight) and_SimpleQueryInFlight(unexpected during simple query protocol)._ExtendedQueryInFlightimplementing real behavior for the extended query protocol's acknowledgment sequence._QueryReady.try_run_queryto match on query type (SimpleQueryvsPreparedQuery) and create the appropriate in-flight state.This is what discussion #62 means by "distinguishing simple query in flight from extended query in flight at the type level."
Testing
This is a behavior-preserving refactoring (with the minor incidental fix to double-delivery on failed transactions, which has no existing test coverage). All existing unit and integration tests must continue to pass.
No new tests are required for the refactoring itself. The sub-state machine is an internal implementation detail whose correctness is validated by the existing end-to-end test suite.
Build and run:
Files Modified
session.pony— Remove boolean flags and data fields from_SessionLoggedIn. Add_query_statefield. Delegate query callbacks. Remove_run_query. Add_QueryStateinterface,_QueryNoQueryInFlighttrait,_QueryNotReady,_QueryReady, and_SimpleQueryInFlightclasses.No other files are modified. The
_SessionStateinterface,_ResponseMessageParser, and all public API types are unchanged.Beta Was this translation helpful? Give feedback.
All reactions