Repository navigation
Tell the session about SET, RESET and set_config - #120
Merged
Merged
Conversation
#23 closed the SET/RESET/DISCARD grammar holes in the middleware and left one seam open: a statement the middleware declined to answer arrived at the session as raw SQL, where an author had to re-parse it to discover it was connection boilerplate at all -- the complaint #23 opened with, narrowed to a couple of statements. Session.set_parameter(name, value) is told about every change the middleware handled, from the single _apply_set_config funnel, so SET, SET LOCAL, RESET, RESET ALL and set_config() all reach it. Does nothing by default. The value is the text the client wrote rather than the parsed one: a session forwarding this to a real backend wants to send what was asked for, and what pg_mimic itself keeps is already parsed in session_vars (#115). Raising PgError refuses the setting, and the connection's own record is rolled back -- including the *absence* of a name never set before, so a refused `SET app.tenant` leaves `current_setting('app.tenant', true) IS NULL` true rather than readable-but-blank. The two are different questions and #32 turns on the difference. The undo is built before the write, since SET LOCAL touches one dict and everything else touches two. RESET ALL now goes through the funnel a name at a time rather than clearing the dicts, which is what lets a session hear it as it would hear `RESET x` written out. A session that refuses one leaves the rest reset -- there is no transaction around a RESET ALL, and that is what Postgres does with a failure partway through. _run_all awaits the effects that need it, since a query may carry several (`SELECT set_config('a','1',false), pg_notify(...)`) and only some reach the session. The store stays on Connection. A bare BaseSession bypasses the middleware chain entirely and the connection still owes ParameterStatus on the wire and still has to answer SHOW, so the session is told rather than asked -- no second source of truth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
set_parameter already had both -- the connection records the change before
telling the session, so the parsed value is in session_vars by the time the hook
runs -- but that ordering was load-bearing and undocumented, which is the kind of
thing that gets discovered rather than read.
SET work_mem = '32MB' -> value='32MB' session_vars['work_mem']=32768
SET row_security='tr' -> value='tr' session_vars['row_security']=True
RESET work_mem -> value=None absent from session_vars
So the hook keeps two arguments rather than three. The parsed value is state, not
part of the event, and every extra positional is one an override has to match.
settings_values is exported alongside describe and middleware, for a session that
needs to read a value this did not hand it -- one a real backend reported back.
It was importable already; now it is public on purpose.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
set_parameter took only the raw text, and the parsed value was reachable because
the connection happens to record before it tells. That worked, and it made the
order of two statements inside _apply_set_config part of a public contract:
moving the hook call above the write would have left every session reading stale
state, silently.
async def set_parameter(self, name, raw_value, parsed_value)
SET work_mem = '32MB' raw='32MB' parsed=32768 (its own unit, kB)
SET row_security = 'tr' raw='tr' parsed=True
SET client_encoding='utf8' raw='utf8' parsed='UTF8'
SET app.tenant = 'acme' raw='acme' parsed='acme' (no type to parse)
RESET work_mem raw=None parsed=None
Both are None for a RESET, which is the only thing None means here -- no
parameter parses to it.
The argument against a third parameter was that an override has to match it. That
does not hold: this hook is not in a release, so there is nothing to break, and
the coupling it removes is real.
SettingValue is exported alongside ResultColumn, being in a public signature now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four statements produce the identical event, and two of them say SET:
RESET work_mem ('work_mem', None, None)
SET work_mem TO DEFAULT ('work_mem', None, None)
SET TIME ZONE LOCAL ('timezone', None, None)
SELECT set_config('work_mem', NULL, ..) ('work_mem', None, None)
`RESET x` *is* `SET x TO DEFAULT` in Postgres -- one statement, one path through
guc.c -- and _set_value has always mapped DEFAULT and LOCAL to None before
anything downstream sees them. Nothing tested that the middle two reach the hook
at all, which is what would break first if RESET were ever routed to a hook of
its own: a session overriding only that would miss two spellings of the same
thing, silently.
Verified against PostgreSQL 18.4 that both really do mean the default:
`SET work_mem TO DEFAULT` reads back 4MB, its boot value.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jbylund
added a commit
that referenced
this pull request
Aug 14, 2026
Part 3 of the #35 stack, on top of #119 and #120. `DISCARD ALL` is what pgbouncer sends between pooled clients, so every pooled deployment sends it — and a session with its own state to drop had no way to hear about it. The statement was answered entirely by the connection. ```python async def discard(self, kind: str) -> None: """Told about DISCARD ALL | PLANS | SEQUENCES | TEMP.""" ``` `TEMPORARY` arrives as `TEMP`, the two being one thing in Postgres, so a session has one spelling to match. ## Two places I deviated from #35's sketch **All four kinds reach the hook, not just `ALL`.** The connection returns early for `PLANS`/`SEQUENCES`/`TEMP` — *"a mimic has none of those to throw away"* — which is true of pg_mimic and not of a session, which may have all three. Keeping the early return would have left three of the four kinds unreachable, and they are the ones where the hook is the *only* thing that could act. **Told before the connection clears, not after.** #35's sketch says "the connection has already cleared the state it owns". That is right for `set_parameter`, which records and undoes on refusal — but undoing a `DISCARD ALL` means snapshotting the whole connection: settings, prepared statements, portals, savepoints, listeners. Telling first buys the same atomicity for nothing, so a session that refuses leaves everything as it was: ```sql SET work_mem = '32MB'; DISCARD ALL; -- ERROR: this pool does not recycle connections SHOW work_mem; -- 32MB, untouched ``` The two hooks now differ in order, deliberately, and each docstring says why. ## What a session sees for DISCARD ALL The statement first, then what it did — one `set_parameter(name, None, None)` per setting still held, from #120: ``` ('discard', 'ALL') ('set', 'work_mem', None) ('set', 'row_security', None) ``` ## Unchanged `25001` still outranks the hook: `DISCARD ALL` inside a transaction is refused before it means anything, so a session is never told about one that cannot run. The narrow forms are still allowed there, as in Postgres. ## Tests 11 in a new `tests/test_discard_hook.py` — every kind, the `TEMPORARY`/`TEMP` equivalence, the narrow forms reaching a session the connection ignores them for, the atomic refusal, both transaction rules, and a session that does not override it being unaffected. Nine fail without the change. Suite: **886 passed, 17 xfailed**. ## Remaining 4. dotted custom GUCs in the SET grammar, and `_PASSED_THROUGH_SETTINGS` reworked — where the `role` / `session_authorization` finding from #119 lands. Co-authored-by: Claude Opus 5 (1M context) <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.
Part 2 of the #35 stack, on top of #119.
#23 closed the SET/RESET/DISCARD grammar holes in the middleware and left one seam open: a statement the middleware declined to answer arrived at the session as raw SQL, where an author had to re-parse it to discover it was connection boilerplate at all — the complaint #23 opened with, narrowed to a couple of statements.
Called from the single
_apply_set_configfunnel, so every route reaches it. Does nothing by default.Both spellings, because a session wants different ones for different jobs
raw_valueis the text the client wrote — what to forward to a real backend.parsed_valueis what it means — what to act on.Both
Noneonly for a reset, which is unambiguous: no parameter parses toNone.Handed over rather than read off
session_vars. The connection records before it tells, so the parsed value is reachable there — but depending on that would make the order of two statements inside_apply_set_configpart of a public contract. Moving the hook call above the write would then leave every session reading stale state, silently.pg_mimic.settings_valuesis now exported, so a session can run the same parser over a value this did not hand it — one a real backend reported back.SettingValueis exported too, being in a public signature.One operation, four spellings
RESET xisSET x TO DEFAULTin Postgres — one statement, one path throughguc.c— and_set_valuehas always mappedDEFAULTandLOCALtoNonebefore anything downstream sees them:Which is why there is one hook rather than
set_parameterplusreset_parameter: two of those saySET, so a reset-specific hook would have nowhere sensible to put them. A session overriding only it would miss two spellings of the same thing, with no symptom. Verified against 18.4 that they really do mean the default —SET work_mem TO DEFAULTreads back4MB, its boot value.Refusal restores absence, not just the previous value
Raising
PgErrorrejects the setting and undoes the connection's record. A refusedSET app.tenanton a name never set before leavescurrent_setting('app.tenant', true) IS NULLtrue, rather than readable-but-blank — different questions, and #32 turns on the difference, being the row-level-security probe for "was this ever set?".The undo is built before the write, since
SET LOCALtouches one dict and everything else touches two.RESET ALL goes through the funnel a name at a time
Rather than clearing the dicts, so a session hears it as it would hear
RESET xwritten out. A session that refuses one leaves the rest reset — there is no transaction around aRESET ALL, and that is what Postgres does with a failure partway through.The store stays on Connection
As #35 argues: a bare
BaseSessionbypasses the middleware chain entirely, and the connection still owesParameterStatuson the wire and still has to answerSHOW. Putting the store onSessionwould mean a fallback for theBaseSessioncase and two things to keep in sync. The session is told, so there is no second source of truth.Tests
15 in a new
tests/test_set_parameter_hook.py, covering each route into the funnel, all four reset spellings, both value spellings, the refusal, the absence-restoring undo, and that a session which does not override the hook is unaffected.Suite: 875 passed, 17 xfailed.
Remaining in the stack
discard()hook_PASSED_THROUGH_SETTINGSreworked — where therole/session_authorizationfinding from An on_execute effect may be awaited #119 landsDotted GUCs already reach the hook through
set_config('app.tenant', ...), which is how the refusal tests here are written; part 4 adds theSET app.tenant = 'acme'spelling.History
The commits show this arriving at a two-argument hook first, with
parsed_valuereachable only throughsession_vars. Review moved it to three: the argument against a third parameter was that overrides have to match it, and that cost is real for a released hook but not for one that has never shipped.