pgwire: report ParameterStatus for DISCARD ALL#37648
Draft
ggevay wants to merge 1 commit into
Draft
Conversation
DISCARD ALL resets all session variables but did not send ParameterStatus messages for the reportable parameters it changed, unlike RESET. Connection poolers that track reported parameters, such as pgbouncer with server_reset_query, silently desynced from the server. PostgreSQL reports every changed GUC_REPORT parameter after DISCARD ALL. SessionVars::reset_all and Session::reset now return the parameters whose value changed, ExecuteResponse::DiscardedAll carries them, and the pgwire and HTTP endpoints report them the same way as transaction ends already do. Also teach the pgtest framework to render ParameterStatus messages when a test explicitly expects them, and add a regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes SQL-485
Motivation
DISCARD ALLresets all session variables but did not sendParameterStatusmessages for the reportable (GUC_REPORT) parameters it changed, unlikeRESET <var>. Connection poolers that track reported parameters silently desync from the server. For example, with pgbouncer andserver_reset_query_always=1, every client'sapplication_namesilently became''on Materialize while PostgreSQL preserves the pooler-tracked value. PostgreSQL reports every changed GUC_REPORT parameter afterDISCARD ALL(verified against Postgres 18 on the raw wire).Description
DISCARD ALLnow sends aParameterStatusmessage for every parameter in the notify set whose value changed as a result of the session reset, matching PostgreSQL's wire behavior.This reuses the existing reporting mechanism instead of hand-emitting messages:
SessionVars::reset_allandSession::resetnow return the parameters whose value changed,ExecuteResponse::DiscardedAllcarries them, and the pgwire endpoint handles them in the same match arm asTransactionCommitted/TransactionRolledBack, which already report changed parameters. The websocket SQL endpoint gets the same treatment, soDISCARD ALLover ws now includes the changed parameters in its result, consistent with how that endpoint reportsSETand transaction ends.One benign divergence from PostgreSQL: Materialize sends
ParameterStatusbeforeCommandComplete, PostgreSQL after. The protocol does not specify an order, and this matches Materialize's existingSET/RESETbehavior.The pgtest framework previously discarded all
ParameterStatusmessages. It now renders them when (and only when) a test'suntilblock explicitly expectsParameterStatus, so existing tests are unaffected.Verification
test/pgtest-mz/discard.ptasserts on the raw wire thatSET application_namefollowed byDISCARD ALLproducesParameterStatus {"name":"application_name","value":""}, including a case with two changed parameters (TimeZoneandapplication_name) reported in one batch.DISCARD ALL(andRESET ALL) emitParameterStatusfor each changed GUC_REPORT parameter, restoring startup-packet values. Materialize already treats startup parameters as session defaults (test_startup_params_survive_reset), so the reported values match.test/sqllogictest/discard.slt, and the mz-sql/mz-pgwire unit tests all pass locally.🤖 Generated with Claude Code