Fix Zero-Row SELECT Producing RowModifying Instead of ResultSet

[SeanTAllen]

Change type: Fixed (bug fix)

Bug Description

When a SELECT query matches zero rows, the library delivers a RowModifying result to the ResultReceiver instead of a ResultSet with zero rows. This is incorrect: RowModifying represents write commands (INSERT, UPDATE, DELETE) with an impacted-row count, while ResultSet represents row-returning queries regardless of how many rows they return.

User-visible impact: code that pattern-matches on Result subtypes to distinguish reads from writes gets the wrong branch for zero-row SELECTs. A user writing match result | let r: ResultSet => ... would never enter the ResultSet branch for a legitimate SELECT * FROM t WHERE false.

Root Cause

In _SessionLoggedIn.on_command_complete (session.pony, lines 190-195), the decision between ResultSet and RowModifying uses the accumulated data row count:

if rows.size() > 0 then
  let rows_object = _RowsBuilder(consume rows, _row_description)?
  receiver.pg_query_result(ResultSet(query, rows_object, msg.id))
else
  receiver.pg_query_result(RowModifying(query, msg.id, msg.value))
end

When a SELECT returns zero rows, rows.size() is 0, so the else branch executes and delivers RowModifying.

PostgreSQL Protocol Context

The PostgreSQL simple query protocol distinguishes row-returning queries from non-row-returning queries via the RowDescription message:

Row-returning query (e.g., SELECT), zero results:

Server: RowDescription → CommandComplete("SELECT 0") → ReadyForQuery

Row-returning query (e.g., SELECT), with results:

Server: RowDescription → DataRow* → CommandComplete("SELECT N") → ReadyForQuery

Non-row-returning query (e.g., INSERT):

Server: CommandComplete("INSERT 0 1") → ReadyForQuery

The key signal is: PostgreSQL always sends RowDescription before any row-returning command's CommandComplete, even when zero rows are returned. Non-row-returning commands never send RowDescription. The presence or absence of RowDescription is the correct discriminator, not whether any DataRow messages followed.

Fix

Change the discriminator from row count to row description presence

Replace rows.size() > 0 with a check for whether a RowDescription message was received for the current command.

Type change: Change _row_description from Array[(String, U32)] val to (Array[(String, U32)] val | None). This makes the "not received" state explicit rather than overloading an empty array, which could theoretically represent a row description with zero columns.

• Initialize _row_description to None in _SessionLoggedIn.create
• on_row_description continues to set it to msg.columns (no change). Add a comment on on_row_description documenting the reset contract: any callback that terminates a command (on_command_complete, on_error_response, on_empty_query_response) must reset _row_description to None and consume _data_rows. This is the point of breakage — a future maintainer adding a new command-terminating callback would need to know about this contract.
• on_command_complete matches on _row_description:
  • Array[(String, U32)] val present: this is a row-returning query. Build a ResultSet (even if the Rows object contains zero rows). Reset _row_description to None.
  • None: this is a non-row-returning query. Build a RowModifying.
• Consume _data_rows via destructive read as before (no change to that mechanism)

Reset _row_description between commands

Currently _row_description is never reset. This is harmless today (the only consumer is _RowsBuilder which is only called when rows.size() > 0), but with the new discriminator it becomes a correctness requirement.

A multi-statement simple query like "SELECT * FROM t; INSERT INTO t2 VALUES (1)" produces:

RowDescription → CommandComplete("SELECT 0") → CommandComplete("INSERT 0 1") → ReadyForQuery

If _row_description is not reset after the first CommandComplete, the INSERT's CommandComplete would see a non-None _row_description and incorrectly produce a ResultSet instead of RowModifying.

Reset _row_description to None in on_command_complete immediately after reading it, alongside the existing destructive read of _data_rows.

Add validation for protocol-inconsistent state

The TODO comment at session.pony:179-189 identifies several consistency checks. With the new discriminator, two become straightforward:

_row_description	_data_rows	Meaning	Action
Present	Non-empty	Normal SELECT with rows	ResultSet
Present	Empty	Zero-row SELECT	ResultSet (empty Rows)
None	Empty	Non-row-returning command	RowModifying
None	Non-empty	Protocol error: data rows without description	DataError

The fourth case (data rows arrived without a preceding row description) indicates a protocol violation. This should be reported to the receiver as DataError.

Note: the existing TODO comment says "we have row description but not rows. that's an error." This is incorrect — that case is precisely the zero-row SELECT, which is legitimate. The fix corrects this understanding.

_RowsBuilder with zero rows

_RowsBuilder already handles the zero-row case correctly. When rows' is empty, the loop body never executes, and it returns a Rows wrapping an empty Array[Row]. The Rows.size() method returns 0. No changes needed in _RowsBuilder.

Reset state in on_error_response

When a query fails, PostgreSQL may send RowDescription and/or DataRow messages before the ErrorResponse. For example, a function call in a SELECT could succeed for some rows and then error — the server would send RowDescription, zero or more DataRow messages, then ErrorResponse, then ReadyForQuery. The current on_error_response (session.pony:225-237) delivers the error to the receiver but does not consume _data_rows or reset _row_description. This leaves stale state that would corrupt the next query's result classification.

Add explicit resets in on_error_response: consume _data_rows via destructive read (discarding the partial rows) and reset _row_description to None. This matches the reset pattern in on_command_complete.

on_empty_query_response validation

The existing TODO at session.pony:210-211 notes that _row_description and _data_rows should be empty/unset when EmptyQueryResponse arrives. With _row_description now being (Array[(String, U32)] val | None), this validation is straightforward: unconditionally consume _data_rows and reset _row_description to None first, then check whether the consumed values indicate a protocol violation (row description was not None or data rows was non-empty). If so, report DataError. Otherwise deliver SimpleResult as today. The unconditional reset ensures state is clean regardless of which branch executes.

Reset state in on_ready_for_query

When on_ready_for_query fires with idle status, the query cycle is fully complete. Reset _row_description to None and _data_rows to a fresh empty array. This is a safety net — all earlier callbacks (on_command_complete, on_error_response, on_empty_query_response) should have already reset these fields, but the on_ready_for_query reset ensures no stale state leaks into the next query cycle if a future code change introduces a missed reset path.

Test Plan

Unit test: zero-row SELECT via mock server

Add a unit test using the existing mock-server pattern (see _JunkSendingTestServer, _DoesntAnswerTestServer). The mock server:

1. Accepts connection, sends AuthenticationOk
2. Sends ReadyForQuery(idle) to make the session queryable
3. On receiving a query, sends: RowDescription (one column, text type) + CommandComplete("SELECT 0") + ReadyForQuery(idle) — no DataRow messages

The test client sends a SELECT query and verifies:

• The result is ResultSet (not RowModifying)
• ResultSet.rows().size() is 0
• The command string is "SELECT"

This tests the core fix without requiring a live PostgreSQL instance.

Integration test: zero-row SELECT against live PostgreSQL

Add an integration test that:

1. Sends "SELECT 1 WHERE false" (guaranteed zero rows with known column structure)
2. Verifies the result is ResultSet with zero rows

This validates the fix against real PostgreSQL protocol behavior.

Integration test: multi-statement query with mixed result types

Add an integration test that issues a multi-statement query combining a zero-row SELECT with a write command, verifying that _row_description reset works correctly across command boundaries. For example:

1. Create a temporary table
2. Send "SELECT * FROM tmp WHERE false; INSERT INTO tmp (col) VALUES ('x')" as a single query
3. Verify the first result is ResultSet (zero rows) and the second is RowModifying (1 impacted)
4. Drop the temporary table

Note: this test depends on the multi-statement query support that already exists in the simple query protocol path. The existing _AllSuccessQueryRunningClient test helper sends queries sequentially (one at a time, waiting for completion). This test would need a new test client that sends a single multi-statement query and collects multiple results.

The test client receives both results via pg_query_result with the same SimpleQuery object. It distinguishes them by call order: the first call corresponds to the SELECT, the second to the INSERT. Pony guarantees causal message ordering within an actor, and _ResponseMessageParser processes messages sequentially, so the delivery order matches the statement order. The client tracks a call counter and validates each result by index.

Test commands

make unit-tests         # run unit tests only (no postgres needed)
make integration-tests  # run integration tests (requires postgres)
make                    # run all tests

Files Modified

File	Changes
postgres/session.pony	Change _row_description type to (Array[(String, U32)] val | None). Update on_command_complete to use row description presence as discriminator, reset _row_description to None after use, add protocol validation for data-rows-without-description. Add state resets in on_error_response for _row_description and _data_rows. Update on_empty_query_response to validate and reset state. Add safety-net reset in on_ready_for_query. Remove resolved TODO comments.
postgres/_test.pony	Register new test classes.
postgres/_test_query.pony	Add zero-row SELECT integration test and multi-statement mixed-result integration test with corresponding test actors.

A new file may be needed if the unit test's mock server infrastructure is large enough to warrant separation, but ideally the mock server actors live alongside the existing mock servers in _test.pony.

[SeanTAllen]

this is addressed via #65