sql: unquote prepared statement and portal names before lookup

[antiguru]

Closes: SQL-667

Reported in #38605: DEALLOCATE "_PLAN0x7" cannot release a statement that pgwire's Parse created under that name, and neither can the unquoted spelling. psqlodbc names every statement it prepares _PLAN0x<addr> and later deallocates it, so those deallocations fail with 26000 and abort the surrounding transaction.

Description

Prepared statement and portal names are case-sensitive keys into per-session maps that Parse and Bind populate with the name exactly as it arrived on the wire. The SCL planners took the name via Ident::to_string(), which is the SQL renderer and re-quotes any identifier that cannot be printed bare, so the plan carried a key containing literal quote characters. The quoted spelling then missed on the added quotes, and the unquoted one missed because the lexer folds it to lowercase.

All seven prepared statement and portal sites now take the identifier's raw value. The lexer already folds unquoted identifiers, so this is Postgres' behavior: a quoted name keeps its case, an unquoted one is lowercase. SQL-only use was self-consistent before and stays green, since PREPARE "Foo" and EXECUTE "Foo" move together. Nothing keyed by these names is persisted outside the session, so no migration is needed.

CLOSE and the cursor planners have the same defect and are fixed alongside, since cursors and protocol portals share one namespace too.

Same class of bug as SQL-526, but the opposite fix: variable names are keys into case-insensitive, durably persisted state and want lowercasing, whereas these names are case-sensitive and shared with the wire protocol.

A process abort fixed alongside

ExecuteResponse::ClosedCursor marked the executing portal completed, but Plan::Close has already removed that portal when the statement closes the portal carrying it. The lookup failed an expect, and the enhanced panic hook turns that into process::abort(), dropping every session in the environment.

It predates this PR and is reachable from three plain SQL statements, not only the extended protocol (thanks @ggevay for pinning down the SQL route):

BEGIN;
DECLARE c CURSOR FOR CLOSE c;
FETCH c;

FETCH c executes portal c, whose statement is CLOSE c, so sequencing removes the portal and the completion bookkeeping then reaches for one that is gone. Portal completion is now split per caller: the CLOSE path tolerates the absence, while the DECLARE path, where a successful declare never removes the executing portal, keeps the signal via soft_panic_or_log! instead of an abort.

This is an independent fix riding on this PR; happy to split it out if reviewers prefer.

Verification

New pgtest cases in test/pgtest/prepare.pt drive both halves of the shared namespace: a Parse-created _PLAN0x7 released by a quoted DEALLOCATE, a SQL-prepared "Foo" described over the protocol as Foo, and a Bind-created portal P closed by CLOSE "P". test/pgtest-mz/portals.pt covers the self-closing portal over the protocol and test/sqllogictest/cursor.slt over plain SQL. cursor.slt and test/sqllogictest/prepare.slt also pin the quoted-name round trips, including FETCH, and the error text, which no longer double-quotes the name.

🤖 Generated with Claude Code

https://claude.ai/code/session_017ixoBLkhDWjU1xzMqat9cg

[linear-code]

SQL-667

SQL-679

[def-]

QA LLM Review

1. HIGH -- CLOSE of the portal carrying it aborts environmentd

src/pgwire/src/protocol.rs:1947

A portal whose statement is CLOSE <that portal's own name> panics the pgwire connection task on expect("portal should exist"), and since environmentd installs the aborting panic hook, this takes down the whole process. Four extended-protocol messages from any authenticated session are enough. This is not a regression from this diff, but this diff widens the spellings that reach it, so it is worth fixing alongside.

Details

Plan::Close removes the named portal (src/adapter/src/coord/sequencer.rs:597) and returns ClosedCursor; pgwire then unconditionally reaches back for the executing portal to mark it completed (src/pgwire/src/protocol.rs:2118 -> complete_portal). When the closed cursor and the executing portal are the same name, that portal is already gone.

Reproduced against main by appending this to test/pgtest-mz/portals.pt:

Parse   {"query": "CLOSE p"}
Bind    {"portal": "p"}
Execute {"portal": "p"}
Sync

which yields thread 'tokio-rt-worker' panicked at src/pgwire/src/protocol.rs:1947: portal should exist and, in the test harness, a 120s client hang instead of any response. In production mz_ore::panic::install_enhanced_handler (src/environmentd/src/environmentd/main.rs:664) turns that into process::abort(), dropping every session in the environment.

Before this PR only the bare spelling reached it (CLOSE p rendered to p); after it, CLOSE "P" against a Bind-created portal P does too, which is exactly the shared-namespace case the new prepare.pt block exercises from the simple-query side (where portal_name is "", so it stays safe).

Minimal fix, since the state update is only bookkeeping for a portal that may no longer exist:

     fn complete_portal(&mut self, name: &str) {
-        let portal = self
-            .adapter_client
-            .session()
-            .get_portal_unverified_mut(name)
-            .expect("portal should exist");
-        *portal.state = PortalState::Completed(None);
+        // A `CLOSE` can name the very portal that carries it, in which case
+        // sequencing has already removed it.
+        if let Some(portal) = self.adapter_client.session().get_portal_unverified_mut(name) {
+            *portal.state = PortalState::Completed(None);
+        }
     }

The alternative, if you would rather surface it than tolerate it, is to error in Plan::Close when plan.name is the portal currently being executed; either way the .expect() should not stay reachable.

[antiguru]

Confirmed and fixed in 7cd3284. ClosedCursor marks the executing portal completed while Plan::Close has already removed it, so the expect was reachable and the enhanced panic hook turns it into an abort.

Took the tolerant fix: the state update is bookkeeping for a portal nobody reads again, and nothing after that match arm touches it. Matching PostgreSQL, which refuses to drop an active portal, would mean teaching the sequencer which portal is currently executing, which is more than this hazard warrants.

Regression test added to test/pgtest-mz/portals.pt, since succeeding here is a deviation from PostgreSQL.

Agreed this predates the PR: the bare spelling already reached it. Fixing it here because the diff adds spellings that reach the same path.

---

Generated by Claude Code

[antiguru]

buildkite/test/restart-2 failed on build 133326 (7cd3284). It is the only failure: the other 68 contexts are green, including rust-cargo-test, which is the job that runs both the new test/pgtest/prepare.pt cases and the new test/pgtest-mz/portals.pt case.

I have not root-caused it, and I want to be clear that I have not: this environment has no Buildkite access (no bk, no gh, api.buildkite.com returns 401, and bin/ci-failures needs a CI_DASHBOARD_TOKEN that is not set here), so I cannot read the job annotation or log, cannot check whether this signature is a known flake, and cannot re-run the job. I am not calling it a flake on no evidence.

What argues against it being this PR's:

• rust-cargo-test is green. A regression in either commit would surface there first, since that job covers pgwire and the SCL planners.
• restarts runs with parallelism: 2, sharding test/restart/mzcompose.py's workflows by index. Shard 2 is github-2454, audit-log, storage-managed-collections, disabled-cluster-replica-size-survives-restart, mcp-feature-flags, drop-materialize-database, bound-size-mz-cluster-replica-metrics-history, user-id-no-reuse-after-restart, arrangement-sizes-stale-snapshot-after-restart, hydration-history-survives-restart. None issues PREPARE, DEALLOCATE, DECLARE, FETCH, or CLOSE, or binds an extended-protocol portal. The only session-lifecycle workflow, temporary-item-cleanup, is in shard 1, which passed.
• The pgwire commit replaces a panic with a tolerated None. Where the portal exists the behavior is unchanged, so it cannot turn a passing test red.
• The SCL commit changes the key only for identifiers that are not printable bare. Nothing in that shard uses such a name.

Against that, restart-2 did pass on this PR's previous head, so I cannot claim it is red on the base branch either.

No fix ported, because I have no failure to fix yet. Someone with Buildkite access re-running restart-2, or pasting its annotation here, would settle it; I will pick it up either way and keep watching the PR.

---

Generated by Claude Code

[ggevay]

Triggered a Nightly: https://buildkite.com/materialize/nightly/builds/18211

[antiguru]

restart-2 passed on re-run, so it was a flake and buildkite/test is green on 7cd3284. Thanks for the Nightly, watching it.

---

Generated by Claude Code

[antiguru]

buildkite/nightly/security-advisories failed on Nightly 18211. It is the only failure so far; the rest of the Nightly is green or still running, and buildkite/test is green.

Not this PR's, and this time I can show it rather than argue it. This PR changes six files, none of them Cargo.toml, Cargo.lock, deny.toml, or about.toml, so the check's entire input is byte-identical to 09a24b3. I installed the pinned cargo-deny 0.19.8 and reproduced it locally:

error[vulnerability]: Legacy `azure_core` writes the `authorization` header value to logs
  azure_core 0.21.0
  ID: RUSTSEC-2026-0275

A newly published advisory, not a code change. It reproduces on any commit carrying this lockfile, so main's Nightly fails on it too. Re-running would not help, and I have no Buildkite access here in any case.

No fix exists to port: I found no open PR or issue referencing azure_core or RUSTSEC-2026-0275.

The advisory: azure_core 0.21.0 and earlier write the outgoing authorization header (Entra ID bearer tokens, Storage SharedKey signatures, SAS tokens) to logs whenever debug-level logging is enabled, trace level in every affected version. It reaches us through mz-persist -> azure_storage_blobs -> azure_core. Our default tracing filter is info (src/ore/src/tracing.rs), so a default deployment should not hit it, but anything raising MZ_LOG_FILTER to debug/trace for that path would.

Two options, both deliberately not applied here since neither belongs in a pgwire/SCL change, and the first is a security-policy call that is not mine to make:

1. Ignore it in deny.toml, next to RUSTSEC-2026-0174, which is already ignored with the note that the azure_* crates have no safe upgrade:

    # Legacy `azure_core` logs the `authorization` header at debug/trace level.
    # Pulled in transitively via `azure_storage_blobs`. Our default log filter is
    # `info`, so this is not reachable in a default deployment.
    "RUSTSEC-2026-0275",

2. Migrate off the legacy Azure SDK to the current azure_core, which is the real fix and a substantial change of its own.

Happy to open a separate PR or a Linear issue for whichever the team prefers.

---

Generated by Claude Code

[antiguru]

Second Nightly failure, buildkite/nightly/unused-dependencies. Same story as the advisory one, and both are dependency-hygiene jobs that run only in Nightly, so neither has ever run on this branch before.

bin/unused-deps reports dependencies that no target of their crate loads, via rustc's unused_crate_dependencies lint. For this PR to trip it, the diff would have to drop the last use of some declared dependency. It removes no use statement and no crate-qualified path at all: the only import change is adding Ident to an existing use crate::ast::{...} list, and mz_sql_parser was already used throughout both files.

I could not reproduce this one locally the way I did the advisory. It needs cargo check --workspace --lib --bins under a modified RUSTFLAGS, which is a full workspace build this environment cannot complete. So this is reasoning from the diff, not a reproduction, and I am flagging that difference rather than papering over it.

Now 3 failures on this head: these two plus the buildkite/nightly rollup. 137 green, 192 still running. I will follow up if anything else goes red.

---

Generated by Claude Code

[ggevay]

Thank you, LGTM, just some minor things.

[antiguru]

Root-caused restart-2, which failed again on 500bc9b. It is not a flake in the vague sense, and it is not this PR's: this branch was based on 09a24b3 from Sep 1, which predates #38636.

#38636 fixes hydration-history-survives-restart, which is in this shard. Its failure mode is the fixture aborting the run when no trial produced an MV with its persist-sink worker off worker 0, a dice roll with no product defect behind it. That matches the pattern here exactly: three builds on the pre-fix base, two first-try failures and one pass on re-run, while restart-2 is green on 18 other recent PRs that are all based on newer main.

Merged main into the branch in dfa98ad, which brings in #38636. Merge, not rebase, so your review anchors survive; the squash on merge collapses it anyway. The merge was clean and the diff against main is still the same six files.

That also drops my earlier "no fix exists to port" on this check. There was one, it had merged an hour before I looked, and I had checked main's commit statuses rather than main's commits.

---

Generated by Claude Code

[antiguru]

Build 133495 on the merged head. Two things, one good and one needing a re-run I cannot do.

The merge did its job. restart-2 is green, confirming #38636 was the cause. Everything that exercises this PR is green too: rust-cargo-test, all five slt shards, clippy, lint-and-rustfmt.

12 jobs failed in a 90-second infrastructure blip, not from this diff:

12:13:44  dataflow-visualizer        12:14:49  postgres-cdc-2, postgres-cdc-5
12:13:52  console-sql                12:14:52  postgres-cdc-6
12:14:44  postgres-cdc-3             12:15:01  s-and-s-error-reporting-3
12:14:45  postgres-cdc-1             12:15:03  debezium-sql-server, kafka-low-watermark
12:14:48  postgres-cdc-4             12:15:14  kafka-exactly-once

Every one is exit status 128, the git-fatal family, across subsystems that share no product code: Kafka, Postgres CDC, Debezium SQL Server, the console, the dataflow visualizer. Twelve independent regressions with one exit code is not a thing; one shared setup step failing is.

It is bounded in time, not a cascade. Jobs kept starting and passing after the window closed: slt-4 at 12:17:42, rust-cargo-test at 12:17:48, short-zippy at 12:18:17. So the build was not cancelled, and the failures are not collateral from an earlier one; whatever broke was broken only between 12:13:44 and 12:15:14.

Ruling out the alternatives: all twelve were green on 500bc9b, whose diff for my six files is identical to this head. They are green on 13 other recent PRs, one built at 11:44 today. The failures span both agent pools (kafka-* on aarch64, console-sql and dataflow-visualizer on x86-64), so it is not one pool. And no other PR in the repo shows any exit-128 failure, so it was a narrow window rather than a broad outage.

These want a re-run. I have no Buildkite access from this environment, so I cannot trigger one. Everything that tests this PR's behavior already passed.

---

Generated by Claude Code

[antiguru]

checks-parallel-1 failed at 12:26:18 with exit status 1. This one is separate from the exit-128 window I described above, and unlike those, I think it is mine.

commit	contents	checks-parallel-1
688e9a3	scl.rs + tests only	success
7cd3284	+ pgwire tolerance	statuses aged out
500bc9b	+ the soft_panic_or_log! split	failure
dfa98ad	+ merge of main	failure

checks-parallel-2 is green on every one of those, and checks-parallel-1 is green on 8 other recent PRs. Reproducible twice on my heads, green before the pgwire commit: that points at the pgwire change, which is the part of this PR that was never in its original scope.

I cannot get further without the job log, and this environment has no Buildkite access. Two mechanisms I can think of, neither confirmed:

• soft_panic_or_log! fires on the declare path, meaning the executing portal can legitimately be gone after a successful DECLARE and the invariant we agreed on does not hold. Against this: main had expect there, which would have panicked outright, and checks-parallel-1 is green on main and everywhere else.
• Soft assertions are off in that build, so soft_panic_or_log! logs an error rather than panicking, and bin/ci-annotate-errors fails the job on an unknown error in the logs even though the workflow itself passed. That fits exit status 1 and would explain why main's expect never fired.

The second fits the evidence better, but I am guessing without the annotation.

Given the pgwire fix is an independent bug that rode along on this PR, the cleanest resolution is probably to drop it from here and give it its own PR where this can be diagnosed properly, returning this one to the scl.rs shape that was green and that @ggevay reviewed. I would rather not rewrite the branch under review without a word, so I am asking before doing it. If someone can paste the checks-parallel-1 annotation instead, I will just fix it.

---

Generated by Claude Code

[antiguru]

Correction to my previous comment: checks-parallel-1 was not this PR's. @antiguru identified it as a GitHub auth failure and retried the affected jobs, which also accounts for the exit-128 cluster, since a failed auth surfaces as a git fatal.

I had inferred from checks-parallel-1 being green at 688e9a3 and red at both commits carrying the pgwire change that the pgwire change caused it. That inference was wrong: the correlation was with which builds ran during the auth failure, not with the diff. The two mechanisms I speculated about are both moot, and there is nothing wrong with the soft_panic_or_log! split.

No change needed, and the proposal to drop the pgwire commit from this PR is withdrawn. Current state on dfa98ad is 73 green, 0 failures, 8 jobs still re-running.

---

Generated by Claude Code

[antiguru]

Retract the retraction: checks-parallel-1 failed again at 13:22 on the clean retry, after the auth problem was resolved, and it is mine after all. Every other job on dfa98ad went green on retry, 79 of them, so it was isolated and I could find it.

misc/python/materialize/checks/all_checks/session.py:58 asserts the old unquoted spelling:

! EXECUTE prepared_stmt_select(0, 3)
contains: unknown prepared statement prepared_stmt_select

Adding .quoted() in 500bc9b makes that unknown prepared statement "prepared_stmt_select", so the contains: no longer matches. That is why it was green at 688e9a3, which predates the quoting, and red on every commit after it. The mechanisms I speculated about earlier, soft_panic_or_log! and the error-log scan, were both wrong.

Fixed in 21e3061 by updating the assertion. The check sits in validate(), and every upgrade scenario runs Validate only after StartMz(tag=None), so it always sees the current build and needs no version guard. It was the only assertion in the repo on that message.

Sorry for the noise on this one: my first read blamed the pgwire commit on a correlation, the auth failure then looked like it explained everything, and the real cause was a one-line test expectation from the review fix in between.

---

Generated by Claude Code

[antiguru]

checks-parallel-1 passed on 21e3061, so the assertion fix was the right diagnosis and the one genuine failure is resolved.

Build 133503 ended 65 green, 12 red, and every one of the 12 is the exit status 128 auth failure, retries included; the only non-128 entry is the build rollup itself. checks-parallel-2, clippy, dataflow-visualizer, the cluster and debezium jobs and the rest all passed.

So the remaining red is entirely the GitHub auth problem, and a fresh build once that clears should be green. Nothing outstanding from my side: @ggevay's four review comments are implemented and their threads resolved.

---

Generated by Claude Code

[def-]

QA LLM Review

1. MEDIUM -- PreparedStatements check now hard-codes an error message only the new binary produces

misc/python/materialize/checks/all_checks/session.py:58

Platform-check validate() runs against old released binaries in several upgrade scenarios, and the new expectation unknown prepared statement "prepared_stmt_select" cannot match those versions, which still emit the name unquoted. The nightly checks-0dt-upgrade-entire-mz-four-versions step will fail on two of its three validate passes.

Details

ZeroDowntimeUpgradeEntireMzFourVersions (misc/python/materialize/checks/scenarios_zero_downtime.py:349 and :358) runs Validate(mz_service="mz_3") while mz_3 is the promoted leader started with tag=get_previous_version(), and Validate(mz_service="mz_4") while mz_4 runs tag=get_last_version(). Both are published minors that predate this PR, so EXECUTE after DEALLOCATE returns unknown prepared statement prepared_stmt_select from the old sql_bail!("unknown prepared statement {}", name), and testdrive's contains: match fails. Only the final Validate(mz_service="mz_5") sees the current build. Scenario._include_check_class (misc/python/materialize/checks/scenarios.py:139) has no version gate, and PreparedStatements defines no _can_run, so the check runs on every one of those passes. PreflightCheckRollback validates on base_version() too, though that step is currently skipped.

The cheap fix is to stop asserting on the name, since the check is about the deallocate/reuse cycle rather than the message formatting:

             ! EXECUTE prepared_stmt_select(0, 3)
-            contains: unknown prepared statement "prepared_stmt_select"
+            contains: unknown prepared statement

If the quoting itself is worth pinning here, gate it on self.current_version (which start_validate sets from the executor, see continual_task.py:60 for the idiom) with a >= MzVersion.parse_mz("v26.41.0-dev") boundary, and keep the unquoted spelling for older versions.

[antiguru]

Confirmed and fixed in 6e4b5f2. The finding is correct and my earlier claim was wrong.

I had checked scenarios_upgrade.py, where every Validate follows StartMz(tag=None), and generalized from that to "validate always sees the current build". scenarios_zero_downtime.py breaks it: ZeroDowntimeUpgradeEntireMzFourVersions validates against mz_3 (tag=get_previous_version()) and mz_4 (tag=get_last_version()) before the final pass on mz_5 (tag=None). Two of three passes run released binaries that spell the name unquoted, so the assertion I pushed would have failed nightly.

Took the version-agnostic option rather than gating on current_version: the check is about the deallocate and reuse cycle, not the message format, so it now asserts the prefix every version shares. The quoting is not left untested, though. test/sqllogictest/prepare.slt gained an EXECUTE case alongside the existing DEALLOCATE one, and SLT only ever runs the new build:

statement error unknown prepared statement "Nope"
EXECUTE "Nope"

That keeps the assertion in a place where it cannot go stale across versions, and leaves the platform check doing what it is for.

---

Generated by Claude Code